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

chore: automatically detect the dev server #29176

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

pavelfeldman
Copy link
Member

No description provided.

@pavelfeldman pavelfeldman merged commit 6a14b1d into microsoft:main Jan 25, 2024
27 of 29 checks passed
Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [installation tests] › playwright-component-testing.spec.ts:21:5 › pnpm: @playwright/experimental-ct-react should work

2 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

21360 passed, 581 skipped
✔️✔️✔️

Merge workflow run.

@Pharb
Copy link

Pharb commented Feb 28, 2024

Sadly the switch from GET -> HEAD can be a breaking change (i.e. not all servers support HEAD requests).

A note in the release notes would have been nice, or an automatic fallback to GET in case of pw:webserver HTTP Status: 405.

@CreativeTechGuy
Copy link

@Pharb you just beat me to it. 😛 This took a while for us to figure out why our tests were failing to start and it wasn't until I added logs for all requests on our server that I realized this stealthily changed from GET to HEAD.

It was definitely breaking for us. We use a plain NodeJS server in development which we added a API route which only responds to GET requests specifically to use in Playwright to check if it's online. It's an easy fix to let it respond to HEAD requests too, but would have been really nice to know this was needed.

@JakeHedman
Copy link

Same, this broke all our tests since we rely on the GET requests to check if our test data is ready etc. Adding special HEAD handlers in our webserver just for tests doesn't make much sense to me.

If it's super important to change this (and without a config option), should this not have been a major release since it's a breaking change?

@Pharb
Copy link

Pharb commented Feb 29, 2024

I opened #29732 to track it as a regression in v1.42.0

@JakeHedman

This comment was marked as outdated.

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.

5 participants