-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(ui): test that UI works behind proxy #33767
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets try to replace it first to see if its feasible?
@@ -16,6 +16,9 @@ | |||
|
|||
import { createImage } from './playwright-test-fixtures'; | |||
import { test, expect, retries } from './ui-mode-fixtures'; | |||
import http from 'node:http'; | |||
import httpProxy from 'http-proxy'; | |||
import { ManualPromise } from 'packages/playwright-core/lib/utils/manualPromise'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use relative imports I think, packages/playwright works by accident here I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a remnant, didn't even use it 😅
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 37178 passed, 650 skipped Merge workflow run. |
We discussed offline that we'd prefer not to have two proxy libraries within the project. But since This PR is superceded by #33771, which instead extends |
This PR adds a test to ensure we support running UI behind a proxy. It adds the
http-proxy
library to test this. As discussed, we need that for websocket support. I'll try replacing the existingproxy
library with that in a separate PR.Closes #33705.