[test] Re-implement the @all_engines decorator based on parameteraization. NFC #25115
Conversation
sbc100
force-pushed
the
all_engines_split_tests
branch
from
76619d3 to
69abd2c
Compare
| suffix += 1 | ||
| engine_mapping[name] = (engine,) | ||
|
|
||
| parameterize(metafunc, engine_mapping) |
There was a problem hiding this comment.
Does this not run the entire test in each engine, while before we built once and just executed the JS once per engine?
There was a problem hiding this comment.
Yes, it does work that way. This means that there will be slight slowdown for folks with multiple engines configured.
However:
- Configuring multiple engines is optional, and rare. None of CI setups do this.
- There are very few tests using this decorator (currently just 1 test on main, updated to 6 tests with [test] Use
all_enginesmore consistently. NFC #25112).
Given upsides (separate names failing tests, avoiding inner loops in tests that make debugging hard), I think its worth it in this case.
There was a problem hiding this comment.
Wait, none of our CI setups test multiple engines? Don't we set up V8 on circleCI?
There was a problem hiding this comment.
We have CI tests that configure different engines, but never more than one engine at the same time.
@all_engines is a no-op unless you have multiple engines configured at the same time.
There was a problem hiding this comment.
Actually this might not be totally true, I think for v8 we do configure it to be in addition to node (i.e. we append it to JS_ENGINES) but for jsc and spidermonkey testing we always just do JS_ENGINES = [spidermonkey] or JS_ENGINES = [jsc].
There was a problem hiding this comment.
But still, given how few tests run with all_engines I think this should be fine.
Also, of the 7 total tests that are currently marked with all_engines we can/should probably remove some of those annotations.
There was a problem hiding this comment.
Fair enough, I guess it's worth the debuggability.
…tion. NFC This means that we can remove the loop over JS_ENGINES from few more tests. It also splits up these tests in to smaller units which is always good and means that it much more obvious what is going on when one of them fails.
sbc100
force-pushed
the
all_engines_split_tests
branch
from
69abd2c to
14c37ff
Compare
|
Oops this auto-merged even though there were failing tests. Reverting for now and so I can re-land once I debug. |
This means that we can remove the loop over JS_ENGINES from few more
tests.
It also splits up these tests in to smaller units which is always good
and means that it much more obvious what is going on when one of them
fails.