Closed GrayedFox closed 1 year ago
If there is no await
before page.goto
you may well get the exception because the context and the page are closed after the test, before load event is fired. You maybe more lucky with waitUntil: 'commit'
just because 'commit' event is fired much sooner. If you omit await before page.goto this is working as intended, you should await for the action to avoid the error.
Hi there @yury-s - thanks for looking into this. Rather than await the goto I am using a custom function to await the network requests to our GraphQL backend made by the page that is navigated to, since that it what signals a "ready to be tested" state. If I await the goto, the next promise (which awaits the requests) can be too slow and miss a request. I know I can do Promise.all()
and do so when combining a Locator method (like a button click) with triggers a request, but technically don't need that here.
Below is the actual method we use at the beginning of each test - the injectCarted()
method exposes a function as well as injects an init script -- very similar to what is in the above OP -- and then we navigate using goto and immediately after await some GraphQL responses.
// open browser, inject needed Carted window object, and wait for graphQL responses
async open(waitForApi = true) {
await this.injectCarted();
this.page.goto(ClientData.baseUrl + '/index.html');
if (!waitForApi) return;
await waitForResponses(this.page, 4);
}
All of the intermittently failing tests in question wait for 4-6 graphql requests to finish - if I edit the code sample in the OP to await for 2000ms before returning a promise after navigating to the page, and the same bug occurs, would you consider that proof of an issue?
and then we navigate using goto and immediately after await some GraphQL responses.
This is racy. You start waiting when some of the requests may have been sent. You should start waiting before the navigation:
// open browser, inject needed Carted window object, and wait for graphQL responses
async open(waitForApi = true) {
await this.injectCarted();
const receivedApiResponses = waitForApi ? waitForResponses(this.page, 4) : undefined;
await this.page.goto(ClientData.baseUrl + '/index.html');
await receivedApiResponses;
}
This way you ensure that there is no unhandled promise from page.goto and that the page didn't issue any requests before you started listening for them. You can pass waitUntil: 'commit'
if you like since you have an alternative condition for the page readiness. If you don't await for page.goto, you have to catch potential error in page.goto as you don't know if the page will be closed before the navigation completes.
Alright so, if I refactor the code to capture the responses first but don't await the goto:
// open browser, inject needed Carted window object, and wait for graphQL responses
async open(waitForApi = true) {
const graphQlResponses = waitForResponses(this.page, 4);
await this.injectCarted();
this.page.goto(ClientData.baseUrl + '/index.html');
if (waitForApi) {
await graphQlResponses;
}
}
Would you consider that proof of a bug? Note I will need to test it still occurs and post another response here. Again thanks for your time/
Edit: ternary version you posted is better as it doesn't unnecessarily create a promise that goes unused - just a style choice on my end 👍🏽
Would you consider that proof of a bug?
You still have to either await this.page.goto
or this.page.goto(ClientData.baseUrl + '/index.html').catch(() => {});
if the page can close before the navigation finishes. An uncaught exception from pending page.goto
thrown during context closure is not a bug.
Okay I think I perhaps haven't explained the issue very well.
The problem is, it seems like the page/context closure is happening too soon - in particular - it is happening before tests have finished running.
The page shouldn't close before navigation finishes, because the page and context should remain open until the cleanUp call during the afterAll hook.
It looks like the goto method is throwing before any tests have been run (afaik). The bug, then, is not that goto throws due to the context or page being closed - but rather - that the page is being closed before the test run finishes (or even really begins).
I can confirm that if I don't await the goto method, even when capturing responses before making the page navigation, I get the navigation failed due to page being closed error - and even when there are other Locator methods after the initial page navigation that are awaiting promises.
Here is a full test file which exhibits the bug:
Edit: this is just the same code posted in the OP now
I get that a bunch of the implementation details are hidden - but I think this still hopefully clarifies what I think the issue is. Note that even when payment.open()
calls the exact code I posted just before (with the graphQlResponses declared before the goto) -- and proceeds to interact with the page by filling out a form and submitting it - the error will occur.
What I don't get is what, exactly, is causing the page to close before the goto method has had a chance to navigate when we are awaiting further promises after the goto? The cleanup method shouldn't be called until after all tests run and those tests shouldn't be run until the entire beforeAll hook has finished.
It seems like page goto, when not awaited, is buggy in that it somehow causes a page or context to be closed when it shouldn't be (or perhaps goes to navigate before the page is fully opened, thus returning a cryptic error making it look like the page/context has been closed, but really the page hasn't fully opened yet -- assuming closed
is the same as the initial state).
I will update the OP so the code is clearer.
It looks like the goto method is throwing before any tests have been run (afaik). The bug, then, is not that goto throws due to the context or page being closed - but rather - that the page is being closed before the test run finishes (or even really begins).
All the tests are missing await
before toHaveText
, toHaveScreenshot
, toContainText
calls. Locator assertions are asynchronous if you don't await them then the test runner may close the page before the actual assertion finishes the check. Make sure that you await
all asynchronous calls in the playwright API, otherwise you may easily get errors like you mentioned. You may want to run an audit to check that there are no hanging async calls.
Closing per the response above, feel free to open a new issue if it doesn't work.
Confirming that removing the waitUntil
option and instead awaiting each expect from inside the tests fixed it.
I recently converted the whole test suite from Cypress to PW and that was a relic I totally overlooked 🤦🏽
Thanks for helping me get to the bottom of the error, hopefully future eyeballs will benefit if they come across this!
System info
Note: OP has been edited for clarity.
Source code
implementation details
test.spec.ts
index.html
Config file
Steps
Expected
No "page goto failed: page was closed error" is thrown
Actual
Fails 33% of the time on my machine. I don't know for sure but after verifying the valid load state here it seems like eventually the private server _goToAction method will be called, which on line 693 awaits an extra
LifecycleEvent
event promise.My suspicion is that the file protocol is behaving differently (due to how the browser treats local html files) - or that it is perhaps just faster than the typical http/https file that, under normal circumstances, is served up by a server which is therefore exposing some raciness.
Mileage may vary, I believe the above code reproduces the issue, I had to take a whole lot of IP and other work related stuff out that over complicates things, but wanted to leave stuff like
dotenv
to mirror our code as close as possible.What I can confirm is that setting the wait until option to
commit
fixes the issue for us.