Make runnable Examples work #590

Merged
merged 13 commits into from Feb 27, 2017

Conversation

Projects
None yet
3 participants
@flimzy
Contributor

flimzy commented Feb 17, 2017

This PR works around the issues discussed in #529 by using a temp file, rather than os.Pipe, when executing examples.

@neelance

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Feb 19, 2017

Member

Looks good to me. Please rebase on latest master, then we can merge.

@shurcooL Looks good to you as well?

Member

neelance commented Feb 19, 2017

Looks good to me. Please rebase on latest master, then we can merge.

@shurcooL Looks good to you as well?

@flimzy

This comment has been minimized.

Show comment
Hide comment
@flimzy

flimzy Feb 21, 2017

Contributor

Done @neelance

Contributor

flimzy commented Feb 21, 2017

Done @neelance

@dmitshur dmitshur self-assigned this Feb 21, 2017

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 21, 2017

Member

@flimzy, those 2 commits where you changed code to avoid extra imports, was that because a test was failing when you did that? Just want to check.

Member

dmitshur commented Feb 21, 2017

@flimzy, those 2 commits where you changed code to avoid extra imports, was that because a test was failing when you did that? Just want to check.

@flimzy

This comment has been minimized.

Show comment
Hide comment
@flimzy

flimzy Feb 21, 2017

Contributor

@shurcooL Yes, exactly.

Contributor

flimzy commented Feb 21, 2017

@shurcooL Yes, exactly.

circle.yml
+ - >
+ gopherjs test --short --minify --run='^Test'
+ net/http/cookiejar
+ reflect

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

