-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] Enable flow-based test scenarios #339
Conversation
src/seedsigner/helpers/qr.py
Outdated
@@ -48,6 +48,6 @@ def qrimage_io(self, data, width=240, height=240, border=3, background_color="80 | |||
# if qrencode fails, fall back to only encoder | |||
if rv != 0: | |||
return self.qrimage(data,width,height,border) | |||
img = Image.open("/tmp/qrcode.png").resize((width,height), Image.NEAREST).convert("RGBA") | |||
img = Image.open("/tmp/qrcode.png").resize((width,height), Image.Resampling.NEAREST).convert("RGBA") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this removes a future deprecation warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I forgot to note that in the description. The deprecation warning only appears (afaict) when running the test suite, but since I was trying to get everything in there neatly tied up, it was too glaring not to fix, too!
Concept ACK at the least. I really like the idea of being able to test from dev environment and not being forced to test on the RPi. I'll try to remain skeptical about "What am I missing out on by not testing on the RPi?" if testing primarily on dev computer becomes the norm. It's nice to be able to test View flows... but what are we missing out on by completely mocking the screens out of the picture? I've taken a few passes reading only the code changes here, while I try to wrap my head around exactly how this is working. I left some comments on code where I wasn't sure if imports were needed? or maybe if they were temporary/obsoleted adds during your prototyping/dev process? or maybe they are put there because you know you need them to merge with another dev branch that is on deck? I look forward to better understanding this, so please invite me in on the explainer chat sessions. Once this proof-of-concept is fine-tuned, and once I've understood it, I'd likely be game for the monotonous tasks of exhaustively testing our other flows... if that would be useful and could free you up for other evolutions that you're spiking into existence. Nice work! |
@jdlcdl the biggest comprehension hurdle is to understand with patch('seedsigner.gui.screens.SomeExactScreen') as mock_screen:
mock_screen().display.return_value = 2 # mock screen interaction as if user selected button_data[2]
# The patch will now override the normal `SomeExactScreen` when called in the View:
next_destination = my_view.run() But the challenge here is to be able to patch out any kind of Screen without knowing ahead of time which Screen a given View might be instantiating (it would be cumbersome to have to tell That's where the new So now our patched |
@jdlcdl It probably makes sense to have a separate set of tests that ONLY run on the Raspi hardware. There's already a basic logical separation anyway: "business logic"/command and control logic is pure python with no hardware dependencies vs user interaction, camera input, screen output is all hardware-dependent w/no bearing on the biz logic layer. Controller, Views, Destination, BackStack, etc. vs seedsigner.hardware and, to some extent, seedsigner.gui Longer term, we'll want to make that divide explicit as the MicroPython/esp32 codebase nears the finish line. We'll want that biz/routing layer completely identical across the two codebases (e.g. refactor so we can do something like |
Fairly major structural update. Now managing the flows with |
* Modifies Views to make button_data and its options available to external caller (FlowTests). * Adds button_data_selection for tighter integration with Views and better human-readability of FlowStep sequences. * Removes the unique recursive run() call in SettingsEntryUpdateSelectionView.
* funcs internal to test_funcs shouldn't start with "test" * comments in test modules preferred to repeating comments in code * type hinting in code preferred to explaining w/ comments
# Set up the QR decoder here so we can inject data into it in the test suite's | ||
# `before_run`. | ||
self.wordlist_language_code = self.settings.get_value(SettingsConstants.SETTING__WORDLIST_LANGUAGE) | ||
self.decoder = DecodeQR(wordlist_language_code=self.wordlist_language_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a flowtest-todo pattern that I'm just noticing for the first time. Perhaps include this in an eventual flow-test writeup.
@@ -178,7 +180,8 @@ def run(self): | |||
|
|||
# All selects stay in place; re-initialize where in the list we left off | |||
self.selected_button = ret_value | |||
return self.run() | |||
|
|||
return Destination(SettingsEntryUpdateSelectionView, view_args=dict(attr_name=self.settings_entry.attr_name, parent_initial_scroll=self.parent_initial_scroll, selected_button=self.selected_button), skip_current_view=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: delete this comment once understood why this has changed so much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdlcdl The recursive self.run()
made it impossible for the flow-based tests to track the click-to-click progression through the settings update. So instead we just re-call the same View with the same params to achieve the same end result (re-display the associated Screen and await next user input).
This morning, I've finally made it thru a good-faith top-to-bottom review of this pr; I have no concerns that were not expressed in per-line code comments above. I still have a pr that is unmerged on this pr at Keith's repo. In that pr, there are proposed changes that are not yet reflected here (seed views and seed tests), so I didn't leave additional comments here.
Keith, assuming that you're not in the middle of reviewing my commit from yesterday, and regarding the per-line code comments that I left above, I have implemented the "I'm sure these are fine" ones into a single commit with a comment like "pr_339 post-review comments implemented" and pushed them to my repo, so that you'll see them in the pr I've issued to yours. |
Additional test flows and cleanup
Additional test flows (plus other edits) merged into this PR by @newtonick and @jdlcdl. This PR is ready for any final review and merge. |
ACK tested 4ce3fba ...all tests pass on my Ubuntu pc x86_64, and pi2 armv7l |
ACK. I'm merging this with a known issue. #395 causes the tests/test_controller.py to fail because the rotation changed since the test was created. |
I believe this was accidentally removed in PR #339. It's needed in lines 169, 187, and 532.
Depends on #347 and #348.
Goal:
Run full end-to-end user flow test scenarios that verify the routing and internal Controller state.
Akin to Selenium website testing where the test case progresses across a series of UI clicks that lead to further web pages through a scripted test flow.
Approach Overview
A View normally instantiates a specific Screen and waits for the user's interactions with the Screen to be completed, at which point the Screen returns the user's selection/action/info/etc back to the View. The test suite will now mock out the Screen class (thereby avoiding hardware-specific dependencies that can only run on a Raspi) and will return the "user's" input for the given test scenario.
e.g. instead of instantiating and rendering a ButtonListScreen, the test suite's mock will simply report back to the View that the user selected option 2.
unittest
patches (side_effect
andreturn_value
) are used to override the normal behavior, but done so in a way that preserves the normalController
main loop. The overrided values are provided as asequence
list that makes the test flow scenarios easy to read.Some light changes are needed in the Controller, Views, and Destination classes to support these test suite operations.
Test Suite Additions
tests/base.py
contains base classes for tests (BaseTest
) and a variant with specific support for testing flows (FlowTest
).Controller.resume_main_flow
is active.setup_method
which runs before each test in the test class is run.Necessary Changes
Controller
that are only meant to be used by the test suite. They support:initial_destination
input var)Controller
at the end of a test flow (StopFlowBasedTest
)except
handing for a baseLetting the test suite handle its own specific exceptions:
FlowTestUnexpectedViewException,
FlowTestInvalidButtonDataSelectionException, and
FlowTestRunScreenNotExecutedException`.View.run_screen(Screen_cls: Type[BaseScreen], **kwargs)
which instantiates the Screen and calls itsdisplay()
method. The resulting return value when the Screen exits is returned to the View.This is a replacement for the current approach where an explicit Screen class is instantiated by the View, which then calls
Screen.display()
to start the user interaction.With this in place, we can now just mock out the
View.run_screen()
calls and deliver whatever return value our test needs. The test suite code does not need to care about all the different Screen implementation classes; they simply never get instantiated in these tests.Breaks
Destination.run()
into two parts (instantiate the View, run the View) so that the test suite can alter the View after it is instantiated but before it is run. This is necessary for certain Views that require specific workarounds to work with the test suite.Modifies Views to make its button_data options available to external callers (FlowTests).
Removes the recursive run() call in SettingsEntryUpdateSelectionView.