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

Allow overriding headless test URL #3861

Merged
merged 5 commits into from
Mar 2, 2024
Merged

Conversation

lynn
Copy link
Contributor

@lynn lynn commented Feb 29, 2024

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 of http://localhost:8000.

This PR would allow me to do the same for headless tests. I imagine it's also useful in combination with #3314.

Copy link
Collaborator

@daxpedda daxpedda left a 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.

@lynn
Copy link
Contributor Author

lynn commented Mar 1, 2024

I've made the requested changes. However, it feels a bit wrong to me to reuse that variable. It seems like WASM_BINDGEN_TEST_ADDRESS is an address specifying where to bind the test server, and this variable is a URL specifying how to reach it. In Rust terms, one is a SocketAddr and the other is a Url. I admit this distinction may not matter so much, though.

@lynn lynn requested a review from daxpedda March 1, 2024 14:58
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment on lines +124 to +125
let url =
std::env::var("WASM_BINDGEN_TEST_ADDRESS").unwrap_or_else(|_| format!("http://{}", server));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

@daxpedda daxpedda merged commit 0095fa7 into rustwasm:main Mar 2, 2024
25 checks passed
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.

None yet

2 participants