Ensure that waitForResponse and similar methods are awaited after triggering condition

[unlikelyzero]

In order to make tests less flaky, any call to page.waitForResponse should be wrapped in a promise.all of the action which invoked the network response.

More here: microsoft/playwright#5470 (comment)

[mxschmitt]

Just some thoughts:

// Good:
const [response] = await Promise.all([
  page.locator("button").click(),
  page.waitForResponse("https://example.com/api/search")  
]);

// Good
const mySearchPromise = page.waitForResponse("https://example.com/api/search")
await page.locator("button").click()
await mySearchPromise;

// Bad

await page.waitForResponse("https://example.com/api/search")

[thomas-phillips-nz]

Yeah we use the create promise -> perform action -> await promise pattern a lot in our codebase.

[mskelton]

@unlikelyzero Interestingly enough, the Playwright docs don't even use Promise.all

const responsePromise = page.waitForResponse('https://example.com/resource');
await page.getByText('trigger response').click();
const response = await responsePromise;

@mxschmitt While outside the scope of this issue, seems like a good change to make to the docs to use Promise.all in the Playwright docs.

My main concern around this rule is doing it in a way that doesn't produce false positives/negatives.

const responsePromise = page.waitForResponse('https://example.com/resource');
await doStuffThatFiresResponse() // Is this valid or invalid?
await responsePromise

const responsePromise = page.waitForResponse('https://example.com/resource');
await myPageObject.doStuff() // Is this valid or invalid?
await responsePromise

await Promise.all([
  page.waitForResponse('https://example.com/resource'),
  doStuff() // How do we know that do stuff actually is firing the request?
])

I suppose the way this could be implemented would be to ensure that either:

1. The waitForResponse (and other similar methods) are in a Promise.all
2. The waitForReponse is not awaited until after another awaited method, regardless of what the other method does.

If we did that, we might not catch everything that is actually an error, for example:

const responsePromise = page.waitForResponse('https://example.com/resource');
const isVisible = await page.locator('.foo').isVisible()
await responsePromise
await page.locator('.foo').click() // This fires the request

Would not be marked as an error, even though it is invalid. That's a super contrived example and unlikely to happen in the real world, so the value of catching the common mistakes seems like it could be implemented in a way that catches 95% of all mistakes.

[unlikelyzero]

Would not be marked as an error, even though it is invalid. That's a super contrived example and unlikely to happen in the real world, so the value of catching the common mistakes seems like it could be implemented in a way that catches 95% of all mistakes.

This seems like a reasonable tradeoff

[mxschmitt]

This was an intentional change, see here for our reasoning: microsoft/playwright#19154