-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Don't set shared.Settings from test runners #6801
Don't set shared.Settings from test runners #6801
Conversation
I ❤️ this change so much. It also makes me wonder why some cases use |
I don't feel strongly either way, but another option here might be to just do the diff when serializing - that is, don't emit |
That was my first approach that I tried a few weeks back. I kept running into snags for reasons I don't fully remember (at least one of which probably stopped being true in #6793 now that I think about it). This solution I feel leaves the code simpler (despite being a larger diff), and also has some other nice properties. |
c448d5d
to
0a51f90
Compare
Turns out we use these overrides from the test runner to get_settings based on default environment. Meaning, running the asm2 test suite would have self.get_setting('WASM') == 1, because wasm is default.
Fixed CI, going to merge in a hour or so if there's no comments |
@jgravelle-google this appears to have broken |
Confirmed locally as well. |
Also confirmed locally. |
Fixed in #6848 |
Instead of
shared.Settings.FOO = x
, now we sayself.set_settting('FOO', x)
This avoids needing to reload the settings for each test, because we can store the modifications in
self.settings_mods
, and just clear that dict between runs.Instead of
if shared.Settings.BAR > 2
, nowself.get_setting('BAR') > 2
This is needed because we need to respect the default settings, as well as any modifications made for test purposes.
Instead of
shared.Settings.serialize()
, nowself.serialize_settings()
This is the real win of this patch: we only need to serialize the setting changes that the test actually cares about. This cleans up command-line errors tremendously.
Before:
After: