test: Make tests faster, more robust, more consistent #692
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We aren't using Selenium, so we shouldn't be saying we do.
With PAUSE_TIME set to zero, on my relatively fast laptop none of these tests run slow enough to fail due to the default 2s timeous, and even with PAUSE_TIME set to 50ms the slowest test is 1024ms (of which 700ms is spent in brower.pause calls inside sendKeyAndWait). We do want to disable timeouts when debugging tests, both because we might set PAUSE_TIME to a larger value and because we might use the debugger to pause things entirely, but we don't need to disable timeouts when running tests just to check their results.
It's very useful to be able to make tests run more slowly so you can watch them, but they should run as fast as possible by default. This cuts total test execution time on a 2021 MacBook Pro M1 approximately in half, from 42s to 22s.
In most places we were already following browser.keys with a browser.pause(PAUSE_TIME), so using sendKeysAndWait is more succinct; in other places we didn't have the pause but it is not harmful to add a pause (especially now the default PAUSE_TIME is zero) and could be helpful when watching tests run with a non-zero PAUSE_TIME.
Previously this function would just send a bunch of tabs, which depended on focus state being as-on-document-load. Some tests (notably the ones in basic_test.ts) that have only a suiteSetup and not a (per-test) setup method were only were only passing because of the combination of: * Due to issue RaspberryPiFoundation#632, pressing tab when the workspace is focused (and there are no further focusable elements on the page) incorrectly causes focus to move to the first focusable element on the page instead of (as would normally be the case) to the browser controls, and * The fact that the index.html had exactly one additional focusable div on the page, preceding the injection div. This meant that calling tabNavigateToWorkspace when the workspace was already focused would, only by sheer coincidence, result in the focus remaining on the workspace. By explicitly focusing a known element, tabNavigateToWorkspace should work correctly regardless of how many focusable elements are on the page and which one (if any) was focused before the call.
Any time a tabNavigateToWorkspace call is followed by a call to focusOnBlock the former can be removed with no discernable effect except to make tests run slightly faster and with less flashing of the flyout.
cpcallen
force-pushed
the
refactor/tests
branch
from
fe32ccb to
8c39866
Compare
Collaborator
Author
|
@maribethb: If this LGTY then please go ahead and merge it so it does not get stale again. |
These two files were inadvertently omitted from commit 14d619c.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A bunch of small fixes and refactors of tests:
scroll_test.tsrestore the original browser window size after it's done.timeout(0)calls only whenPAUSE_TIMEis nonzero (and make them in every suite).PAUSE_TIMEto0, making tests run about 50% faster.sendKeyAndWaitalmost everywhere (instead ofbrowser.keys).tabNavigateToWorkspaceidempotent, to avoid relying on a bug Tabbing past workspace fails to move focus to browser controls #632.tabNavigateToWorkspacecalls that are immediately followed by afocusOnBlockcall.This is a rebase, with updates, of the changes from (and supercedes) PR #660, and incorporates changes based on the comments thereto.