LedgerHQ/ragger

Extend the `navigate_until_text` method

aido opened this issue · 6 comments

aido commented

Would it be possible to extend the navigate_until_text method to navigate_until_text_at_position where x and y positions are provided?
Or use the same navigate_until_text method but add x and y parameters. Ignore position if x and y are not provided but test if text is at that position if x and y are provided.

Hello,
I guess you are trying to distinguished two screen with partially the same content?
Can you share the screen snapshots here?

Do you know that navigate_until_text now support searching for a regex? This might help distinguish screens.
Also, there is the navigate_until_snap function, though I don't really recommend using it unless really necessary.

Regarding your proposal, we can discuss about change in navigate_until_text, but we have to keep in mind that ragger tests needs to be as functional as possible on tests run on device through physical_backend where it is hard for an user to check for x and y values.

aido commented

Hi @xchapron-ledger,

I guess you are trying to distinguished two screen with partially the same content?

Take for example the app-recovery-check application. A user scrolls left-to-right through an alphabet before choosing a letter. When the letter is at a certain position on screen the user chooses it.
Rather than use the compare_screen_with_snapshot or navigate_until_snapmethods with lots of snapshots a test could use "right button press until letter/text is at position x"

Take for example the app-recovery-check application. A user scrolls left-to-right through an alphabet before choosing a letter. When the letter is at a certain position on screen the user chooses it.
Rather than use the compare_screen_with_snapshot or navigate_until_snapmethods with lots of snapshots a test could use "right button press until letter/text is at position x"

Note that current ragger navigation is using a screen comparison between each navigation instruction to make sure the previous instruction has been processed and the device is ready to receive the next.

So I think in your situation I would have go with a compare_screen_with_snapshot, with a reference snapshot for each letter of the alphabet.

@lpascal-ledger You worked on the password app and the recovery one, do you have any opinion on this?
What do you think about precise coordinate text searching on the screen? I think it won't looks great on physical backend, but hopefully this stays a niche usage, and therefore it doesn't really matter?

This discussion pulls a lot of interesting questions on the Navigator.navigate* methods.

Personally I use Navigator.navigate* methods primarily to check the whole interface, so I didn't had needs for text position when I wrote the tests.

On a pure functional level, it would make sense to me to allow checking the text position. This information is provided by Speculos through its events, and so backends could allow some kind of event filtering on methods like compare_screen_with_text (eventually cascading to this function in the case of SpeculosBackend).
Implementing it for physical backend would be a bit complicated, but possible - and it is nice but niche alright.

Abstraction-wise, this feel too specific and low-level for the Navigator methods. I'm already not very comfortable with the current Navigator.navigate* granularity which I find complex enough as it is. I fear adding additional specific behavior would add confusing complexity and call for over specific behaviors.
A possible path to bring flexibility without adding a new method every time could be to have a Navigate.navigate_until(calback) method. User would give a callback() -> bool that would be called at every step to check if the stop condition is reached. That would allow to check for snapshot, text, text at specific position (given the backends offer such capability) or any other specific need.
What do you think of this?

If the expected result is to fasten the test execution however, as @xchapron-ledger mentionned currently they won't be performance benefits from waiting for this specifically positioned text. Snapshot will still be asked to Speculos, with the induced delay we know on graphic tests.

aido commented

HI @lpascal-ledger,

Just to add that this suggestion may be niche but not so niche that it would be useful in only one app; app-recovery-check.
I have written an app called app-seed-tool that is based on app-recovery-check where a navigate_until_text_at_position method would also be useful.

@aido the niche thing I'm talking about is implementing BackendInterface.compare_screen_with_text with a specific position on physical backends, not the suggestion itself.