wasm test suite #1116

bradleypeabody · 2020-05-17T02:11:21Z

As a followup to #1091, here is a working test suite for webassembly.

Running go test ./tests/wasm will run the tests using the local copy of Chrome. Or providing the -docker-headless will use a docker container with headless Chrome (which is what the CircleCI config calls). I've made some small changes to the Makefile and CircleCI config to integrate it in (let's see if it works correctly in that environment).

Each test is a set of two files - the program to be compiled with TinyGo (*_pgm.go), and a regular Go test (*_test.go) that drives the test case. It expects tinygo, go and if applicable docker to be available in the path.

The amount of wiring required to make this work is unfortunate but all the guts are in one folder (tests/wasm - let me know if there is a better home for it).

…g-docker-images/

…e.Get on string, and I do not see where it is coming from. but it works on the latest which is ok for now

bradleypeabody · 2020-05-17T04:40:05Z

Switched over to installing chrome in CI test env instead of docker and build tagged some tests for Go 1.14+ and it's looking good now.

deadprogram · 2020-05-22T10:38:22Z

This is some seriously impressive work @bradleypeabody thank you very much!

@johanbrandhorst @aykevl @jaddr2line and anyone else who is deeply into wasm take a look at these changes, please. They pass, and they perform real tests of a bunch of things that we need tested, so my first impulse is to merge it.

aykevl · 2020-05-22T11:47:23Z

.circleci/config.yml

+          command: |
+            wget https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb
+            sudo apt install ./google-chrome-stable_current_amd64.deb
+            rm ./google-chrome-stable_current_amd64.deb


The rm here is not strictly necessary because the CI container will be thrown away anyway after use. But there's no harm either (rm should be very fast).

aykevl · 2020-05-22T11:50:22Z

tests/wasm/fmtprint_test.go

@@ -0,0 +1,37 @@
+// +build go1.14


Does this really depend on Go 1.14 (just checking)?

Ideally not, but I was getting a strange error I could not debug on go 1.13. I will make another attempt to fix as part of the refactor to remove docker.

Thinking about this again, I suspect you may be running into this Go 1.13 incompatibility: #1035

aykevl · 2020-05-22T12:02:21Z

tests/wasm/chan_pgm.go

@@ -0,0 +1,18 @@
+// +build tinygo


It looks like you put the actual tests in the same directory as the code that runs the tests (where the files necessary for running the tests seem to be prefixed with a 0).

I would propose this organization for consistency with other tests:

