Skip to content

net/http: fix build errors on js/wasm target #25714

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

Closed
wants to merge 1 commit into from
Closed

net/http: fix build errors on js/wasm target #25714

wants to merge 1 commit into from

Conversation

johanbrandhorst
Copy link
Member

The in-progress WASM port does not yet have sufficient automatic
testing performed against it, so these errors slipped through when
adding the new Fetch API backed http.Roundtripper.

Updates #25506

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Jun 4, 2018
@gopherbot
Copy link
Contributor

This PR (HEAD: e6dc6ec) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/116076 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Richard Musiol:

Patch Set 1:

This is a good start, but it does not yet fully fix the tests of net/http. The reason is that useFakeNetwork returns false, because os.Args is this:

[/var/folders/sy/qczh9gpd5f51brb923qxb6vr0000gn/T/go-build207682035/b001/http.test -test.short=true -test.run=Test -test.v=true]


Please don’t reply on this GitHub thread. Visit golang.org/cl/116076.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Johan Brandhorst:

Patch Set 1:

Patch Set 1:

This is a good start, but it does not yet fully fix the tests of net/http. The reason is that useFakeNetwork returns false, because os.Args is this:

[/var/folders/sy/qczh9gpd5f51brb923qxb6vr0000gn/T/go-build207682035/b001/http.test -test.short=true -test.run=Test -test.v=true]

Ah so Brad's suggestion of using "node" is not sufficient here. Any other suggestions on how we could turn this on for tests alone? Is "http.test" something we could rely on instead?


Please don’t reply on this GitHub thread. Visit golang.org/cl/116076.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Richard Musiol:

Patch Set 1:

Patch Set 1:

Patch Set 1:

This is a good start, but it does not yet fully fix the tests of net/http. The reason is that useFakeNetwork returns false, because os.Args is this:

[/var/folders/sy/qczh9gpd5f51brb923qxb6vr0000gn/T/go-build207682035/b001/http.test -test.short=true -test.run=Test -test.v=true]

Ah so Brad's suggestion of using "node" is not sufficient here. Any other suggestions on how we could turn this on for tests alone? Is "http.test" something we could rely on instead?

The main problem is that you need to use the fake networking not only for the tests of net/http, but also for other packages that use net/http in their tests. For those packages the binary wouldn't be "http.test".

Maybe we should use a build tag and then have the js/wasm tests use that build tag? @brad What do you think?


Please don’t reply on this GitHub thread. Visit golang.org/cl/116076.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 1: Code-Review+2

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/116076.
After addressing review feedback, remember to publish your drafts!

The in-progress WASM port does not yet have sufficient automatic
testing performed against it, so these errors slipped through when
adding the new Fetch API backed http.Roundtripper.

Updates #25506
@gopherbot
Copy link
Contributor

This PR (HEAD: 064062b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/go/+/116076 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Johan Brandhorst:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/116076.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/116076 has been merged.

@gopherbot gopherbot closed this Jun 4, 2018
jasonwbarnett pushed a commit to jasonwbarnett/fileserver that referenced this pull request Jul 11, 2018
The in-progress WASM port does not yet have sufficient automatic
testing performed against it, so these errors slipped through when
adding the new Fetch API backed http.Roundtripper.

Updates #25506

Change-Id: I84c5832452e3e6067a02d926f67d01aaca66b837
GitHub-Last-Rev: 064062b5fd256e7fce961a13a8ac00a135f60221
GitHub-Pull-Request: golang/go#25714
Reviewed-on: https://go-review.googlesource.com/116076
Reviewed-by: Brad Fitzpatrick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants