[webdriver] Close old windows at the end of each test as well as beginning #24879
Conversation
|
What more do we think this needs before landing? Some performance analysis? |
@jgraham - how do you feel about closing windows both at end and start of test (as this patch does), versus moving closing to end of test only? (Versus the status quo of closing at the start only). If we're ok with doing both to be as stringent as possible, I can do some performance measurements locally and using trigger runs to see if I can see any clear overhead being introduced. (Locally I can test Chrome, can use trigger runs for Edge and Safari. Firefox doesn't use this path, I believe). |
This is a test to determine whether closing old windows at the end of each test would be useful to find troublesome tests (since the current behavior results in failures in the *next* test that runs). Not for submitting in its current form.
stephenmcgruer
force-pushed
the
smcgruer/close_windows_earlier
branch
from
586ae19
to
37bb5d4
Compare
|
Using a trigger run on Chrome Dev, I can't see any clear difference: 37bb5d4 (this change) - https://community-tc.services.mozilla.com/tasks/groups/LpMYzvFhQ7Kh7x_6mjbZhQ 7bdb28b (parent commit) - https://community-tc.services.mozilla.com/tasks/groups/GkQ6NK7iQkqSeaVVC-5qxA Each of the chrome-dev jobs seem to be within a minute or two of each other. It seems like with-this change is more often the minute-slower one, but sometimes its the flip. I've also pushed it to STP trigger as well, since 7bdb28b has a STP run too (https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=54305&view=results). The trigger run is https://dev.azure.com/web-platform-tests/wpt/_build/results?buildId=54324&view=results, so we'll see in a bit if the numbers are comparable (I suspect STP might be less stable generally?) |
|
Locally I can see a 2.3% slowdown on |
|
Hmm, a 2% performance regression for a change that's nearly a noop seems pretty bad. But I guess I don't have a strong objection here; ensuring the error is associated with the correct test does seem valuable. |
The STP runs show a ~2.76% speedup, but I have no idea whether that's within the MOE or not. |
stephenmcgruer
changed the title
wpt-pr-bot
added
infra
wptrunner
… as beginning" (#25567) This is a reland of #24879 (which was reverted in #25544). It updates the code to only close the windows if the user hasn't asked us to pause after a test has run. From the original PR: ---- Previously, we closed old windows at the start of each test. This was nice in terms of defensiveness (don't assume the last run left the world in a good state), but made it hard to find problematic tests that left dialogs open (since they wouldn't throw until the next test). Instead, this patch does both - close both at the start and end of a test. This should improve the blaming situation, whilst still being defensive. There are potential performance implications to this patch, however test runs are inconclusive. Full runs of Chrome and Safari show +- 2%, which is possibly within margin of error. Running locally, some directories showed a ~2% slowdown, whilst others had little or no difference.
Previously, we closed old windows at the start of each test. This was nice in terms
of defensiveness (don't assume the last run left the world in a good state), but made
it hard to find problematic tests that left dialogs open (since they wouldn't throw until
the next test).
Instead, this patch does both - close both at the start and end of a test. This should
improve the blaming situation, whilst still being defensive.
There are potential performance implications to this patch, however test runs are
inconclusive. Full runs of Chrome and Safari show +- 2%, which is possibly within
margin of error. Running locally, some directories showed a ~2% slowdown, whilst
others had little or no difference.