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

feat: Add stubs for more of the reflect package #2624

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

kyleconroy
Copy link
Contributor

With this change, tinygo can now compile and pass the example_test.go for the text/template package.

@dgryski @dkegel-fastly How do I add the example_test.go to tinygo?

Replaces #2574

@kyleconroy kyleconroy changed the base branch from release to dev February 8, 2022 05:58
@dkegel-fastly
Copy link
Contributor

Simplest might be to add testdata/template.{go,txt} and then add "template.go" into main_test.go's list of tests.

template.txt should contain the expected output.

@kyleconroy
Copy link
Contributor Author

kyleconroy commented Feb 8, 2022

Looks like my initial testing was incorrect. example_test.go does not run with this patch. Instead it fails with the following error:

Dear panic: unimplemented: (reflect.Value).MethodByName()

I didn't realize that example_test.go doesn't have tests in it, only two examples. By default, tinygo test doesn't run these.

@dkegel-fastly
Copy link
Contributor

You could add a smoke test instead a la #2565

Then once you get that other fix pushed upstream, maybe add testcases/protobuf.{go,txt}.

@dgryski
Copy link
Member

dgryski commented Feb 8, 2022

Yeah, I ran into exactly that MethodByName (or FieldByName) panic during my testing.

@dkegel-fastly
Copy link
Contributor

That windows failure looks spurious.

@kyleconroy
Copy link
Contributor Author

Smoke test added, let me know if you'd like it in a different place / format.

@dkegel-fastly
Copy link
Contributor

You also need to invoke it from Makefile's smoketest rule, otherwise it doesn't get run.

Most of the tests in that rule specify a target, but this one probably shouldn't.

Note that the tinygo maintainers may have different feedback; I have added a smoke test, but am a relative newcomer.

@kyleconroy
Copy link
Contributor Author

I added the line but forgot to commit it. Will fix

@kyleconroy
Copy link
Contributor Author

Maybe I need to specify a target in the Makefile?

cd tests/text/template/smoke && /go/bin/tinygo test -c && rm smoke.test
# os
/home/circleci/project/src/os/dir_unix.go:44:36: undeclared name: readdirMode
/home/circleci/project/src/os/dir_unix.go:44:76: undeclared name: DirEntry
/home/circleci/project/src/os/file_unix.go:139:56: undeclared name: DirEntry
/home/circleci/project/src/os/dir_unix.go:116:14: undeclared name: readdirName
/home/circleci/project/src/os/dir_unix.go:118:21: undeclared name: readdirDirEntry
/home/circleci/project/src/os/file_unix.go:145:29: undeclared name: testingForceReadDirLstat
/home/circleci/project/src/os/file_unix.go:154:24: info.Mode().Type undefined (type FileMode has no field or method Type)
FAIL
make: *** [Makefile:311: smoketest] Error 1

Exited with code exit status 2

@dankegel
Copy link
Contributor

dankegel commented Feb 9, 2022

Heh. Nope! Note which build failed.... it's the go 1.15 build. Perhaps you took your example from go 1.16 or newer, and it needs 1.16-only features, who knows. It's probably fine to exclude 1.15 on this file. I think the syntax is:

--- a/tests/text/template/smoke/smoke_test.go
+++ b/tests/text/template/smoke/smoke_test.go
@@ -1,3 +1,6 @@
+//go:build go1.16
+// +build go1.16
+
 // Copyright 2011 The Go Authors. All rights reserved.

The other problem is that you have not turned it into a test, so it's still not compiling the examples.
(Adding support for examples to tinygo wouldn't be all that hard, but nobody has gotten around to it.)
To turn it into a test, just change the signatures and names, e.g.

-func ExampleTemplate() {
+func TestTemplate(t *testing.T) {

and of course import testing, and fix the one variable name clash and any lint problems that pop up.

They won't be the greatest tests of all, but they will do just fine as a "does it compile at all" smoke test.

@kyleconroy
Copy link
Contributor Author

  • Updated example_test.go to the version for Go 1.15
  • Changed the examples to tests

@dkegel-fastly
Copy link
Contributor

Oh, apologies, it's not the version of go you got the code from, it's the version the user is running (and thus which go code they're getting.)
So you'll still need the build constraint.

@dkegel-fastly
Copy link
Contributor

Oh, ffs:

package github.com/tinygo-org/tinygo/tests/text/template/smoke: build constraints exclude all Go files in /home/circleci/project/tests/text/template/smoke

So add a pacifier test file, I guess.

@kyleconroy
Copy link
Contributor Author

Okay I give up lol. Is there a better place / way for me to add these tests?

@dkegel-fastly
Copy link
Contributor

heh. Hang on, let me give it a shot.

@kyleconroy
Copy link
Contributor Author

@dkegel-fastly any luck?

@dkegel-fastly
Copy link
Contributor

Been swamped, will try in the morning.

@dkegel-fastly
Copy link
Contributor

dkegel-fastly commented Feb 11, 2022

luck. See last commit in https://github.com/dkegel-fastly/tinygo/commits/compile-text-template

empty.go and the build constraint can be deleted once we drop support for go 1.15, I think.

@kyleconroy
Copy link
Contributor Author

@dankegel tests are now passing. Is this ready to merge?

@dkegel-fastly
Copy link
Contributor

dkegel-fastly commented Feb 14, 2022

No, now it's ready for code review :-) Congrats on getting this far, though!

This is the point I often get stuck at, especially when a maintainer is on vacation...

Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

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

Thanks! Can you rebase and squash some of these commits?

cc @deadprogram I suppose we can always squash on merge too.

With these methods stubbed out, the text/template package can be
imported. These changes also allow code generated by protoc to compile.
@kyleconroy
Copy link
Contributor Author

@dgryski Done!

Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

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

LGTM

@niaow niaow merged commit b406422 into tinygo-org:dev Feb 24, 2022
@dkegel-fastly
Copy link
Contributor

Now that this is merged, are you going to open an issue on vtprotobuf as mentioned earlier ( #2574 (comment) ) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants