-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow overriding headless test URL #3861
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.
I don't think we need a new environment variable here, lets reuse WASM_BINDGEN_TEST_ADDRESS
.
Please add an entry in the changelog as well.
I've made the requested changes. However, it feels a bit wrong to me to reuse that variable. It seems like |
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.
Thank you!
let url = | ||
std::env::var("WASM_BINDGEN_TEST_ADDRESS").unwrap_or_else(|_| format!("http://{}", server)); |
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.
let url = | |
std::env::var("WASM_BINDGEN_TEST_ADDRESS").unwrap_or_else(|_| format!("http://{}", server)); | |
let url = format!("http://{}", std::env::var("WASM_BINDGEN_TEST_ADDRESS").unwrap_or(server)); |
I was briefly thinking that this might be a good idea, but maybe there is a use case out there for https
?
I have wasm code that uses web-sys to make cross-origin requests with cookies. To test this interactively, I navigate to
http://localhost.mydomain.org:8000
(which I've set up to resolve to 127.0.0.1) instead ofhttp://localhost:8000
.This PR would allow me to do the same for headless tests. I imagine it's also useful in combination with #3314.