Skip to content

Embed core GopherJS packages into build system; enable vendoring of GopherJS. #787

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

Merged
merged 7 commits into from
Apr 21, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
tests: Add test that GopherJS can be vendored.
Rely on runtime.GOARCH and t.Skip to skip tests that shouldn't be run
with GopherJS. It's more clear and reliable than a file-wide build tag.

Include stderr from gopherjsvendored_test.sh in output of TestGopherJSCanBeVendored.
This is only to get get more information when it fails.
  • Loading branch information
dmitshur committed Apr 20, 2018
commit d0d69c08b6af458cebb60d3295f643e4a471029d
48 changes: 48 additions & 0 deletions tests/gopherjsvendored_test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#!/bin/sh
# Don't run this file directly. It's executed as part of TestGopherJSCanBeVendored.

set -e

tmp=$(mktemp -d "${TMPDIR:-/tmp}/gopherjsvendored_test.XXXXXXXXXX")

cleanup() {
rm -rf "$tmp"
exit
}

trap cleanup EXIT HUP INT TERM

# copyGoPackage copies Go package with import path $1 to directory $2.
# The target directory is created if it doesn't exist.
copyGoPackage() {
mkdir -p "$2"
pkgDir=$(go list -f '{{.Dir}}' "$1")
# Copy all files (not directories), other than ones that start with "." or "_".
for f in $(find -H "$pkgDir" -maxdepth 1 -name "[^._]*" -type f); do
cp "$f" "$2"
done
}

# Make a hello project that will vendor GopherJS.
mkdir -p "$tmp/src/example.org/hello"
echo 'package main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thought, but the heredoc syntax often reads a bit cleaner in these multi-line situations because the redirect is on the same line:

cat <<EOD > "$tmp/src/example.org/hello/main.go"
package main

import "github.com/gopherjs/gopherjs/js"

func main() {
    js.Global.Get("console").Call("log", "hello using js pkg")
}
EOD 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I wasn't familiar with this syntax, but I'll consider it next time.


import "github.com/gopherjs/gopherjs/js"

func main() {
js.Global.Get("console").Call("log", "hello using js pkg")
}' > "$tmp/src/example.org/hello/main.go"

# Vendor GopherJS and its dependencies into hello project.
for pkg in $(go list -f '{{if not .Goroot}}{{.ImportPath}}{{end}}' $(go list -f '{{.ImportPath}} {{join .Deps " "}}' github.com/gopherjs/gopherjs)); do
copyGoPackage "$pkg" "$tmp/src/example.org/hello/vendor/$pkg"
done

# Make $tmp our GOPATH workspace.
export GOPATH="$tmp"

# Build the vendored copy of GopherJS.
go install example.org/hello/vendor/github.com/gopherjs/gopherjs

# Use it to build and run the hello command.
(cd "$GOPATH/src/example.org/hello" && "$GOPATH/bin/gopherjs" run main.go)
25 changes: 23 additions & 2 deletions tests/gorepo_test.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
// +build !js

package tests_test

import (
"os"
"os/exec"
"runtime"
"testing"
)

// Go repository basic compiler tests, and regression tests for fixed compiler bugs.
func TestGoRepositoryCompilerTests(t *testing.T) {
if runtime.GOARCH == "js" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thinking behind dropping the // +build !js build tag here?

Copy link
Member Author

@dmitshur dmitshur Apr 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation is written in the commit message of d0d69c0:

Rely on runtime.GOARCH and t.Skip to skip tests that shouldn't be run
with GopherJS. It's more clear and reliable than a file-wide build tag.

t.Skip exists per-test (rather than for the entire file) and can offer an individual message for the skip reason. This was better in this context. Build tags were used previously because I didn't get the idea that t.Skip could be used instead until now.

t.Skip("test meant to be run using normal Go compiler (needs os/exec)")
}

args := []string{"go", "run", "run.go", "-summary"}
if testing.Verbose() {
args = append(args, "-v")
Expand All @@ -22,3 +25,21 @@ func TestGoRepositoryCompilerTests(t *testing.T) {
t.Fatal(err)
}
}

// Test that GopherJS can be vendored into a project, and then used to build Go programs.
// See issue https://github.com/gopherjs/gopherjs/issues/415.
func TestGopherJSCanBeVendored(t *testing.T) {
if runtime.GOARCH == "js" {
t.Skip("test meant to be run using normal Go compiler (needs os/exec)")
}

cmd := exec.Command("sh", "gopherjsvendored_test.sh")
cmd.Stderr = os.Stdout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not catching the stderr and dumping it as an error message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is considered failed if the exit code is non-zero, or if the stdout output is not exactly the expected "hello using js pkg\n" value.

Stderr can be anything, it shouldn't fail the test. It's just information.

For example, if you run the test without the sourcemap module, it'll print "gopherjs: Source maps disabled. Install source-map-support module for ..." message to stderr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

got, err := cmd.Output()
if err != nil {
t.Fatal(err)
}
if want := "hello using js pkg\n"; string(got) != want {
t.Errorf("unexpected stdout from gopherjsvendored_test.sh:\ngot:\n%s\nwant:\n%s", got, want)
}
}
7 changes: 5 additions & 2 deletions tests/lowlevel_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
// +build !js

package tests_test

import (
"bytes"
"io/ioutil"
"os/exec"
"path/filepath"
"runtime"
"testing"
)

Expand All @@ -15,6 +14,10 @@ import (
//
// See https://github.com/gopherjs/gopherjs/issues/279.
func TestTimeInternalizationExternalization(t *testing.T) {
if runtime.GOARCH == "js" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question on why the build tag isn't sufficient in this situation and why you prefer the t.Skip

t.Skip("test meant to be run using normal Go compiler (needs os/exec)")
}

got, err := exec.Command("gopherjs", "run", filepath.Join("testdata", "time_inexternalization.go")).Output()
if err != nil {
t.Fatalf("%v:\n%s", err, got)
Expand Down