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

all: test Examples on wasm? #25933

Closed
bradfitz opened this issue Jun 17, 2018 · 7 comments
Closed

all: test Examples on wasm? #25933

bradfitz opened this issue Jun 17, 2018 · 7 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@bradfitz
Copy link
Contributor

In #25913 we disabled testing of Example for js/wasm because it required os.Pipe, which js/wasm doesn't implement.

But disabling all those tests is slightly overkill.

This bug tracks restoring as many of those tests as possible.

/cc @neelance

@bradfitz bradfitz added Testing An issue that has been verified to require only test changes, not just a test failure. help wanted NeedsFix The path to resolution is known, but the work has not been done. arch-wasm WebAssembly issues labels Jun 17, 2018
@bradfitz bradfitz added this to the Unplanned milestone Jun 17, 2018
@neelance
Copy link
Member

Do you have any initial idea on how we could do this?

@bradfitz
Copy link
Contributor Author

Options:

  • modify the testing package in js/wasm mode to use something besides os.Pipe
  • decide to implement some fd mapping anyway, enough for os.Pipe to work, at least in testing mode, even if it's not available and/or compiled in regular mode
  • implement os.Pipe in the node.js host (so it'll be available for tests, but not in browsers)

@flimzy
Copy link
Contributor

flimzy commented Jul 9, 2018

For reference, I added this support to GopherJS some time ago (See gopherjs/gopherjs#590), by having the tests write to the filesystem, rather than to a pipe, so basically an implementation of @bradfitz's first suggestion.

@empijei
Copy link
Contributor

empijei commented Sep 4, 2018

I read the code that runs examples, and it just treats the two parts of the os.Pipe as ReadCloser and WriteCloser, and it doesn't leverage the fact that those are actual files.

The problem here is that it then replaces os.Stdout, and that must be an *os.File.

I think we could provide a dummy implementation of os.Pipe for wasm that creates two *os.File by just wrapping a call to io.Pipe() and returning a couple of partially functioning files.

Any invocation to methods that are not Write, Read or Close would just return an error and zero values, but that would still make most (if not all) examples work.

I did a quick grep around sources looking for examples that might break but it looks like this might work.

If this makes any sense I could try it out locally to see how many examples leverage the fact that os.Stdout is indeed an *os.File and report back or submit a CL if nothing breaks.

@gopherbot
Copy link

Change https://golang.org/cl/165097 mentions this issue: testing: use temporary file instead of pipe for capturing example output

@odeke-em
Copy link
Member

Alright, so since we've merged CL https://go-review.googlesource.com/c/go/+/165357 which uses os.File for js/wasm, I think the party can now start, with enabling example tests to run on js/wasm.

@gopherbot
Copy link

Change https://golang.org/cl/167800 mentions this issue: cmd/dist: reenable testing of Example for js/wasm

@golang golang locked and limited conversation to collaborators Mar 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly issues FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

6 participants