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

Rules for running jest tests #395

Closed
wants to merge 1 commit into from
Closed

Rules for running jest tests #395

wants to merge 1 commit into from

Conversation

vmax
Copy link

@vmax vmax commented Oct 26, 2018

No description provided.

@Globegitter
Copy link
Contributor

Globegitter commented Oct 26, 2018

@vmax thanks for getting started on this, that is awesome, we are using jest internally in a quite hacky way so far and I have not been able yet to write a proper rule, so happy to see something happening here.

Generale comments: I wonder if it makes sense to stick to the existing naming and name this jest_node_test, what do you think? Also bazel treats certain exit codes in a special way as also used by the jasmin tests: https://github.com/bazelbuild/rules_nodejs/blob/master/internal/jasmine_node_test/jasmine_runner.js#L11 That is one thing I actually gut stuck with last time, trying to map the exit codes, but maybe you have some ideas.

I am also seeing you are copying the file to get around the jest symlink issue: jestjs/jest#5356 - I wonder if there is also a better way of getting around this as I could imagine that impacting performance once you reach a certain number of tests. I know it is possible to run jest path/to/file.js but have not tested yet if that fixes the symlink issues.

Also can you provide examples? This is a simple way of being able to tests the functionality but also to show how to use the rules.

@Globegitter
Copy link
Contributor

Ahh I am also just seeing #394 for naming - yeah that also makes some sense.

@alexeagle
Copy link
Collaborator

/cc @FrozenPandaz since we just discussed it

@Globegitter
Copy link
Contributor

Globegitter commented Nov 14, 2018

I had more of a play with it and no matter how jest is invoked it does not recognize symlinks. I do have started adding symlink support to jest a while ago, here is an incomplete PR: jestjs/jest#7364

If anyone wants to help finish it up quicker.

I also had a brief look into creating our own runner that bypasses the whole jest symlink logic but it was not easy to figure out where exactly to plug that in and it does not seem to be officially supported, see: https://stackoverflow.com/questions/50827216/use-jest-run-or-jest-runcli-to-run-all-tests-or-ways-to-run-jest-programmati & jestjs/jest#3737

@dicarlo2
Copy link

@Globegitter AFAICT you can avoid the Jest symlinks issue entirely by using the --runTestsByPath option.

In case it helps, here's our entry_point script for running jest tests: https://gist.github.com/dicarlo2/7b920185b28058f7f7379ad39834472b. We use a modified set of rules from this repo, so there may be some differences that I'm not accounting for, but this might give some ideas on how to make jest work.

At a high level, we just call jest.runCLI with the appropriate templated arguments (list of files, whether we're running in ci, etc.). We then have some logic for maintaining snapshots - ww write out to the test output directory all of the added/updated snapshots + a summary.json file that contains the paths to those snapshots + the paths of the deleted ones. We have another process running that watches the local bazel out directory and syncs the changes over to the workspace. No idea if this is optimal, but it works for us.

@Globegitter
Copy link
Contributor

@dicarlo2 thanks for posting that, that is pretty useful - will take a look.

@Globegitter
Copy link
Contributor

@dicarlo2 Do you also have the set of rules that you are using with this runner? Or some more hints? TEMPLATED_filePaths, is quite obvious, but what are you setting TEMPLATED_env to and when are you setting TEMPLATED_update? In any case I will start investigating, maybe that will make some of the things clearer by itseld

@dicarlo2
Copy link

dicarlo2 commented Nov 27, 2018

@Globegitter Here's the rules file: https://gist.github.com/dicarlo2/3f35d9ef183a0cfe856997b7122a105a
(And the test sources aspect: https://gist.github.com/dicarlo2/8254f5b8c3d954515287d5e33f20466f)

Note that we are using a slightly customized variant of nodejs_binary, hence the load from an internal file. However, I don't think we actually require any of the changes that we made for the jest rule, so in theory dropping in the nodejs_binary from this repo should work (with a few modifications to expose the underlying function and arguments).

Also, FWIW this is one of the first rules I wrote for bazel and I haven't updated it since, so it may be doing some janky things.

@Globegitter
Copy link
Contributor

@dicarlo2 Thanks for posting this, that was enough for me to also put together a set of jest_rules I have here: https://github.com/ecosia/bazel_rules_nodejs_contrib/tree/jest-rules/internal/nodejs_jest_test

I changed things a bit to make it more conformant with test rules from other languages. Feel free to have a look if you are interested :)

@vmax
Copy link
Author

vmax commented Jan 6, 2020

We've switched our primary test framework to be Jasmine such that the officially supported rule is used. Therefore, we no longer plan on supporting nodejs_jest_test.

@vmax vmax closed this Jan 6, 2020
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this pull request Oct 17, 2020
Closes bazel-contrib#395

PiperOrigin-RevId: 231848839
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this pull request Oct 18, 2020
Closes bazel-contrib#395

PiperOrigin-RevId: 231848839
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.

4 participants