Fix running stability jobs for wdspec in Firefox #41243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
added
infra
wptrunner
Ideally we'd rename this everywhere, but in the meantime, avoid inventing new names/patterns for specific functions.
This is a small optimisation for cases where paths are passed in.
jgraham
force-pushed
the
firefox_stability_fix
branch
from
2b633c1
to
42ca367
Compare
whimboo
reviewed
Jul 31, 2023
| executor_kwargs["webdriver_binary"] = kwargs.get("webdriver_binary") | ||
| executor_kwargs["webdriver_args"] = kwargs.get("webdriver_args") | ||
| executor_kwargs["binary"] = kwargs["binary"] | ||
| executor_kwargs["binary_args"] = kwargs["binary_args"][:] |
There was a problem hiding this comment.
So what's the benefit for using [:] instead of just .copy()? The latter seems to be in use way more often in this repository.
There was a problem hiding this comment.
Mostly that .copy() on lists is only 3.3+ so I didn't know about it (.copy() on dictionaries is much older). I've changed it now.
Otherwise if the value is mutated later we'll end up also mutating the original value, and this can cause problems when rerunning multiple times. In particular this fixes a bug with the Firefox stability job where we were duplicating the binary agument to geckodriver and therefore only the first iteration completed successfully.
jgraham
force-pushed
the
firefox_stability_fix
branch
from
42ca367
to
528ad0f
Compare
wpt-pr-bot
requested review from
AutomatedTester,
jrandolf,
juliandescottes,
sadym-chromium and
shs96c
jgraham
force-pushed
the
firefox_stability_fix
branch
from
528ad0f
to
702c295
Compare
whimboo
approved these changes
Aug 1, 2023
There was a problem hiding this comment.
Here is a check as run in GitHub actions and it looks all fine to me. Good to see that the stability job is finally passing again. Thanks a lot @jgraham.
Note: Please revert the test change before merging.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These were failing because we were passing duplicate
--binaryarguments to geckodriver. That occurred because we were mutating a reference to a singlewebdriver_argslist on each iteration.