fix(plugins): follow redirects when checking the webServer url

[ghost91-]

Fixes #22144

This change implements following redirects when a 3xx HTTP status code is received, when probing the url of a webServer config.

Without this change (i.e., currently), playwright still tries to follow the redirect, but often fails, since it incorrectly uses the received Location header as new URL, instead of combining it with the URL from the previous request.

For example, if the URL is http://localhost:8080/some-path and a redirect with Location: /some-other-path is received, playwright incorrectly tries to query http://localhost:80/some-other-path, which of course fails, due to the incorrect port.

An alternative solution would be to not follow redirects at all when probing the URL, and just consider a 3xx status code as "server is available". This is actually what the documentation claims will happen right now, but it's incorrect. I chose this solution over not following the redirect, since it makes sense, in my opinion, and not following the redirect would require more code changes (httpRequest would need some kind of option that prevents it from following redirects). But if that solution would be preferred, I am happy to change the implementation.

[mxschmitt]

Suggested change

	command: 'node ${JSON.stringify(SIMPLE_SERVER_PATH)} ${server.PORT}',
	command: 'exit 1',

I think we can do this to make clear that we should never run the command, since reuseExistingServer is true.

[ghost91-]

This could also be done for some of the other tests, e.g., should send Accept header or should be able to use an existing server when reuseExistingServer:true. Should I also add it for those tests? If so, should that be done in the same commit or in a separate one?

[pavelfeldman]

I like the simple formula that we currently document:

The url on your http server that is expected to return a
2xx, 3xx, 400, 401, 402, or 403 status code when the
server is ready to accept connections.

Does that not work? We can prevent probe from following the redirects if current behavior does not much the doc above.

[ghost91-]

Yeah, I mentioned that in the PR description: At the moment, the docs don’t match reality: redirects are attempted to be followed, but it’s buggy. I am fine with both not following redirects, or fixing redirects. But not following redirects is more work, because the underlying utility (httpRequest) does not support that yet. And I wanted to keep things as simple as possible. But if that’s the preferred solution, I’m also Ok with that. I’d probably need some guidance on how to add that „don’t follow redirects“ option to httpRequest.

[pavelfeldman]

https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/utils/network.ts#L78 we are using http.request for that, which should not follow redirects. Could you add a failing tests that surfaces the bug?

[ghost91-]

http.request may not follow redirects, but there is dedicated code in httpRequest to do so: https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/utils/network.ts#L73

But this code is broken, which is the whole point of this PR. At the moment it fixes the broken redirect implementation. But I’m also happy to remove the redirect behavior, if that’s preferred.

Regarding a test case that fails, here is the simplest one I can think of:

test('should not follow redirects but accept them as "server is available"', async ({ runInlineTest, server }) => {
  server.setRedirect('/redirect', '/redirected-to');
  const result = await runInlineTest({
    'test.spec.ts': `
      import { test, expect } from '@playwright/test';
      test('dummy test', async () => {
        expect(true).toBe(true);
      });
    `,
    'playwright.config.ts': `
      module.exports = {
        webServer: {
          command: 'exit 1',
          url: 'http://localhost:${server.PORT}/redirect',
          reuseExistingServer: true,
        }
      };
    `,
  });
  expect(result.exitCode).toBe(0);
});

[mxschmitt]

Suggested change

	httpRequest({ ...params, url: new URL.URL(res.headers.location, params.url).toString() }, onResponse, onError);
	httpRequest({ ...params, url: new URL(res.headers.location, params.url).toString() }, onResponse, onError);

change this to that and import * as URL from 'url'; to import * as url from 'url';.

Clarified offline with Pavel that we want to continue to follow redirects.

[mxschmitt]

actually its fine, we used to do this before. Merging in since require('url').URL === URL.