Implement automatic test suite skipping facility into the browser test harness #25533
Conversation
There was a problem hiding this comment.
Very nice!
I do worry a little bit about any mechanism that automatically skips test though.
Having to spell out EMTEST_LACKS_GRAPHICS_HARDWARE explicitly in the CI configuration means that the we don't need to trust the auto-disable feature.
This is why we have EMTEST_SKIP_NODE_CANARY explicitly rather than trying to automatically figure out at runtime if we should skip them.
I think I've seen cases where we were mistakenly always skipping certain tests due to a bug in the auto-skip magic, and its went unnoticed.
Imagine this scenario where we have an auto-skip mechanism:
1.I setup a CI bot with idea that it will run all the tests under node.
2. I install node on the bot
3. I run the tests and see that they all pass
4. Someone adds some new tests that depend on a more recent version of node.
5. An auto-skip mechanism means I never actually run them and bot keeps passing
I would much rather have my the bot start failing at (4) so can explictly choose to either:
- Update node.
- Set some environment variable such as
SKIP_NODE24orSKIP_MEMORY64to opt out explicitly.
Another example is that we don't automatically skip d8 tests when d8 is not found. We expect folks to explicitly opt out with EMTEST_SKIP_V8.
Maybe your are not proposing that we go that far, but JIC I thought I would mention the rationale for the status quo.
|
There are multiple use cases that motivate this work:
Then if the user goes and points Our
And when you do all that work, there is currently no model to contribute that information back to the project, so that others running the harness could benefit from out of the box. Other people running the suite will need to independently do that same work. Until now by this PR, that is.
Yeah, I agree this kind of accident could happen. Here I would stress that the fact that all existing functionality will still be green, i.e. the Emscripten project will not have been regressed (at least as far as test coverage is concerned), since if there is a new feature that requires newer Node, then all the tests that are running against older Node will still be ok, verifying that the project runs successfully on the older Node. So nothing has regressed(?) - maybe just the newly added test coverage is not being incorporated correctly. It is definitely a burden on the project that it also needs to keep up with new Node versions. But the risk of missing a new feature long-term by "auto-magic" seems low.. it is not like the Emscripten project could be oblivious to the latest feature developments in its domain (like Wasm Exnref exceptions), since the Emscripten devs are also contributing to the development of those feature specifications. Maybe we can add a disable feature to these auto-detection flags. For example:
That way Emscripten CI can opt-out from the auto-disables and be able to run all the tests? |
|
I updated the PR to enable setting |
|
How about is the versions checking this was all behind Specifically I think I would want to turn off Funnily enough we just had an example of where skipping tests caused us to loose coverage. In this case were explictly skipping using |
Having a single Only for this specific use case of CircleCI, we need to set up to disable the autoskip, so maybe we can set the circleci config.yml files to have a |
Is it actually more friendly though? Imagine I am a new contributor and run all the tests, and they all appeared to pass. Then, later, in review a bunch of tests fail because for example the v8 we automatically skipped because I didn't install v8. The test framework is basically saying "I noticed you don't have v8 installed, so I conveniently skipped all the v8 test for you". I would much rather the test framework say something like: "I noticed you don't have v8 installed so all tests will fail, please either install v8 or set Having to explicitly set |
I could see I suppose the autoskipping based on particular browser versions might be different...? |
I think it is more friendly. It is better for first-time users to get out of the box, instead of that has no hint towards the browser version being the problem (unless we start regex-matching individual error messages or bw-compat checking all features before using them) The above example was even simplified to a nice printed version, only running the growable arraybuffer tests. In the real world it is a worse result: anyone running
This does not seem like a problem at all? When the tests fail on CI, they will then get to read which tests failed, and they will likely try to run those tests locally, and the local test run then gives them helpful info: and then they can have that moment "oh, I need to test my feature against browser/node Y instead. Let me install that." There is hand-holding information all the way to take the developer forward. Not also that the set of developers who just report bugs or only use Emscripten is larger than the set of people who actually propose PRs to Emscripten. So |
|
OK, I see that a nice error would be better by default. How about we do the same thing we do for node canary and error with something like" "ERROR: Test requires resizeable array buffer but your browser does not support this. Run with EMTEST_AUTOSKIP=1 to skip this test automatically". That matches the existing behaviour for things like node canary, has a nice message, but is also not skipping things by default, so hopefully can make everyone happy. If folks complain about having to set EMTEST_AUTOSKIP all the time we can consider changing the default then. |
|
What I want to avoid here is folks getting the false impression that "the entire browser test suite passes" becuase they don't see any failures, when in fact a large chunk of tests were in fact auto-skipped. |
Sure. Though here I realize that the current |
We can probably make our own helper/wrapper like |
…t harness, so that users do not need to manually maintain skip lists of features. Paves the way toward test harness working for users out of the box.
…MTEST_LACKS_x=0 can be used to force-don't-skip tests.
juj
force-pushed
the
test_harness_feature_skip_checks
branch
from
5a1ab53 to
1c5aa15
Compare
|
Any more review here? |
…skip_checks # Conflicts: # test/test_browser.py
| 'firefox': 145, | ||
| 'safari': UNSUPPORTED, | ||
| 'node': 240000, | ||
| }, |
There was a problem hiding this comment.
I still feel a little uncomfortable adding stuff here that is only ever used in test code.
For years I've been trying to keep test code and production/compiler code separate and this re-entangles a little.
However, since this is basically just data is doesn't feel quite as bad.
Could you maybe add comments to easy of these new features saying "Emscripten itself doesn't use this feature but we use it in our browser tests".
There was a problem hiding this comment.
FWIW, this feature could potentially be leveraged by the experimental -sGROWABLE_ARRAYBUFFERS setting. IIUC, it only requires adding:
if settings.GROWABLE_ARRAYBUFFERS:
enable_feature(Feature.GROWABLE_ARRAYBUFFERS, 'GrowableSharedArrayBuffer')inside apply_min_browser_versions() below (untested).
There was a problem hiding this comment.
That is a good idea. I think in general for other features as well (e.g. WebGPU), we could evolve to integrate with the feature checking.
Though that's good to improve in a separate PR.
There was a problem hiding this comment.
True, although webgpu support has moved out-of-tree now.
test/test_browser.py
Outdated
| requires_offscreen_canvas = unittest.skipIf(os.getenv('EMTEST_LACKS_OFFSCREEN_CANVAS'), 'This test requires a browser with OffscreenCanvas') | ||
| requires_es6_workers = unittest.skipIf(os.getenv('EMTEST_LACKS_ES6_WORKERS'), 'This test requires a browser with ES6 Module Workers support') | ||
| requires_growable_arraybuffers = unittest.skipIf(os.getenv('EMTEST_LACKS_GROWABLE_ARRAYBUFFERS'), 'This test requires a browser that supports growable ArrayBuffers') | ||
| requires_offscreen_canvas = unittest.skipIf(test_browser_should_skip_feature('EMTEST_LACKS_OFFSCREEN_CANVAS', Feature.OFFSCREENCANVAS_SUPPORT), 'This test requires a browser with OffscreenCanvas') |
There was a problem hiding this comment.
Can you make a single helper function like skipExecIf instead of repeating the unittest.skipIf(test_browser_should_skip_feature thing?
Maybe make_require_browser_feature? Or skipIfBrowserFeatureMissing?
There was a problem hiding this comment.
Updated the custom helper to perform this kind of testing. This is now good for a second review.
test/test_browser.py
Outdated
| exe_path = shlex.split(common.EMTEST_BROWSER)[0] | ||
| ini_path = os.path.join(os.path.dirname(exe_path), "platform.ini") | ||
| # Extract the first numeric part before any dot (e.g. "Milestone=102.15.1" → 102) | ||
| m = re.search(r"^Milestone=([^\n\r]+)", open(ini_path).read(), re.MULTILINE) |
There was a problem hiding this comment.
I think you can just use do r^Milestone=(.+)$' here can't you? The dollar sign will match the EOL
There was a problem hiding this comment.
Updated, I don't have a preference if one format is better or the other.
3 participants
Implement automatic test suite skipping facility into the browser test harness, so that users do not need to manually maintain skip lists of features. (or in present state, have to maintain fewer of them).
Paves the way toward test harness working for users out of the box, improving First-Time User Experience, rather than have a large bulk of tests fail for users by default.