You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes#746
This PR does a bunch of things, but most importantly it fixes repeatedly failing OS X tests that ran in CI. These fell into two categories:
- A single scrolling test that would always fail due to a dropped rendering frame in headless Chrome at an inopportune point in the test.
- Two `alert` tests that would reliably cause a _different_ test to timeout.
These two fixes required completely different solutions and underwent different investigations. A long discussion of the investigation can be found in the conversation thread in this PR.
### Fixing `alert` dialog problems
These failures were a bit odd. Two flyout tests verify callback logic which is set up in the test to open an alert dialog. These tests pass without an issue. However, when either one of these tests is run exactly before move start tests (both of them). The first move start test will pass, the second will reliably time out in CI on OS X. Sometimes WebdriverIO will include errors about focus issues, but this didn't always happen.
The best guess here is that there's an actual issue in Chrome and/or WebdriverIO specifically with alert dialogs that is putting the framework or browser into a bad state. Blockly shares no state across its tests due to page reloads and no cookies or local storage being leveraged, but forcing a session reload between tests (i.e. a full browser reopen) fixes the issue which seems to confirm that it's an issue in the framework environment and not Blockly. This seems like a reasonable fix.
Note that this is sort of discouraged due to session reloading increasing test runtime, but the CI completions do not seem much slower than before even with the extra reloads. Locally it takes 2-3x longer to run when using headless mode. When using non-headless mode it encounters actual failures, but fortunately this needs to be forced since the setup currently is such that session reloading will only ever happen with headless runs. It also seems likely that forcing session reloads like this will actually improve other behavioral inconsistencies or flakes in the tests, as well.
Session reloading has only been enabled for CI runs of tests since it shouldn't be needed locally, at least per observation (though it can be demonstrated locally using the command: `CI=true npm run test:ci`).
### Fixing rendering synchronization issues
The rendering issue came down to the fact that Blockly can get into a temporarily broken state if its rendering queue is not run. Since Blockly relies on `requestAnimationFrame` for this to occur, dropped frames (which are legitimate according to the spec and something we can observe specifically in Mac CI with headless Chrome) will lead to block positioning not updating when the test expects, leading to a failing test. This can be easily reproduced even on Linux locally by linking against a version of Blockly that forces immediate rendering (e.g. pretending JavaFX is enabled).
The fix was to force the tests to try and trigger a re-render of the workspace at select moments. During investigation it was easiest just to do this everywhere there's a moment to pause execution since that's effectively a time when the test actually wants to wait for things to 'settle', so this is what's actually changed in the PR. All previous moments of pausing execution now route through a specific utility function that also will try to force rendering of Blockly, bypassing any dropped frames that the browser might introduce and ensuring tests are a lot more stable. It seems possible (though it hasn't been proven) that other tests may have also been flaky due to this behavior.
It's important to note that this _might_ be a bug in core Blockly, but the reality is it seems nearly impossible to ever actually occur. Even if a browser dropped or delayed a rendering frame, eventual consistency should kick in since Blockly does deterministically render itself and thus will self-correct on the next render (which is fundamentally why the solution above works as well as it does).
Note also that this fix had a compatibility issue with the dialog fix above and required a special setting to disable synchronization since it's not valid in WebdriverIO to execute browser-side code while a dialog is open (and attempting to detect this results in a significant slowdown in CI runtime and a large increase in console warnings).
### Other test infrastructure improvements
As part of the investigation it was useful to try and see what the tests were doing when they failed. WebdriverIO supports this with a built-in `saveScreenshot` function. All test suites have been updated in this PR to automatically check for failures when they finish each test and, if there's a failure, the current screen state will be snapshotted and uploaded to a zip file (specific to the OS) in GitHub Actions.
This actually did help with determining the root rendering issue because the screenshot showed a correct block layout even though debug logs showed incorrect positions. At first this hinted toward inconsistencies between the data model and actual rendering, but continued investigation eventually revealed the rendering frame being dropped (and the screenshot function was likely forcing rendering and 'correcting' the broken state).
Failures will also be screenshotted for local tests and saved in `test/webdriverio/test/failures/` with a filename corresponding to the test's name. These are automatically ignored from being added to Git via `.gitignore`.
Finally, a couple other miscellaneous changes:
- One additional pause was added to the scroll test. This probably isn't needed, but more pauses generally help with stability.
- Ditto for `sendKeyAndWait` though this was more of a contract correction: if `PAUSE_TIME` was zero then it wasn't actually waiting anymore which seemed incorrect. That's been fixed.
- There are some new string casts that seem necessary after some NPM upgrades, and they're generally innocuous.
And one final note: this PR mainly ensures all tests generally pass, not that there are no flakes. From observation tests may still flake on occasion, but they can be restarted and generally expected to pass. The OS X tests ran 14 times in a row without a failure before running into the 6 hour GitHub Actions runtime limit (it seems they slowed down each subsequent run, perhaps due to some sort of memory or resource leak). It may be possible to add a "try 3 times" approach to the WebdriverIO tests in CI in the future to significantly reduce flakes and increase the trustworthiness of failures to be actual problems, but this PR does not address that.
0 commit comments