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

Test file locations #4

Merged
merged 22 commits into from Oct 20, 2019
Merged

Test file locations #4

merged 22 commits into from Oct 20, 2019

Conversation

jonaskello
Copy link
Member

This PR will re-organize the location of the test files.

It seems like jest has a convention/recommendation of putting the tests in the same source tree as the app files under a folder named __test__ or alongside the app files like foo.ts andfoo.tests.ts. I have not found an offical recommendation but there are several places this is mentioned like here and here. Also looking at the jest source itself, it uses this pattern for example here.

Initially I tried to keep all the tests separated into their own folder but to do that we need to fight how jest was intended to be used so I've come to the conclusion it is better to follow the convention. See this issue in jest for some info about the issues in putting tests in an isolated folder.

One interesting thing about having the test files in the src folder is that they will get built to the lib folder and therefore if we are not careful they will also be published to npm. I looked into how the jest source avoids this and it seems they have a .npmignore file that excludes the test, like here. We currently use package.json files filed to whitelist published files instead of blacklisting in .npmignore. This blog post says files is better, but under the heading "The one time npmignore is ok" it also specifcially mentions the __test__folder and recommends having both files and .npmignore in that case.

@jonaskello
Copy link
Member Author

jonaskello commented Oct 19, 2019

Jest recommends having a __test__ folder in the same directory as the file to be tested.

This means there should be a __test__ folder on every level there are files tested, not only the top-level. Feels like having a lot of these folders on different levels is littering the code. But moving the test code closer to the app code may have a postive effect that you think about the test more when you change the app code. For that reason it might be even better to have the tests in a file just alongside the file being tested instead of in a __test__ folder.

@jonaskello
Copy link
Member Author

One advantage of having the tests in the same source tree as the app code is that when using typescript the tests do not need a separate tsconfig.json or a separate build step to verify them etc. Since they are part of the app code, just build the app and the tests are also built.

Now it becomes a matter of deciding if you want to the test-runner to run the compiled tests in libor if you want it to run the tests in src. To run the tests in lib nothing extra is needed except you first have to compile the app (the tests are compiled as part of the app). If you want it to run in src you need something extra that dynamically transforms .ts files to .js files, something like ts-jest.

In this monorepo, some packages are referring other packages code. So in order for the tests to work in the first place, the referred packages needs to be built (or at least I think that is the case). So maybe it makes sense to also build the package being tested before testing it.

@jonaskello
Copy link
Member Author

Now the problem with running the compiled tests under lib/__tests__ is that the snapshots will be stored under lib/__tests__/__snapshots__ and that folder by definition is not checked into source control. So when the CI server runs there are no shapshots (but when we run locally they exists so we get no error).

@jonaskello
Copy link
Member Author

jonaskello commented Oct 20, 2019

Seems like the choices we have for snapshot files are that we either:

  1. Run the tests directly in src using ts-jest (so the snapshots end up in src).
  2. We run the tests in lib and use a transformer for the snapshot paths as described in this issue to force the snapshots to end up in src. (That issue also talks about "co-locating" tests and snaps with the source file which is an interesting idea).
  3. We could also check-in parts of lib to source control of course but that does not seem like a good idea.

In a non-monorepo I think (1) is the simplest option. However since this is a monorepo, the only code that can be tested as *.ts files is the code in the package having the tests, referenced code in other packages will use compiled lib/*.js files because they are imported using node_modules resolution. So we need to not forget to build the referenced packages anyway so then we could just built it all before testing. Of course, we could override this with tsconfig-paths but that adds complexity to the setup.

@jonaskello
Copy link
Member Author

jonaskello commented Oct 20, 2019

I did an experiment with the snapshot resolver option. It seems to work well, we can run tests in lib/ and it writes/read snapshots in src/.

However I did discover that running tests in lib/ will not map back to the original source in error messages etc. This is not very friendly. So now I'm thinking that that running the tests directly in src is a better option since that will give us correct file and line-number in error messages. We could run tests directly in src without ts-jest. I think jest has built-in support for .ts files but without type-checking. However type-checking could be performed by building the source before testing.

@jonaskello
Copy link
Member Author

I went back to using ts-jest and putting each package as a separate project with its own config. This was the only way I could make ts-jest use the tsconfig.json for each package. I like this setup, the only drawback is that we cannot run tests for one project only this way. See this issue. So in order to run for one project/package we need to put separate jest.config.js in each package instead of having a single root jest.config.js.

@jonaskello jonaskello merged commit 39ff7ea into master Oct 20, 2019
@jonaskello jonaskello deleted the test-folders branch October 20, 2019 12:57
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.

None yet

1 participant