It looks like only two examples are failing:

  • ExampleNew, because it tries to perform network operations that GopherJS doesn't support (i.e., it tries to create a new HTTP server with httptest.NewServer).
  • ExampleStructOf is failing because reflect.StructOf is not supported (see #499).

In the case of net/http/cookiejar, ExampleNew is the only example it has.

However, for reflect, there are other examples. Do you know if they work?

I think it'd be nice to simply disable the failing/unsupported examples, the way we already do it for tests (e.g., see 3303ee8), instead of outright disabling all examples for the packages that contain examples that don't work. That'd be cleaner, and hopefully some examples in reflect packages do pass (and might catch regressions in the future).

@dmitshur

dmitshur Feb 21, 2017

Member

It looks like only two examples are failing:

  • ExampleNew, because it tries to perform network operations that GopherJS doesn't support (i.e., it tries to create a new HTTP server with httptest.NewServer).
  • ExampleStructOf is failing because reflect.StructOf is not supported (see #499).

In the case of net/http/cookiejar, ExampleNew is the only example it has.

However, for reflect, there are other examples. Do you know if they work?

I think it'd be nice to simply disable the failing/unsupported examples, the way we already do it for tests (e.g., see 3303ee8), instead of outright disabling all examples for the packages that contain examples that don't work. That'd be cleaner, and hopefully some examples in reflect packages do pass (and might catch regressions in the future).

This comment has been minimized.

@flimzy

flimzy Feb 21, 2017

Contributor

I believe that's the only failure in the reflect package. I was essentially being lazy about creating yet another special case to run the other examples, but I can do that.

@flimzy

flimzy Feb 21, 2017

Contributor

I believe that's the only failure in the reflect package. I was essentially being lazy about creating yet another special case to run the other examples, but I can do that.

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

Well, it would allow you to simplify circle.yml back to the original version. I think it's worth doing, if it's indeed possible. Thanks!

@dmitshur

dmitshur Feb 21, 2017

Member

Well, it would allow you to simplify circle.yml back to the original version. I think it's worth doing, if it's indeed possible. Thanks!

This comment has been minimized.

@flimzy

flimzy Feb 21, 2017

Contributor

Ah yes, that is a much better approach. I'll make that change.

@flimzy

flimzy Feb 21, 2017

Contributor

Ah yes, that is a much better approach. I'll make that change.

compiler/natives/src/testing/ioutil.go
+ if err != nil {
+ return "", err
+ }
+ io.Copy(buf, f)

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

Why not check errors from io.Copy?

How about this:

func readFile(filename string) (string, error) {
	f, err := os.Open(filename)
	if err != nil {
		return "", err
	}
	defer f.Close()
	var buf bytes.Buffer
	_, err = io.Copy(&buf, f)
	if err != nil {
		return "", err
	}
	return buf.String(), nil
}

It would also be more efficient to do a stat on the file to get its size, then allocate a buffer of exactly that size. I'm guessing you chose not to do that because most test output is pretty small, so it's not worth it. Is that the reason?

@dmitshur

dmitshur Feb 21, 2017

Member

Why not check errors from io.Copy?

How about this:

func readFile(filename string) (string, error) {
	f, err := os.Open(filename)
	if err != nil {
		return "", err
	}
	defer f.Close()
	var buf bytes.Buffer
	_, err = io.Copy(&buf, f)
	if err != nil {
		return "", err
	}
	return buf.String(), nil
}

It would also be more efficient to do a stat on the file to get its size, then allocate a buffer of exactly that size. I'm guessing you chose not to do that because most test output is pretty small, so it's not worth it. Is that the reason?

This comment has been minimized.

@flimzy

flimzy Feb 21, 2017

Contributor

What is the best way of allocating a bytes.Buffer of a specific size?

b := make([]byte,size)
buf := bytes.NewBuffer(b)

?

@flimzy

flimzy Feb 21, 2017

Contributor

What is the best way of allocating a bytes.Buffer of a specific size?

b := make([]byte,size)
buf := bytes.NewBuffer(b)

?

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

Depends on if you want a byte slice, then:

b := make([]byte, 0, capacity)
... populate/use b

Or if you want a bytes.Buffer, then:

buf := bytes.NewBuffer(make([]byte, 0, capacity))
... populate/use buf

See bytes.NewBuffer documentation:

NewBuffer creates and initializes a new Buffer using buf as its initial contents. It is intended to prepare a Buffer to read existing data. It can also be used to size the internal buffer for writing. To do that, buf should have the desired capacity but a length of zero.

@dmitshur

dmitshur Feb 21, 2017

Member

Depends on if you want a byte slice, then:

b := make([]byte, 0, capacity)
... populate/use b

Or if you want a bytes.Buffer, then:

buf := bytes.NewBuffer(make([]byte, 0, capacity))
... populate/use buf

See bytes.NewBuffer documentation:

NewBuffer creates and initializes a new Buffer using buf as its initial contents. It is intended to prepare a Buffer to read existing data. It can also be used to size the internal buffer for writing. To do that, buf should have the desired capacity but a length of zero.

compiler/natives/src/testing/example.go
- os.Exit(1)
- }
- outC <- buf.String()
- }()

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

Just trying to understand this better. Do you know why the original Go code did this here in a goroutine, instead of simply doing it at the end (original line 51, out := <-outC)?

Is it so that writes into the write-end of pipe don't block? I don't know how io.Pipe works in that sense. Do you need to read from the read-end, otherwise writing-end blocks?

Ok, reading its documentation confirms that's the case:

Pipe creates a synchronous in-memory pipe. It can be used to connect code expecting an io.Reader with code expecting an io.Writer.

Reads and Writes on the pipe are matched one to one except when multiple Reads are needed to consume a single Write. That is, each Write to the PipeWriter blocks until it has satisfied one or more Reads from the PipeReader that fully consume the written data. The data is copied directly from the Write to the corresponding Read (or Reads); there is no internal buffering.

@dmitshur

dmitshur Feb 21, 2017

Member

Just trying to understand this better. Do you know why the original Go code did this here in a goroutine, instead of simply doing it at the end (original line 51, out := <-outC)?

Is it so that writes into the write-end of pipe don't block? I don't know how io.Pipe works in that sense. Do you need to read from the read-end, otherwise writing-end blocks?

Ok, reading its documentation confirms that's the case:

Pipe creates a synchronous in-memory pipe. It can be used to connect code expecting an io.Reader with code expecting an io.Writer.

Reads and Writes on the pipe are matched one to one except when multiple Reads are needed to consume a single Write. That is, each Write to the PipeWriter blocks until it has satisfied one or more Reads from the PipeReader that fully consume the written data. The data is copied directly from the Write to the corresponding Read (or Reads); there is no internal buffering.

circle.yml
+ gopherjs test --short --minify --run='^Test'
+ net/http/cookiejar
+ reflect
+ - gopherjs test --short --minify --run='^Example(MakeFunc|StructTag|StructTag.Lookup|TypeOf)'

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

This isn't the approach I meant. Is it possible to override and skip examples similar to how we skip failing/unsupported tests?

Look at 3303ee8 for example.

There's no t.Skip for examples, but just leaving the func body blank would probably work, right?

My goal is to simplify the command to run tests to just gopherjs test --short --minify ... instead of adding more special cases. That way, it's easier to run it locally, not just in CI.

@dmitshur

dmitshur Feb 21, 2017

Member

This isn't the approach I meant. Is it possible to override and skip examples similar to how we skip failing/unsupported tests?

Look at 3303ee8 for example.

There's no t.Skip for examples, but just leaving the func body blank would probably work, right?

My goal is to simplify the command to run tests to just gopherjs test --short --minify ... instead of adding more special cases. That way, it's easier to run it locally, not just in CI.

This comment has been minimized.

@flimzy

flimzy Feb 21, 2017

Contributor

I realized after I committed the change. Stand by for an update.

@flimzy

flimzy Feb 21, 2017

Contributor

I realized after I committed the change. Stand by for an update.

@dmitshur

dmitshur requested changes Feb 21, 2017 edited

This PR works around the issues discussed in #529 by using a temp file, rather than os.Pipe, when executing examples.

Thanks for working on this @flimzy! This is a cool workaround to the problem preventing some examples to run due to #529.

See this comment and the 2 I posted previously. Otherwise this looks good to me.

compiler/natives/src/testing/example.go
+ fmt.Fprintf(os.Stderr, "testing: reading stdout file: %v\n", e)
+ os.Exit(1)
+ }
+ _ = os.Remove(w.Name())

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

If there's any error during reading the example stdout file, _ = os.Remove(w.Name()) line will never run.

Would the following be better?

out, readFileError := ioutil.ReadFile(w.Name())
os.Remove(w.Name())
if readFileError != nil {
	fmt.Fprintf(os.Stderr, "testing: reading stdout file: %v\n", readFileError)
	os.Exit(1)
}
@dmitshur

dmitshur Feb 21, 2017

Member

If there's any error during reading the example stdout file, _ = os.Remove(w.Name()) line will never run.

Would the following be better?

out, readFileError := ioutil.ReadFile(w.Name())
os.Remove(w.Name())
if readFileError != nil {
	fmt.Fprintf(os.Stderr, "testing: reading stdout file: %v\n", readFileError)
	os.Exit(1)
}
+
+package cookiejar
+
+func ExampleNew() {}

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

It's very helpful (for later) to put a comment inside that explains why a test/example is disabled:

func ExampleNew() {
    // Example relies on httptest.NewServer, which GopherJS does not implement.
}
@dmitshur

dmitshur Feb 21, 2017

Member

It's very helpful (for later) to put a comment inside that explains why a test/example is disabled:

func ExampleNew() {
    // Example relies on httptest.NewServer, which GopherJS does not implement.
}
+
+package reflect
+
+func ExampleStructOf() {}

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

Same here.

func ExampleStructOf() {
    // Blocked on https://github.com/gopherjs/gopherjs/issues/499.
}
@dmitshur

dmitshur Feb 21, 2017

Member

Same here.

func ExampleStructOf() {
    // Blocked on https://github.com/gopherjs/gopherjs/issues/499.
}
@@ -0,0 +1,7 @@
+// +build js
+
+package cookiejar

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

I think this needs to be package cookiejar_test. The example is in an external test package.

See https://github.com/golang/go/blob/go1.8/src/net/http/cookiejar/example_test.go#L5.

@dmitshur

dmitshur Feb 21, 2017

Member

I think this needs to be package cookiejar_test. The example is in an external test package.

See https://github.com/golang/go/blob/go1.8/src/net/http/cookiejar/example_test.go#L5.

@@ -0,0 +1,8 @@
+// +build js
+
+package reflect

This comment has been minimized.

+package cookiejar
+
+func ExampleNew() {
+ // network access not supported by GopherJS, and this test depends on httptest.NewServer

This comment has been minimized.

@dmitshur

dmitshur Feb 21, 2017

Member

gofmt?

@dmitshur

dmitshur Feb 21, 2017

Member

gofmt?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 21, 2017

Member

@flimzy I've pushed some fixes to this PR, I hope that's okay with you. See last 4 commits and their commit messages for details.

Member

dmitshur commented Feb 21, 2017

@flimzy I've pushed some fixes to this PR, I hope that's okay with you. See last 4 commits and their commit messages for details.

@flimzy

This comment has been minimized.

Show comment
Hide comment
@flimzy

flimzy Feb 22, 2017

Contributor

Thank you @shurcooL. I have squished a number of my commits now as well.

I believe the only outstanding issue you brought up is that of reading the file size to pre-allocate an appropriate buffer. Do you think that's still worth doing? The official ioutils package has some pretty elaborate logic about how it handles that (specifically in that it doesn't trust the reported file size to always be accurate). I suspect there are cases of networked filesystems where the size might be under-reported, especially immediately after having written the file. If you think it's worth optimising that bit of code, I can continue to work on it. But as you mentioned, with our small file sizes (the vast majority only a few dozen bytes, I suspect), it may not be worth it.

Contributor

flimzy commented Feb 22, 2017

Thank you @shurcooL. I have squished a number of my commits now as well.

I believe the only outstanding issue you brought up is that of reading the file size to pre-allocate an appropriate buffer. Do you think that's still worth doing? The official ioutils package has some pretty elaborate logic about how it handles that (specifically in that it doesn't trust the reported file size to always be accurate). I suspect there are cases of networked filesystems where the size might be under-reported, especially immediately after having written the file. If you think it's worth optimising that bit of code, I can continue to work on it. But as you mentioned, with our small file sizes (the vast majority only a few dozen bytes, I suspect), it may not be worth it.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 22, 2017

Member

No problem. Don't hesitate to squash my commits too, there's no reason not to. This PR will likely be merged as one squashed commit anyway.

As for the read file preallocation issue, I don't think it's worth it. It's fine as is.

At the most, you can do a stat to get the file size and use that to set initial buffer size. bytes.Buffer will still grow if needed, if the initial size was underestimated. But I doubt it's worth it. Add some quick timeTaken := time.Since(started) printf statements and see how long it takes to the read the file (and how much of a difference it makes).

Leaving it as is is fine by me. I commented because I wanted us to be aware and make a conscious decision, that's all.

Member

dmitshur commented Feb 22, 2017

No problem. Don't hesitate to squash my commits too, there's no reason not to. This PR will likely be merged as one squashed commit anyway.

As for the read file preallocation issue, I don't think it's worth it. It's fine as is.

At the most, you can do a stat to get the file size and use that to set initial buffer size. bytes.Buffer will still grow if needed, if the initial size was underestimated. But I doubt it's worth it. Add some quick timeTaken := time.Since(started) printf statements and see how long it takes to the read the file (and how much of a difference it makes).

Leaving it as is is fine by me. I commented because I wanted us to be aware and make a conscious decision, that's all.

@@ -18,7 +18,7 @@ test:
- go tool vet *.go # Go package in root directory.
- for d in */; do echo $d; done | grep -v tests/ | grep -v third_party/ | xargs go tool vet # All subdirectories except "tests", "third_party".
- >
- gopherjs test --short --minify --run='^Test'
+ gopherjs test --short --minify

This comment has been minimized.

@dmitshur

dmitshur Feb 22, 2017

Member

This change is so clean and beautiful now. 👍 😻

@dmitshur

dmitshur Feb 22, 2017

Member

This change is so clean and beautiful now. 👍 😻

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 26, 2017

Member

@neelance Do you want to look this over again, since there've been some changes based on my feedback? I think by now it has both mine and @flimzy's approval.

Member

dmitshur commented Feb 26, 2017

@neelance Do you want to look this over again, since there've been some changes based on my feedback? I think by now it has both mine and @flimzy's approval.

compiler/natives/src/testing/example.go
+ defer func() {
+ dstr := fmtDuration(time.Now().Sub(start))
+
+ // Close pipe, restore stdout, get output.

This comment has been minimized.

@dmitshur

dmitshur Feb 26, 2017

Member

Minor, we've changed the code to use file rather than pipe, but the comment still says pipe.

I can fix it.

@dmitshur

dmitshur Feb 26, 2017

Member

Minor, we've changed the code to use file rather than pipe, but the comment still says pipe.

I can fix it.

@neelance

This comment has been minimized.

Show comment
Hide comment
@neelance

neelance Feb 26, 2017

Member

Looks good to me, feel free to merge.

Member

neelance commented Feb 26, 2017

Looks good to me, feel free to merge.

@flimzy

This comment has been minimized.

Show comment
Hide comment
@flimzy

flimzy Feb 26, 2017

Contributor

Thanks, by the way, for your help in getting this PR polished, @shurcooL . I've been without my normal development machine the last week, which made responding to your comments in a timely and efficient manner more difficult than usual.

Contributor

flimzy commented Feb 26, 2017

Thanks, by the way, for your help in getting this PR polished, @shurcooL . I've been without my normal development machine the last week, which made responding to your comments in a timely and efficient manner more difficult than usual.

flimzy and others added some commits Feb 17, 2017

Don't import filepath
And use `js` build tag
compiler/natives/src: Print expected output for disabled examples.
gopherjs tool testFuncs.load does not take into account the overridden
natives, see https://github.com/gopherjs/gopherjs/blob/b9bcb1da229a59cc1e1d168401662cb6450aae08/tool.go#L827.
So we still need to print the expected output for the example to pass.

Given there are only 2 disabled examples, it's easier to just do this
in short term. But it might be good to eventually improve code for
overriding natives so that examples are properly overriden as well.
compiler/natives/src/testing: Update comment to match behavior.
This version of runExample uses a temporary file rather than pipe.
Update comment to match.

@dmitshur dmitshur merged commit 7d94d73 into gopherjs:master Feb 27, 2017

1 check was pending

ci/circleci CircleCI is running your tests
Details
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Feb 27, 2017

Member

No problem @flimzy, I'm happy if I can I do something useful and help make progress!

Thanks for the PR! I've rebased it on latest master to avoid conflicts in the generated vfsdata file, and merged as one squashed commit (this PR preserves the development history).

Member

dmitshur commented Feb 27, 2017

No problem @flimzy, I'm happy if I can I do something useful and help make progress!

Thanks for the PR! I've rebased it on latest master to avoid conflicts in the generated vfsdata file, and merged as one squashed commit (this PR preserves the development history).

@flimzy flimzy referenced this pull request in jtolds/gls Mar 4, 2017

Merged

Add support for GopherJS build target #3

@flimzy flimzy deleted the flimzy:examples branch Jul 9, 2018

@flimzy flimzy referenced this pull request in golang/go Jul 9, 2018

Open

all: test Examples on wasm? #25933

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment