[Enhancement] improve FlowTest + Exception handling interactions

[kdmukai]

Description

Quick Background

A FlowTest sequence normally proceeds from View to View, exactly as if a user was clicking from one screen to the next.

But we also have redirects that are often invisible to the user (e.g. only Native Segwit is enabled, so skip the script type selection View).

There are three types of possible redirects:

• (newest) The View's __init__ determines that the user should be redirected elsewhere and calls View.set_redirect(). In this case the View's run() is never called. This method is preferred but is not widely implemented yet.

• (most common) The View's run() immediately returns a new Destination without ever calling its run_screen(). This was how v0.5.0+ redirects were originally implemented.

• An unhandled exception occurs during any phase (in __init__(), in run() before or after run_screen(), or within the associated Screen.display()). The Controller catches the exception and redirects the user to the UnhandledExceptionView.

The Problem

A FlowStep can specify is_redirect=True to indicate that the expected View should execute, but is expected to immediately return a new Destination without ever running its Screen.

But lack of clarity on the above three distinct redirect scenarios made that one single is_redirect attr ineffective and confusing in FlowTest.run_sequence(). This was wholly my fault, by the way! The original implementation more or less properly handled the middle case (redirect triggered in View.run()), maybe handled the first case correctly (triggered via View.set_redirect()), and definitely did NOT handle the third case correctly (UnhandledExceptionView).

Yes, FlowTests have been passing all this time, but partially due to silent, unnoticed failures. A typical example is that the last or penultimate FlowStep triggers an UnhandledExceptionView but the heart of the FlowTest is already over and so there's no next step (next expected View) that would fail.

But if the penultimate FlowStep lands on UnhandledExceptionView, shouldn't the FINAL step detect some kind of mismatch? This is where the second aspect of the problem comes in: when do we sequence.pop(0) the current FlowStep out of the sequence?

In this example, that final FlowStep was being improperly popped before the FlowTest could verify that we got the expected View. So the FlowTest was totally unaware that our sequence ended on the wrong View.

The Fix

First, just getting mental clarity on those three possible redirect scenarios was huge.

Second, remove ALL the sequence.pop(0) calls that created too many confusing hell scenarios.

Instead, wrap it all in a try...finally block to guarantee that sequence.pop(0) is always called after each iteration AND that it's only called once.

The explicit handling for each of the redirect scenarios:

• __init__.set_redirect() obviously happens before the Screen is run, so put that logic at the front.

• Redirects coming out of View.run() are obvious because we can check the call_count on run_screen().

• And UnhandledExceptionView doesn't actually need any special handling. If the FlowTest was expecting that to be triggered, it would specify is_redirect=True and the next FlowStep would be the UnhandledExceptionView. If the FlowTest was NOT expecting it, either the is_redirect=True would be missing (in which case we raise the new FlowTestUnexpectedRedirectException) or the next FlowStep's expected View won't match when we run the next iteration in the sequence (resulting in a FlowTestUnexpectedViewException).

Additional:

• FlowTestRunScreenNotExecutedException removed as its purpose has been superseded by FlowTestUnexpectedRedirectException. Raised when a redirect occurs but the FlowStep did not specify is_redirect=True.
• FlowTestMissingRedirectException added for the opposite scenario (FlowStep specified is_redirect=True but no redirect happened.
• FlowTest.run_sequence().run_screen() no longer makes any changes to the sequence. Just (simulate) running the f'n Screen.
• Completing the FlowTest sequence is now cleaner. Before, if the last FlowStep's View was, say, MainMenuView but with no user input result specified, the FlowTest would execute the View and its Screen, but would actually create a quiet Exception that we just weren't aware of and wouldn't care about anyway (the View runs as if the Screen returned None, which then causes an IndexError when the View tries to access None to see which item in its button_list the user ostensibly selected).
  • FlowTest can exit on the last item in the sequence if the FlowStep implies no user interaction.
  • Can still exit as before when the sequence list is empty.

---

This pull request is categorized as a:

• Bug fix
• Code refactor

Checklist

• I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

• Yes

I have tested this PR on the following platforms/os:

• Raspberry Pi OS Manual Build
• SeedSigner OS on a Pi0/Pi0W board
• Other

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

[Marc-Gee]

Keith's idea of a large Try... Finally block and then just 1 sequence.pop(0) definitely is an exciting one!

[newtonick]

tACK. I can't say I fully grok this code change. I certainly could not have written the code. However I understand the purpose, scope, and function of the changes in this PR. I'm comfortable merging this PR.