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

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

Comments

@agnivade
Copy link
Contributor

@agnivade agnivade commented May 28, 2019

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

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented May 28, 2019

What do you propose?

@agnivade

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented May 28, 2019

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

This comment has been minimized.

Copy link
Member

@johanbrandhorst johanbrandhorst commented May 28, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented May 28, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 28, 2019

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

@gopherbot gopherbot closed this in 220552f May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.