rogeliog / jest-runner-mocha

A Mocha runner for Jest
70 stars 12 forks source link

Add support for resetting files #11

Open hswolff opened 6 years ago

hswolff commented 6 years ago

Hello! It's me again :)

After #8 lands we'll have support for including test harness files for each individual process used to run tests. For my use cases these test harness files have some generic before and after handlers that I expect to run for each individual test file that I'm running.

However I encountered an issue when attempting to integrate this runner with our tests at Mongo. When this runner spins up its test runner processes it recycles them for each individual test. Meaning that test A and test C are ran within process 1 and test B and test D are ran in process 2.

However the test harness files are only ran once, when the process is first created. This means that for test A and test B the before and after handlers are ran, however because test C and D are ran within the existing processes, and the test harness files are already cached, they do not get the test harness handlers.

I have a solution that I'm using currently that I'd love to get upstreamed but it's a little more controversial IMO and wanted to discuss it first before opening a PR.

The solution is available for viewing here. The clearModuleCache is copy and pasted from mocha directly so it keeps parity there as well.

Questions:

rogeliog commented 6 years ago

This is really interesting, when does purge gets called in Mocha?

hswolff commented 6 years ago

It's called here, when you're in 'watch' mode and you trigger a re-run of the mocha test suite.

rogeliog commented 6 years ago

I think this makes sense for me, but I would love to see what @ljharb thinks

ljharb commented 6 years ago

I'm a bit confused; why would the setup files be run more than once per suite?

If you have logic you need run before all tests, your setup file would use a beforeAll callback, no?

hswolff commented 6 years ago

I believe beforeAll will run before each describe block. I can create a test case to verify this.

The behavior i want is to have my test harness code ran once before all tests and once after all tests. With beforeEach I was seeing the code ran more than desired.

ljharb commented 6 years ago

beforeAll should run once, before the entire suite - not just before each describe. beforeEach of course runs before each it.

However, with jest-runner-mocha, if there's N files, there's N suites - so beforeAll would get run before each one (which might happen to have one describe per file). Since each file runs in its own process, this would be necessary for setting up polyfills/shims/mocks/etc.

If what you want is something that runs before/after the entire jest-run suite, I think that'd need to be a separate option unique to this runner.

hswolff commented 6 years ago

Well explained!

Indeed, what I want is something ran before/after either every suite ran within an individual process, or even before/after an individual suite run.

I did some more debugging and it seems like the current state of code results in some unexpected behavior (at least to me).

With the addition of the cliOptions.file option we can now include test harness code for mocha suites. i.e. I can have a testSafeguards.js file passed into cliOptions.file which contains:

before(function() {
  // do stuff before all tests
  console.log('before');
});
let count = 1;
beforeEach(function() {
  // before each test 
  console.log('beforeEach', count++);
});
after(function() {
  // after all tests
  console.log('after');
});

When I run use this file and run it before two individual test files (each being a test suite with one top level describe block) I get:

When ran with mocha directly ```shell before js/common/components/ActivityFeed/ActivityDescription beforeEach 1 ✓ displays the right template string per activity type common/components/ActivityFeedTable when rendered with defaultProps beforeEach 2 ✓ shows the expected 3 columns beforeEach 3 ✓ shows 1 rows beforeEach 4 ✓ contains a dropdown filter in the first header column beforeEach 5 ✓ does not show the empty state message when row for global only users beforeEach 6 ✓ shows a globalOnly tooltip when row for isMmsAdmin beforeEach 7 ✓ shows a globalOnly tooltip when rendered with no activities beforeEach 8 ✓ shows the expected 3 columns beforeEach 9 ✓ shows 0 rows beforeEach 10 ✓ shows the empty state message after 10 passing (471ms) ```
When ran with `CI=true jest --verbose --maxWorkers=1` (maxWorkers 1 to force reuse of the same process) ```shell before beforeEach 1 beforeEach 2 beforeEach 3 beforeEach 4 beforeEach 5 beforeEach 6 beforeEach 7 beforeEach 8 beforeEach 9 after PASS test/common/components/ActivityFeed/ActivityFeedTable.spec.js ✓ shows the expected 3 columns (0ms) ✓ shows 1 rows (0ms) ✓ contains a dropdown filter in the first header column (0ms) ✓ does not show the empty state message (0ms) ✓ shows a globalOnly tooltip (0ms) ✓ shows a globalOnly tooltip (0ms) ✓ shows the expected 3 columns (0ms) ✓ shows 0 rows (0ms) ✓ shows the empty state message (0ms) PASS test/common/components/ActivityFeed/ActivityDescription.spec.js ✓ displays the right template string per activity type (0ms) Test Suites: 2 passed, 2 total Tests: 10 passed, 10 total Snapshots: 0 total Time: 6.94s ```


When I include this proposed change to delete the nodejs cache this is the output I get from jest.

Output when deleting nodejs cache ```shell before beforeEach 1 beforeEach 2 beforeEach 3 beforeEach 4 beforeEach 5 beforeEach 6 beforeEach 7 beforeEach 8 beforeEach 9 after PASS test/common/components/ActivityFeed/ActivityFeedTable.spec.js ✓ shows the expected 3 columns (0ms) ✓ shows 1 rows (0ms) ✓ contains a dropdown filter in the first header column (0ms) ✓ does not show the empty state message (0ms) ✓ shows a globalOnly tooltip (0ms) ✓ shows a globalOnly tooltip (0ms) ✓ shows the expected 3 columns (0ms) ✓ shows 0 rows (0ms) ✓ shows the empty state message (0ms) globalTestSafeguards before beforeEach 1 after PASS test/common/components/ActivityFeed/ActivityDescription.spec.js ✓ displays the right template string per activity type (0ms) Test Suites: 2 passed, 2 total Tests: 10 passed, 10 total Snapshots: 0 total Time: 8.138s ```


It appears that due to the test harness file testSafeguards.js having been already required() for the first test file, when the second one is require'd the testSafeguards.js test hooks are not re-created as the file is already cached.

This change will resolve that issue and fix this issue.

hswolff commented 6 years ago

Would love to get this included upstream!