test(webview): add a timeout for clearCookies in teardown so a busy page can't stall it#41416
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // The shared Mobile Safari cookie store persists across tests; clear it | ||
| // while still on the test's domain (webview cookies are domain-scoped). | ||
| await page.context().clearCookies().catch(() => {}); | ||
| await Promise.race([ |
There was a problem hiding this comment.
Sounds like the fix should be in clearCookies()
9392db6 to
1f976f6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| cookieName: c.name, | ||
| url: `${c.secure ? 'https' : 'http'}://${c.domain.replace(/^\./, '')}${c.path}`, | ||
| }))); | ||
| await raceAgainstDeadline(async () => { |
There was a problem hiding this comment.
This is still unexpected - it looks like page.deleteCookies can stall when user calls it?
There was a problem hiding this comment.
raceAgainstDeadline does a Promise.race with a setTimeout so this should never take more than 5s
There was a problem hiding this comment.
also, WVPage.prototype.deleteCookies is not exposed to clients
the only thing clients can invoke is BrowserContext.prototype.clearCookies, which eventually routes to here
if the concern is about somewhere else in playwright using WVPage.prototype.deleteCookies (or WVPage.prototype.getCookies) then yeah i can move the raceAgainstDeadline to be there instead
There was a problem hiding this comment.
Ah, we should rename this page to wvPage to make it clear it is not the one from the page.
There was a problem hiding this comment.
Look for "await progress.race(this._context.clearCookies({" and instead of racing externally, plumb the progress as the first argument into the call chain, all the way to WVPage.prototype.deleteCookies, where you will race the protocol call with the progress.
There was a problem hiding this comment.
digging into this a bit more, we actually already have a progress.race(...) inside browserContextDispatcher.clearCookies, which should handle all browsers and platforms, so i dont think we need/want another one here just for iOS
circling back, this fix was just about trying to solve some test timeouts that happened as a result of us attempting to ensure cookies are cleared at the end of each test run
as such, it should only ever affect developers if their test involves infinitely busying the page such that no inspector protocol messages can be received (which is also what one of our tests does)
the only reason we're having issues is because we're using a lower-level clearCookies than what all other developers could use
im happy to move the progress.race(...) to a lower level if you feel that's preferred, but it would be a different approach to almost all other commands
There was a problem hiding this comment.
I'm lost as to what we are doing here then. Will failing to delete cookies result in error? Should it respect a given timeout that comes with the call?
There was a problem hiding this comment.
the original fix i had in this PR modified the test harness to only wait for this lower-level clearCookies for 5s in the teardown after a test run finishes (i.e. there's no timeout)
ultimately i think the question we need to answer is "should it be considered a test failure if the page is left in a state that we can no longer communicate with it after the test is over?"
if yes then we should close the PR as that's the intended behavior
if no then we could probably use any previously set timeout as a limit on the lower-level clearCookies so that we give it as much time as possible to work while ensuring that it doesn't exceed the overall time limit
There was a problem hiding this comment.
If we can no longer communicate with the page, this test has failed and we should shut down the worker altogether.
1f976f6 to
2833926
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // The shared Mobile Safari cookie store persists across tests; clear it | ||
| // while still on the test's domain (webview cookies are domain-scoped). | ||
| await page.context().clearCookies().catch(() => {}); | ||
| // The shared Mobile Safari cookie store persists across tests; clear it while |
There was a problem hiding this comment.
I am not sure I follow: this does not clear any tests, does not do proper cleanup for all the visited origins either. If there is a clean way to implement it (reset for reuse-alike), let's do it. Otherwise, let's not add this code.
There was a problem hiding this comment.
unless something has otherwise changed, tests/page/page-set-content.spec.ts:153:3 › should handle timeout properly is a frequent timeout failure when running tests
so yes this does not enable any previously skipped tests, but it does fix a (previously) known flaky test
There was a problem hiding this comment.
I'd rather say it masks an issue. Why not a proper fix?
There was a problem hiding this comment.
that's definitely a fair way of looking at it
something i think is worth mentioning tho is that tests/page/page-set-content.spec.ts:153:3 › should handle timeout properly DELIBERATELY causes the page to hang with the goal of verifying that the client API timeout mechanism works correctly
to make matters worse, because we're trying to share a single iOS simulator for all of this, that busy page now has no way to recover
ill try looking into detecting when the page is hung and tearing down MobileSafari and/or the iOS Simulator in that case
… page can't stall it `tests/page/page-set-content.spec.ts:153:3 › should handle timeout properly` will infinitely loop the page as a result, none of the WebKit inspector protocol messages are handled `context.clearCookies` performs multiple round-trips using the WebKit inspector protocol (`Page.getCookies` and then `Page.deleteCookie` for each) as such, give the page fixture a `timeout` so that if `clearCookies` hangs then the worker will be killed and restarted instead of waiting forever
2833926 to
6caf0c0
Compare
Test results for "MCP"6 failed 7455 passed, 1132 skipped Merge workflow run. |
Test results for "tests 1"2 failed 1 flaky49170 passed, 1163 skipped Merge workflow run. |
tests/page/page-set-content.spec.ts:153:3 › should handle timeout properlywill infinitely loop the pageas a result, none of the WebKit inspector protocol messages are handled
context.clearCookiesperforms multiple round-trips using the WebKit inspector protocol (Page.getCookiesand thenPage.deleteCookiefor each)as such, give the page fixture a
timeoutso that ifclearCookieshangs then the worker will be killed and restarted instead of waiting forever