Skip to content

Move tests in js package to tests package. #792

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

Closed
dmitshur opened this issue Apr 16, 2018 · 8 comments
Closed

Move tests in js package to tests package. #792

dmitshur opened this issue Apr 16, 2018 · 8 comments

Comments

@dmitshur
Copy link
Member

dmitshur commented Apr 16, 2018

After PR #787 lands, the js and nosync packages will be considered "core" and embedded into the GopherJS compiler via compiler/gopherjspkg package. gopherjspkg needs to be regenerated whenever js or nosync changes (not often).

To make everything work, I embedded the entire js, nosync packages, including their tests, and did c964bad to make their tests run.

When reviewing #779, I've realized we might want to modify/add tests in js package more often. Having to regenerate gopherjspkg is inconvenient (e.g., especially so when in the process of writing/modifying a test). It also increases the size of the compiler, and I don't want to feel pressure to keep the tests smaller because of that.

Finally, there's always been an odd split of tests in js package and tests in tests package. Sometimes it's not clear where to add a test.

By moving js tests from js package into tests, all these problems are resolved.

All of js tests are external, so I think its should be easy. I think it's better than trying to make js normal package embedded but its tests not embedded.

I think a good way of thinking github.com/gopherjs/gopherjs/js package (after #787) is that it's somewhat like C pseudo-package (related to cgo), and it wouldn't make sense to try to put tests inside C.

@myitcv
Copy link
Member

myitcv commented Apr 16, 2018

To make everything work, I embedded the entire js, nosync packages, including their tests, and did c964bad to make their tests run.

Apologies if I'm missing something here, but why are we embedding the tests in the first place?

I would just make the change proposed here in #787.

@dmitshur
Copy link
Member Author

but why are we embedding the tests in the first place?

Because a package consists of normal .go files and _test.go files. I embedded the entire package, because any other approach would be more complicated.

I got this idea after I made #787. The goal there was to fix the vendoring issue.

I can give this a try, if it goes smoothly, we can get this done before #787. But there isn't a reason this must be done before.

dmitshur added a commit that referenced this issue Apr 20, 2018
This way, all GopherJS-specific tests are in tests package, rather than
having an arbitrary split. None of the package js tests were internal
to the package.

This helps a future change where the js package becomes embedded into
the compiler. It makes it easier to add and modify GopherJS tests
without having to regenerate the js package.

Resolves #792.
@dmitshur
Copy link
Member Author

Not seeing any surprises with them move so far. Sent #800.

@myitcv
Copy link
Member

myitcv commented Apr 20, 2018

Because a package consists of normal .go files and _test.go files. I embedded the entire package, because any other approach would be more complicated.

Is it not just a case of excluding _test.go files? Because nothing in those files can ever be imported, they never need to be embedded.

The issue with #800 is that the js tests are now further from the js package. It would seem better to not vendor the tests, that way we don't have to worry about moving tests in #800.

Is that not possible?

@dmitshur
Copy link
Member Author

The issue with #800 is that the js tests are now further from the js package.

Many tests in tests package also test js and other misc GopherJS behavior. Now they're all in one place. Further away from js isn't a big negative.

It would seem better to not vendor the tests, that way we don't have to worry about moving tests in #800.

That does not seem better to me. It would mean that a single import path can refer to different things depending on whether it's being used as a test package or as a normal Go package. It's complex conceptually, and the implementation becomes more complex too.

@myitcv
Copy link
Member

myitcv commented Apr 20, 2018

It's complex conceptually, and the implementation becomes more complex too.

Right, I think I can see where that complexity comes in.

I wonder, is there an alternative here that gets us the best of both worlds?

If we have a flag to gopherjs, something like -novendor, that doesn't read the embedded versions of js and nosync but instead reads them from disk as it does currently, then all of this is solved, no? We don't need to embed the test files and we simply test these two packages with -novendor.

Would this work?

@dmitshur
Copy link
Member Author

dmitshur commented Apr 20, 2018

We already have that, except it's a build tag. It's gopherjsdev. If you build GopherJS with go build -tags=gopherjsdev, it reads natives (and gopherjspkg) from disk rather than using embedded version.

This is documented at https://github.com/gopherjs/gopherjs/wiki/Developer-Guidelines#code-generation.

I've considered that and it doesn't change the situation. I still think it's better to make js package smaller in scope, akin to C package. All of its tests were external, and tests is a good place to have such tests.

@myitcv
Copy link
Member

myitcv commented Apr 20, 2018

@shurcooL and I discussed offline and we're going to proceed with #800.

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

No branches or pull requests

2 participants