-
Notifications
You must be signed in to change notification settings - Fork 13
test: Make tests faster, more robust, more consistent #660
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
Conversation
We aren't using Selenium, so we shouldn't be saying we do.
On my relatively fast laptop none of these tests run slow enough to fail due to the default 2s timeout. Only one test takes longer than 1s, and of the 1024ms that that that test takes, 700ms is spent in brower.pause calls inside ssendKeyAndWait.
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 teste execution time on a 2021 MacBook Pro M1 approximately in half, from 42s to 22s.
Rather than waiting for some arbitrary amount of time after right-clicking, wait for the context menu to appear. This fixes the only test that started failing when PAUSE_TIME was set to zero.
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.
maribethb
left a comment
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.
Tests are not passing, not sure if it's due to actual timeout or if the timeout is due to the test hanging after an error
| this.timeout(0); | ||
|
|
||
| // Setup Selenium for all of the tests | ||
| // Clear the workspace and load start blocks. |
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.
Note that unlike the rest of the test files, this one only does this before the entire suite, not before each test. Possibly worth either changing that or adding a comment.
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.
Good point re: documentation. I had noticed the setup vs. suiteSetup inconsistency. While looking in to that I realised that the tests in this suite are almost certainly passing only because of a combination of #632 and the fact that there happen to be a total of exactly the right number of tab stops in the document that tabNavigateToWorkspace loops focus back to where it started.
Unfortunately the test failure is the same in both runs, is not due to a timeout, is not transient, and does not reproduce locally. :-( Now attempting to bisect by pushing some partial reverts to this branch. |
|
Superseded by #692. |
A bunch of small fixes and refactors of tests:
scroll_test.tsrestore window size after it's done.timeout(0)calls.PAUSE_TIMEto 0, making tests run about 50% faster by default.keyboard_mode_test.tsthat was previously depending on a fixed-duration pause.sendKeyAndWaitin almost all places instead ofbrowser.keys.