Move the tests itself (tests/wasm/*_pgm.go) to a subdirectory. The conventional subdirectory for this would be testdata (tests/wasm/testdata). This also means no // +build tinygo is necessary.

Remove the 0 prefix. If needed, you could make sure tests are sorted together with a prefix (such as test-pgm.go etc).

Maybe avoid underscores in filenames: they have a somewhat special in that they can represent build tags (unless they really should be conditionally compiled with the given build tag as a suffix). At least that's the convention I've followed elsewhere.

Take a look at the tests in the transform package for example, which puts *_test.go files in the same package as the actual transforms but puts input/output files in transform/testdata.

aykevl · 2020-05-22T12:05:48Z

tests/wasm/0setup_test.go

+
+	// copy wasm_exec.js from targets to this dir
+	b, err := ioutil.ReadFile("../../targets/wasm_exec.js")
+	must(err)


It may be a good idea to add an explanation to must calls, such as:

must(err, "reading wasm_exec.js failed")

You can then print it together in the panic.

aykevl · 2020-05-22T12:12:50Z

tests/wasm/0setup_test.go

+	os.Remove("wasm_exec.js")
+	os.Remove("server")


Would it be possible to use a temporary directory instead? That avoids the need for remembering to clean up all the files, and doesn't leave any files in the testing directory after the test aborts unexpectedly.

The pattern I usually use is:

dir, err := ioutil.TempDir("", "tinygo-wasm-test") // handle err defer os.RemoveAll(dir) // Run tests, storing temporary files in dir.

This pattern cleans up all files even in the case of a panic, but still doesn't leave any around on very unexpected exits (e.g. power loss) in the source directory. The OS will still clean up the temporary files, probably on the next reboot.

aykevl · 2020-05-22T12:20:35Z

tests/wasm/0dockerfile

@@ -0,0 +1,10 @@
+
+FROM chromedp/headless-shell:latest


I should admit I'm not a fan of using Docker everywhere (although it can be very useful in certain situations).

Why should the tests run in a Docker container? Can they also run directly on the host? In my experience, Docker often adds quite a bit of (startup) overhead and consumes a significant amount disk space.
(Not directly a request for change, but for clarification).

Yes, in retrospect I agree. The rationale was that it would be easy to ensure consistent behavior both in the CI and for local developers using docker. However, since the CI now uses Chrome directly, and for developers it's probably just as easy, if not easier, to install Chrome as it is to install docker, I think all the docker stuff can just come out.

This would mean developers wanting to run the wasm test suite would just need to have Chrome installed in the usual place for their system, which I think is not any worse a requirement than requiring docker.

I'll do this refactor along with the rest of the points mentioned here. The result will definitely be simpler.

aykevl · 2020-05-22T12:23:37Z

I should add that I really like these kind of changes. They may add some CI overhead but should definitely avoid regressing WebAssembly support, as has happened a few times in the past.

johanbrandhorst · 2020-05-22T15:52:06Z

tests/wasm/0setup_server.go

@@ -0,0 +1,91 @@
+// +build ignore


Is this right?

It is intentional for the docker setup as it needed a separate server executable; but I'm going to remove docker support per the conversation with Ayke and simplify things. The code from this file will move into a _test.go file and become directly part of the test setup/teardown.

- all the docker stuff is now gone, the only requirement is a local install of Chrome - test programs moved into tests/wasm/testdata and "_pgm" removed from the name, unnecessary tinygo build tags removed - all of the test setup stuff now lives in one file, setup_test.go, and it runs the necessary http server internally - a temp dir is used for all output files - any remaining `must` calls replaced with `mustf` and explanations added - added t.Parallel(), seems to work fine, can be easily removed later if there are memory problems with too many open headless Chrome instances testing fmtprint against all versions again in the pipeline to see if i can debug

bradleypeabody · 2020-05-22T22:41:59Z

Thanks everone for the feedback! I've made the following changes:

all the docker stuff is now gone, the only requirement is a local install of Chrome
test programs moved into tests/wasm/testdata and "_pgm" removed from the name, unnecessary tinygo build tags removed
all of the test setup stuff now lives in one file, setup_test.go, and it runs the necessary http server internally
a temp dir is used for all output files
must removed, not necessary with this simpler setup
added t.Parallel(), seems to work fine, can be easily removed later if there are memory problems with too many open headless Chrome instances
added a fmt_test.go with an explanation about the fmt package and Go 1.13 where doing import "fmt" causes a panic. I spent a little time trying to track it down and didn't entirely figure it out. Would be good to fix but I don't think it needs to hold things up.

deadprogram · 2020-05-23T05:55:43Z

Thanks for making all of the changes, @bradleypeabody I just tried this out and it worked flawlessly!

I think that it would be best to squash these commits before merging. Do you want to do this, or would you like me to do so?

bradleypeabody · 2020-05-23T05:58:13Z

@deadprogram Okay awesome! If you're in there now then feel free, otherwise I'll do this in the morning (about 12 hours from now).

bradleypeabody added 7 commits May 16, 2020 18:58

wasm test suite

b70182d

testing setup_remote_docker per https://circleci.com/docs/2.0/buildin…

9e5decb

…g-docker-images/

try just installing chrome instead of using docker for circleci

f7d903c

build go1.12+ and add debug output

5c3af35

making event test go1.14+ for now

c3686c6

debug output

16a7aec

making this go1.14+ - 1.13 is giving a panic syscall/js: call of Valu…

569d4e7

…e.Get on string, and I do not see where it is coming from. but it works on the latest which is ok for now

aykevl reviewed May 22, 2020

View reviewed changes

johanbrandhorst reviewed May 22, 2020

View reviewed changes

bradleypeabody added 3 commits May 22, 2020 14:32

rm unnecessary rm

8e79bb6

cleanup and added fmt test to show go 1.13 issue

0540e67

deadprogram merged commit 95f509b into tinygo-org:dev May 23, 2020

niaow added this to the v0.14 milestone Jun 27, 2020

wasm test suite #1116

wasm test suite #1116

Uh oh!

Conversation

bradleypeabody commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradleypeabody commented May 17, 2020

Uh oh!

deadprogram commented May 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradleypeabody May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aykevl commented May 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradleypeabody commented May 22, 2020

Uh oh!

deadprogram commented May 23, 2020

Uh oh!

bradleypeabody commented May 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bradleypeabody commented May 17, 2020 •

edited

Loading

bradleypeabody May 22, 2020 •

edited

Loading