Skip to content
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

cherry-pick(#29180): fix: interception id not found error in route.co… #29222

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

yury-s
Copy link
Member

@yury-s yury-s commented Jan 29, 2024

…ntinue

We stopped catching all exceptions in
#28539 in hope that we'll get loadingFailed even before Fetch.continue/fulfill command's error. Turns out this is racy and may fail if the test cancels the request while we are continuing it. The following test could in theory reproduce it if stars align and the timing is good:

it('page.continue on canceled request', async ({ page }) => {
  let resolveRoute;
  const routePromise = new Promise<Route>(f => resolveRoute = f);
  await page.route('http://test.com/x', resolveRoute);

  const evalPromise = page.evaluate(async () => {
    const abortController = new AbortController();
    (window as any).abortController = abortController;
    return fetch('http://test.com/x', { signal: abortController.signal }).catch(e => 'cancelled');
  });
  const route = await routePromise;
  void page.evaluate(() => (window as any).abortController.abort());
  await new Promise(f => setTimeout(f, 10));
  await route.continue();
  const req = await evalPromise;
  expect(req).toBe('cancelled');
});

Fixes #29123

… route.continue

We stopped catching all exceptions in
microsoft#28539 in hope that we'll
get loadingFailed even before Fetch.continue/fulfill command's error.
Turns out this is racy and may fail if the test cancels the request
while we are continuing it. The following test could in theory reproduce
it if stars align and the timing is good:

```js
it('page.continue on canceled request', async ({ page }) => {
  let resolveRoute;
  const routePromise = new Promise<Route>(f => resolveRoute = f);
  await page.route('http://test.com/x', resolveRoute);

  const evalPromise = page.evaluate(async () => {
    const abortController = new AbortController();
    (window as any).abortController = abortController;
    return fetch('http://test.com/x', { signal: abortController.signal }).catch(e => 'cancelled');
  });
  const route = await routePromise;
  void page.evaluate(() => (window as any).abortController.abort());
  await new Promise(f => setTimeout(f, 10));
  await route.continue();
  const req = await evalPromise;
  expect(req).toBe('cancelled');
});
```

Fixes microsoft#29123
Copy link
Contributor

Test results for "tests 1"

3 flaky ⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks

21287 passed, 579 skipped
✔️✔️✔️

Merge workflow run.

@yury-s yury-s merged commit 7a72adc into microsoft:release-1.41 Jan 29, 2024
29 of 30 checks passed
@yury-s yury-s deleted the cp-route-catch branch January 29, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants