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

Jump to bottom

net/http: improve useFakeNetwork check to allow browsers to run tests in wasm #32289

Closed
agnivade opened this issue May 28, 2019 · 5 comments
Closed

net/http: improve useFakeNetwork check to allow browsers to run tests in wasm #32289

agnivade opened this issue May 28, 2019 · 5 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@agnivade
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version devel +1531192272 Mon May 27 17:58:39 2019 +0000 linux/amd64

net/http/rountrip_js.go has this code:

// useFakeNetwork is used to determine whether the request is made
// by a test and should be made to use the fake in-memory network.
func useFakeNetwork() bool {
	return len(os.Args) > 0 && strings.HasSuffix(os.Args[0], ".test")
}

This code has gone through several iterations. First it was:

host, _, err := net.SplitHostPort(req.Host)
if err != nil {
	host = req.Host
}
if ip := net.ParseIP(host); ip != nil {
	return ip.IsLoopback(ip)
}
return host == "localhost"

Then, it was changed to:

return len(os.Args) > 0 && path.Base(os.Args[0]) == "node"

And finally,

return len(os.Args) > 0 && strings.HasSuffix(os.Args[0], ".test")

The latest version will always return true if any code is invoked with go test. That prevents wasm codepath from being taken if the test is being run using a binary (by replacing the -exec flag) which spins up a browser, and loads the wasm file in it and captures the logs. Essentially a test environment for wasm code inside the browser.

I stumbled onto this while writing a tool like that. The current workaround from my end is to rename the binary to .wasm if it ends with .test. But ideally, this should be fixed in the code.

@johanbrandhorst @neelance @bradfitz
@agnivade agnivade added arch-wasm WebAssembly issues NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 28, 2019
@bradfitz
Copy link
Contributor

What do you propose?

@agnivade
Copy link
Contributor Author

Actually, I can't recall why the 2nd version was changed. The only difference between 2nd and 3rd is that from catching all node-spawned processes, now it catches all tests under js,wasm flag.

Perhaps -

len(os.Args) > 0 && path.Base(os.Args[0]) == "node" && strings.HasSuffix(os.Args[0], ".test")

sounds better ? Since it filters both node processes and tests, allowing browser tests to run.

@johanbrandhorst
Copy link
Member

The reason it was introduced was to allow the gerrit tests to pass, so if such a change would still allow the gerrit tests to pass while allowing tests to be run in the browser then I think that sounds good.

@agnivade
Copy link
Contributor Author

I meant what exactly changed in the logic from v2 to v3 to allow trybots to pass. AFAIU, v3 generalizes the logic to follow a non-wasm path for all tests. But v2 already allowed node. So how did it fail ? (assuming wasm trybots failed)

Anyways, I am afk at the moment, but I will check if my suggestion passes both node and browser tests.

@gopherbot
Copy link

Change https://golang.org/cl/179118 mentions this issue: net/http: allow WASM fetch RoundTripper in non-node envs

@golang golang locked and limited conversation to collaborators May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bradfitz @agnivade @johanbrandhorst @gopherbot