Add test command to tinygo #243

carolynvs · 2019-03-18T04:06:54Z

Stuff that works so far:

Added function to figure out the the package path based on the current directory so that we can call tinygo test without passing in a package path
Test command passes flag to compile indicating that a test binary should be built
Adds the test build tag
Imports packages from test files

I'm stuck on how to find the TestMain that I have defined in my test file. It is coming back nil when I query for it.

You can try it out by running make tinygo-test.

After that, I'm not sure how I would reflect on the functions defined in the test files and then build up the test suite (M) in a basic block. First step I figured was calling a simple predefined test suite though. So that is what I'd like to understand before diving into other stuff.

This is for #228

deadprogram · 2019-03-18T08:58:52Z

Hi @carolynvs thank you very much for working on this. One initial comment without looking at anything, please use the dev branch as your base since that is where all the active development is taking place.

You may have already seen it, but take a look at https://github.com/tinygo-org/tinygo/blob/master/CONTRIBUTING.md#how-to-use-our-github-repository for more info.

I have taken the liberty of changing the branch for this PR to dev accordingly.

I will look at the content of this PR later today, thanks for getting us collectively started on this important set of features.

aykevl · 2019-03-19T23:01:33Z

src/testing/testing.go

+"_syscall.Exit", referenced from:
+  _main in main.o
+ld: symbol(s) not found for architecture x86_64
+clang: error: linker command failed with exit code 1 (use -v to see invocation)


syscall.Exit is implemented in the runtime, as can be seen here:

https://github.com/golang/go/blob/cb8aefd3b06f39679ebe1abbd38abef52cbd76b7/src/syscall/syscall.go#L102-L105

The implementation is here:

https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/runtime/runtime.go#L61

It could be implemented in TinyGo in the following way in the runtime package:

//go:linkname exit exit func exit(code int32) //go:linkname syscall_Exit syscall.Exit func syscall_Exit(cide int) { exit(int32(code)) }

What this basically does is implement syscall.Exit by calling the libc exit function.
You could make a separate PR for this feature.

Reading it again, I see I made a mistake. The exit declaration should be the following:

//go:export exit func exit(code int32)

(//go:export is a bit of a misnomer, we're importing a function here. But it lets the compiler know it should follow the C ABI).

I tried to get this to work, and while there aren't any errors, it doesn't emit the specified exit code either. Maybe someone else can pick this up and get it to work.

Hmmm maybe it should be //go:extern exit?

Oh wait! I wasn't doing it right. I was testing it with tinygo run but after trying it with tinygo build then running the binary it created, that worked. So let me try more to get it to work with tinygo test.

So I thought it didn't work so I wrote my own: #380 (sorry, didn't see your comment soon enough, don't want to cause duplicate work...)
And had the exact same realization. We should probably fix this, it makes logical sense that tinygo run exits with the same exit code as the program you're trying to run. But that is somewhat of a separate issue.

aykevl · 2019-03-19T23:15:44Z

Great to get this up! You haven't exactly chosen an easy feature to implement as a first thing.

I see a few changes in this PR that would be easier to review and merge as separate PRs, as they are not directly related to the test subcommand but are likely required for it:

The syscall.Exit function I commented on.
The change to the reflect package.
Perhaps also the feature to leave out the package to build. For example, tinygo flash -target=microbit currently expects a package name. It would be useful to have this as a generic feature for flash, run, build, etc. and later test. It could be as simple as letting this default to "." if no package is given, instead of printing an error.

aykevl · 2019-03-19T23:46:28Z

I've found another interesting file:
https://github.com/golang/go/blob/master/src/cmd/go/internal/test/test.go

Regarding the testing itself:
I think TestMain cannot be found because it is eliminated during the first round of dead code elimination. Anything not reachable from main is removed.
Also, I think testing should be implemented in an earlier stage, directly on the AST. As far as I know, the go toolchain does the same thing. This should also solve the DCE of TestMain. I think the logic should be something like this;

If there is no main.TestMain: generate it.
If there is a main function: delete it (for testing main packages).
Create a main function that calls testing.MainStart and then main.TestMain with the result.

This logic should likely live in the loader package. Take a look at the CGo support for how the AST can be modified.
After these modifications, the program can be compiled as usual.

carolynvs · 2019-03-24T14:48:48Z

After rebasing on the dev branch (sorry about that!), I am seeing new behavior. When I run tinygo test I get the following output

error: import cycle:
	github.com/tinygo-org/tinygo/tests/example1
	testing
	runtime/trace
	context
	math/rand
	internal/testenv
	os/exec
	context
 at /usr/local/go/src/os/exec/exec.go:25:2

I am guessing that however I am building or running tinygo now that I'm on dev is no longer using the alternate source files in tinygo/src. So when it imports "testing", it is importing the real testing from go 1.11, which is then bringing in all those other packages. In tinygo/src/testing, it only imports fmt.

Do you have advice on what has changed, or what I was taking advantage of on master that no longer works on dev that is causing the wrong files to be used?

Sorry I'm not quite sure if I should be saying that the GOROOT is wrong? It's hard coded to /usr/local/go (which is where go 1.11 is installed on my machine) but that was what master was doing too so I'm a bit unsure of how to describe the problem.

aykevl · 2019-03-24T14:59:49Z

After rebasing on the dev branch (sorry about that!)

Actually, I think that's a good thing. Especially right now when lots of stuff changes.

When I run tinygo test I get the following output

Ah, yes, this was changed in #247. Standard library packages that must be overridden should now be explicitly listed here:

tinygo/compiler/compiler.go

Line 199 in a2d0f79

case "machine", "os", "reflect", "runtime", "sync":

carolynvs · 2019-03-24T15:05:25Z

Thank you for the fast reply! That fixed it and I'm able to keep working on it. 👍

carolynvs · 2019-03-24T15:28:34Z

FYI, the go toolchain used to do this at the SSA level it looks like. In 1.12 they've completely redone it, but here's the code for 1.11

https://github.com/golang/tools/blob/release-branch.go1.11/go/ssa/testmain.go

aykevl · 2019-03-24T15:38:32Z

It's in the go/ssa package, but still they seem to produce regular Go code there:

https://github.com/golang/tools/blob/e9f45831faab4562ce3f53f7196b67c08f948e86/go/ssa/testmain.go#L210-L259

Very interesting how they make the test main package using a template.

deadprogram · 2019-04-22T08:27:10Z

Now that v0.5.0 has been released, it would be great to get back to this PR. What do we need to do to support this?

carolynvs · 2019-04-22T21:37:21Z

I actually got tinygo test working where the user supplied the TestMain (yeah I realize that's not the right signature, but it was the first step of what was needed)

func TestMain() {
	fmt.Println("running tests...")
	m := testing.M{
		Tests: []testing.TestToCall{
			{Name: "TestFoo", Func: TestFoo},
		},
	}
	m.Run()
}

Next step is to generate the TestMain if they don't have it (with the real signature), or call theirs, passing in the proper arguments if they do have one. Now that I have the right place in the loader, that is doable.

I'll get what I have working so far pushed up so you can see and keep working on it.

aykevl · 2019-04-23T00:06:13Z

That's great! Maybe I can help with the TestMain generation?
Also related: compiling packages independently, see #285. That is a big change I want to do in smaller steps, and will definitely affect how tinygo test is implemented. For example, I want to make dead code elimination optional in preparation of splitting the compilation per package and caching the results.

carolynvs · 2019-04-23T00:11:53Z

Oh shoot. At some point with the rebasing and updates/changes, I introduced a regression. When the code in the test files call a package, like fmt.Println I'm getting a null pointer exception. So I'm chasing down why my test imports aren't working correctly anymore. Hopefully I can get that figured out tonight.

carolynvs · 2019-04-23T01:25:54Z

Turns out my problem was just that I had rebased onto dev from a few weeks ago. I rebased to the most recent (post 0.5.0) and the regression went away. Phew! 😅

carolynvs · 2019-04-23T03:07:30Z

Got a bit further tonight. I've updated the call to TestMain to supply a populated test suite, so it's finally calling the proper TestMain signature TestMain(m *testing.M).

Unfortunately, I've hit a snag. The same code that worked in the TestMain, when moved into the main function and passed into TestMain as an argument is causing things to not compile properly.

$ tinygo test
DEBUG: TEST MAIN CONTENTS
package main

func main () {
	m := &testing.M{
		Tests: []testing.TestToCall{

			{Name: "TestFoo", Func: TestFoo},

		},
	}
	TestMain(m)
}

panic: interface conversion: ssa.Member is nil, not *ssa.Function

goroutine 1 [running]:
github.com/tinygo-org/tinygo/ir.(*Program).SimpleDCE(0xc0023a6c80)
	/Users/carolynvs/go/src/github.com/tinygo-org/tinygo/ir/passes.go:69 +0xb86
github.com/tinygo-org/tinygo/compiler.(*Compiler).Compile(0xc000001380, 0xc00001a1f8, 0x2b, 0xe, 0x0)
	/Users/carolynvs/go/src/github.com/tinygo-org/tinygo/compiler/compiler.go:255 +0x891
main.Compile(0xc00001a1f8, 0x2b, 0x4419c31, 0x4, 0xc00014a000, 0xc000eb3f08, 0x442f120, 0x0, 0x0)
	/Users/carolynvs/go/src/github.com/tinygo-org/tinygo/main.go:80 +0x47d
main.Test(0xc00001a1f8, 0x2b, 0x0, 0x0, 0xc0000c1f08, 0x0, 0xe)
	/Users/carolynvs/go/src/github.com/tinygo-org/tinygo/main.go:322 +0xe6
main.main()
	/Users/carolynvs/go/src/github.com/tinygo-org/tinygo/main.go:639 +0xbe2

I've pushed up all the latest code in case that helps. I am assuming that the error interface conversion: ssa.Member is nil, not *ssa.Function is due to {Name: "TestFoo", Func: TestFoo}, in the new main that I'm generating, since Func: TestFoo is supposed to be a function pointer.

I tried adding a explicit import to the package being tested and changing that line to Func: p.TestFoo but that didn't help. 🤷‍♀️

aykevl · 2019-04-23T14:02:23Z

loader/loader.go

 		if err != nil {
 			return err
 		}
 	}

+	if compileTestBinary {
+		p.SwapTestMain()


It looks like this returns an error that is not handled: error: invalid source.

carolynvs · 2019-05-26T17:25:01Z

Ok, I hope enough of this is working now to merge and improve incrementally?

$ go install .
$ cd tests/example1
$ tinygo test
=== RUN   TestFail1
--- FAIL: TestFail1
	TestFail1 failed because of stuff and things

=== RUN   TestFail2
--- FAIL: TestFail2
	TestFail2 failed for reasons

=== RUN   TestPass
--- PASS: TestPass

exit status 2
FAIL

Generates a TestMain and calls all Test* functions in the package.
Followup: Improve signature matching to check that the Test* function has the correct function signature. I am just matching on the function name.
Followup: Implement the gaps in tinygo so that we can use upstream testing package instead of the stub that I wrote. I didn't spend time matching what's in testing because ultimately we should be working towards using this upstream package instead.
Followup: Support the tinygo test -run flag to filter the tests to run.
Followup: Implement syscall.Exit. I tried to follow your suggested implementation. It compiles and doesn't error out, but it also doesn't cause the supplied exit code to be returned either.

aykevl · 2019-05-26T17:48:15Z

I have put these changes in the staging branch so that CircleCI will run all tests.

carolynvs · 2019-05-26T17:49:36Z

I was able to get syscall.Exit to work, and have the exit code propagate from the test binary to tinygo test properly. Just pushed that up too.

aykevl · 2019-05-26T17:55:52Z

src/testing/testing.go

+}
+
+// Fatal is equivalent to Log followed by Fail
+func (t *T) Error(args ...interface{}) {


nit: the comment here has the wrong function name in it.

aykevl · 2019-05-26T17:57:50Z

target.go

@@ -301,6 +301,30 @@ func getGopath() string {
 	return filepath.Join(home, "go")
 }

+func getPackageRoot() (string, error) {


In the meantime, I wrote my own not realizing that you already had one here:

tinygo/target.go

Line 319 in 421ef04

func getGoroot() string {

I think it would be best to use that one instead.

Oh nevermind, getPackageRoot does something different.

Why do you try to resolve the package relative to GOPATH? Relative paths should just work (or there is a bug in the compiler), so in theory this function could be replaced with return ".".
Symbol names are currently a bit ugly with that. Their import path should probably be replaced with just main (so you get main.main and not ugly stuff like ./src/examples/blinky1/blinky1.go.main), but that is a separate issue.

I will remove getPackageRoot and use "."

aykevl · 2019-05-26T18:02:02Z

Can you run make fmt? That should format all the code using gofmt.

carolynvs · 2019-05-26T18:43:37Z

Fixed!

deadprogram · 2019-05-26T19:05:17Z

Is there anything stopping us from squashing/merging this PR now? Seems like there is already enough here to be useful.

deadprogram · 2019-05-27T07:58:41Z

When I rebased this branch and dev I get the following errors:

$ make tinygo-test 
cd tests/tinygotest && tinygo test
# runtime
../../src/runtime/runtime_unix.go:22:6: exit redeclared in this block
../../src/runtime/runtime.go:111:6:     other declaration of exit
../../src/runtime/runtime_unix.go:83:6: syscall_Exit redeclared in this block
../../src/runtime/runtime.go:114:6:     other declaration of syscall_Exit
../../src/runtime/runtime_unix.go:84:7: cannot use code (variable of type int) as int32 value in argument to exit
Makefile:89: recipe for target 'tinygo-test' failed
make: *** [tinygo-test] Error 1

There are conflicting implementations of exit now, how should we proceed to resolve this?

deadprogram · 2019-05-27T08:16:19Z

I commented the implementation of exit etc. in the runtime.go and was able to execute the tests within tinygo directory itself. Yes!

OK, then I went on to try to call some tests within the TinyGo drivers directory. I received the following error:

$ tinygo test ./thermistor
could not determine the package root from the current directory "/home/ron/Development/tinygo/drivers" and gopath "/home/ron/.gvm/pkgsets/go1.12/global"

The problem might be that I am using a symlink from my development directory to the actual gopath. I thought that we had that problem at one point, and it had had been resolved for tinygo build? If so that is the same thing we will need here probably.

aykevl · 2019-05-28T14:38:02Z

loader/loader.go

@@ -362,7 +479,7 @@ func (p *Package) importRecursively() error {
 		if importedPkg.Importing {
 			return &ImportCycleError{[]string{p.ImportPath, importedPkg.ImportPath}, p.ImportPos[to]}
 		}
-		err = importedPkg.importRecursively()
+		err = importedPkg.importRecursively(includeTests)


Doesn't this only apply to topmost packages?
Imported packages may also have tests with imports in the test files, but I don't think they should be included.

aykevl · 2019-05-28T14:41:16Z

loader/loader.go

@@ -62,6 +66,11 @@ func (p *Program) Import(path, srcDir string) (*Package, error) {
 	p.sorted = nil // invalidate the sorted order of packages
 	pkg := p.newPackage(buildPkg)
 	p.Packages[buildPkg.ImportPath] = pkg
+
+	if p.mainPkg == "" {
+		p.mainPkg = buildPkg.ImportPath


This implies that the first import sets the mainPkg, which seems a bit unclean to me as it may be unexpected that the order of imports matters here (for example, things would be different if you imported the runtime package first). I think it would be more explicit if mainPkg is exported and set in compiler/compiler.go.

Is this a change that could be made in a follow-up PR? It's not actually based on the import statement order. The compiler calls ldprogram.Import or ldprogram.ImportFile, depending on whether it is testing a package or a single go file, that is how the mainPkg is being set.

I'm feeling pressured to get this merged quickly (understandable, sorry it's taken so long) and as you can tell I don't have time to work on this anymore. I tried to export and set the package from the compiler, but it doesn't have all the information to set it. So I'd have to replicate logic from the loader and this would slow down getting the PR merged even more.

aykevl · 2019-05-28T14:44:10Z

loader/loader.go

@@ -91,6 +100,11 @@ func (p *Program) ImportFile(path string) (*Package, error) {
 	p.sorted = nil // invalidate the sorted order of packages
 	pkg := p.newPackage(buildPkg)
 	p.Packages[buildPkg.ImportPath] = pkg
+
+	if p.mainPkg == "" {
+		p.mainPkg = buildPkg.ImportPath


aykevl · 2019-05-28T14:55:27Z

target.go

@@ -301,6 +301,30 @@ func getGopath() string {
 	return filepath.Join(home, "go")
 }

+func getPackageRoot() (string, error) {


Why do you try to resolve the package relative to GOPATH? Relative paths should just work (or there is a bug in the compiler), so in theory this function could be replaced with return ".".
Symbol names are currently a bit ugly with that. Their import path should probably be replaced with just main (so you get main.main and not ugly stuff like ./src/examples/blinky1/blinky1.go.main), but that is a separate issue.

aykevl · 2019-05-28T15:06:13Z

main.go

@@ -654,6 +680,14 @@ func main() {
 		}
 		err := Run(flag.Arg(0), *target, config)
 		handleCompilerError(err)
+	case "test":
+		pkgRoot, err := getPackageRoot()


Using the current directory is not unique to go test: it is also supported by go install, go build, etc. and it would be useful to extend the support to these other commands. Also, go test supports providing a package path just like go install.
What I would propose is that in all relevant commands (build, flash, run, test), the package to process is determined using the same method. That is: if a command line argument was given that package path is used, and otherwise it is determined based on the current working directory.

I don't want to change the behavior of the other commands in this PR, it's out of scope. But I did change test to test if the positional argument is not present and use ".". I think that's what you were asking for, and would work for the other commands.

aykevl · 2019-05-28T15:09:45Z

test.go

+	"strings"
+)
+
+func RunTests() error {


This file does not seem to be used. I guess this was intended as a test, in which case it should probably be renamed to <something>_test.go.

I removed it since it wasn't used.

aykevl · 2019-05-28T15:12:36Z

@carolynvs can you rebase this branch on the current dev branch? There are a few conflicts.

deadprogram · 2019-06-04T13:46:00Z

Oh, how much I want this feature!

deadprogram · 2019-06-12T20:11:21Z

OK update to above comment. I need this feature now. How can I help?

aykevl · 2019-06-12T22:05:14Z

Also, I should note that there is an upcoming big refactor that will conflict with this PR. It would be great to have this merged before then to avoid more (and harder to resolve) merge conflicts.

* Test command passes flag to compile indicating that a test binary should be built * Adds the test build tag * Imports packages from test files Stuck on how to find the TestMain that I have defined in my test file. It is coming back nil. Not sure how I would reflect on the functions defined in the test files and then build up the test suite (M) in a basic block. First step I figured was calling a simple predefined test suite though.

This is currently failing with panic: interface conversion: ssa.Member is nil, not *ssa.Function

Don't use build/tinygo since it assumes that you are building llvm from source Example: $ go install && make tinygo-test cd tests/tinygotest && tinygo test === RUN TestFail1 --- FAIL: TestFail1 TestFail1 failed because of stuff and things === RUN TestFail2 --- FAIL: TestFail2 TestFail2 failed for reasons === RUN TestPass --- PASS: TestPass exit status 2 FAIL

* Remove getPackageRoot and use . instead * Do not recursively import test files * Remove unused test

carolynvs · 2019-06-14T18:38:52Z

I've rebased and addressed the review feedback with the exception of the following items:

loader.mainPkg is still set as-is (see comment)
build and other top level commands don't default to working against the current package when no positional argument is specified (out of scope for this PR)

deadprogram · 2019-06-18T10:23:16Z

Hi @carolynvs agreed that this makes some very substantial progress. As you point out, still more is needed on this feature, but can be added in subsequent PRs. Most excellent contribution, thank you so much for putting in all the effort.

Now squashing/merging.

carolynvs force-pushed the tinygo-test branch from 96186f0 to 0b6970f Compare March 18, 2019 04:10

deadprogram changed the base branch from master to dev March 18, 2019 08:55

aykevl reviewed Mar 19, 2019

View reviewed changes

carolynvs force-pushed the tinygo-test branch from 0b6970f to 48ba6f6 Compare April 23, 2019 01:41

aykevl reviewed Apr 23, 2019

View reviewed changes

carolynvs force-pushed the tinygo-test branch 2 times, most recently from 24776c8 to ae344fb Compare May 26, 2019 14:54

carolynvs changed the title ~~WIP: Add test command to tinygo~~ Add test command to tinygo May 26, 2019

carolynvs force-pushed the tinygo-test branch 2 times, most recently from cee1844 to c5f2a9d Compare May 26, 2019 17:22

aykevl reviewed May 26, 2019

View reviewed changes

aykevl reviewed May 28, 2019

View reviewed changes

aykevl mentioned this pull request Jun 3, 2019

Stub out reflect.New and reflect.Zero #390

Closed

carolynvs added 11 commits June 14, 2019 12:13

scaffolding

349af15

getpackageroot

38122d8

Call TestMain instead of main

104c1d7

Call TestMain with the proper signature

076c4fb

This is currently failing with panic: interface conversion: ssa.Member is nil, not *ssa.Function

Fix running tests

1ef3b60

Improve failed test formatting

7edfa96

Propagate the test binary exit code

667f9ab

Format code

dd765dc

Fix docstring

04e49fa

carolynvs force-pushed the tinygo-test branch from 910b2a1 to 3ff8ce0 Compare June 14, 2019 18:33

Incororate review feedback

b94fe7f

* Remove getPackageRoot and use . instead * Do not recursively import test files * Remove unused test

carolynvs force-pushed the tinygo-test branch from 3ff8ce0 to b94fe7f Compare June 14, 2019 18:35

deadprogram merged commit 208e171 into tinygo-org:dev Jun 18, 2019

Add test command to tinygo #243

Add test command to tinygo #243

Conversation

carolynvs commented Mar 18, 2019 • edited Loading

deadprogram commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aykevl May 26, 2019 • edited Loading

Choose a reason for hiding this comment

aykevl commented Mar 19, 2019

aykevl commented Mar 19, 2019

carolynvs commented Mar 24, 2019

aykevl commented Mar 24, 2019

carolynvs commented Mar 24, 2019

carolynvs commented Mar 24, 2019

aykevl commented Mar 24, 2019

deadprogram commented Apr 22, 2019

carolynvs commented Apr 22, 2019

aykevl commented Apr 23, 2019

carolynvs commented Apr 23, 2019

carolynvs commented Apr 23, 2019

carolynvs commented Apr 23, 2019

Choose a reason for hiding this comment

carolynvs commented May 26, 2019

aykevl commented May 26, 2019

carolynvs commented May 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aykevl commented May 26, 2019

carolynvs commented May 26, 2019

deadprogram commented May 26, 2019

deadprogram commented May 27, 2019

deadprogram commented May 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aykevl commented May 28, 2019

deadprogram commented Jun 4, 2019

deadprogram commented Jun 12, 2019

aykevl commented Jun 12, 2019

carolynvs commented Jun 14, 2019

deadprogram commented Jun 18, 2019

carolynvs commented Mar 18, 2019 •

edited

Loading

aykevl May 26, 2019 •

edited

Loading