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

fix(jest-runner): Support multi project jest-configs #2780

Closed

Conversation

swist
Copy link
Contributor

@swist swist commented Mar 5, 2021

This mimics behaviour of jest - if you want to use projects,
you need to pass the config by file. This teaches stryker to do
the same - if you set explicit config path in jest config,
that gets passed into jest and voila, multi project jest works :)

https://github.com/swist/stryker-test is the minimum reproducible
example repo for this

Gives a sensible-ish workaround for #2574 until jest fix their stuff (and it doesn't look like they will, the issue was open for a couple of years, so would be great to merge this instead!)

@swist swist force-pushed the bugfix/allow-multiple-jest-projects branch from fcdae68 to 7d35190 Compare March 5, 2021 20:41
This mimics behaviour of jest - if you want to use projects,
you need to pass the config by file. This teaches stryker to do
the same - if you set explicit config path in jest config,
that gets passed into jest and voila, multi project jest works :)

https://github.com/swist/stryker-test is the minimum reproducible
example repo for this
@swist swist force-pushed the bugfix/allow-multiple-jest-projects branch from 7d35190 to 455e170 Compare March 5, 2021 21:00
@swist swist changed the title fix(jest-runner): pass config by path if configPath provided fix(jest-runner): Support multi project jest-configs Mar 7, 2021
@swist
Copy link
Contributor Author

swist commented Mar 7, 2021

This looks like it fell victim to unrelated test flake, jasmine runner tests have sometimes been running out of heap on my local host too.

[edit] it was a real failure, should be fixed now

Turns out that we can also make projects play nice with stryker-modified jest configs. There are two requirements however - they need to have rootDir set in jest CLI args and we need to make sure that the filename ends in .json so that require doesn't trip over
@swist swist force-pushed the bugfix/allow-multiple-jest-projects branch from 46f9cc6 to 2828fcb Compare March 7, 2021 23:58
@nicojs
Copy link
Member

nicojs commented Mar 11, 2021

Thanks, @swist 🐱‍👤, I will need some time to look into this in detail.

@swist
Copy link
Contributor Author

swist commented Mar 11, 2021

TL;DR - jest needs to read config from file to behave correctly with multi project. This takes stryker modified jest-config and writes it to a specially suffixed .json file so that jest can also read the modifications. It doesn't teach stryker how to modify each config yet, I figured this could land in a separate PR

@nicojs
Copy link
Member

nicojs commented Mar 18, 2021

Ok, thanks for the explanation. I've started looking into this. Am I allowed to say: I hate jest for having this --projects feature 😔?

A feature like this deserves its very own "e2e" test project. I was trying to base it on your project, but there seems to be a problem. When I run npm t, but I get this error:

 ~/github/swist-stryker-test (master)$ npm t

> starter-kit@0.0.1 test /home/nicojs/github/swist-stryker-test
> jest

 PASS   server  server/app.test.js
GET /api 200 2.153 ms - 27
GET / 200 3.245 ms - 167
GET /main.js 200 0.835 ms - 47
GET /foobar 200 1.527 ms - 167
POST /foobar 404 1.283 ms - 146
 PASS   client  client/src/pages/Home.test.js
  ● Console

    console.error
      Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
          at Home (/home/nicojs/github/swist-stryker-test/client/src/pages/Home.js:8:32)
          at Router (/home/nicojs/github/swist-stryker-test/node_modules/react-router/cjs/react-router.js:99:30)

      17 |                      })
      18 |                      .then((body) => {
    > 19 |                              setMessage(body.message);
         |                              ^
      20 |                      })
      21 |                      .catch((err) => {
      22 |                              console.error(err);

      at console.error (node_modules/@testing-library/react/dist/act-compat.js:53:34)
      at printWarning (node_modules/react-dom/cjs/react-dom.development.js:67:30)
      at error (node_modules/react-dom/cjs/react-dom.development.js:43:5)
      at warnAboutUpdateOnUnmountedFiberInDEV (node_modules/react-dom/cjs/react-dom.development.js:23914:9)
      at scheduleUpdateOnFiber (node_modules/react-dom/cjs/react-dom.development.js:21840:5)
      at dispatchAction (node_modules/react-dom/cjs/react-dom.development.js:16139:5)
      at setMessage (client/src/pages/Home.js:19:5)

A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks.

Test Suites: 2 passed, 2 total
Tests:       7 passed, 7 total
Snapshots:   0 total
Time:        5.796 s
Ran all test suites in 2 projects.

@nicojs
Copy link
Member

nicojs commented Mar 19, 2021

Ok, I've added an e2e test.

As for the solution direction, I don't like test runners just writing files to the sandbox. This is problematic for a couple of reasons:

  1. Test runners run in parallel on the same sandbox. So 2 test runners might be trying to access the same file and get an error.
  2. Since test runners share the sandbox, they might be writing different configuration to the file resulting in race conditions (this is currently not the case because dryRun and mutantRun run in different phases of the mutation testing process, but we might want to change this in the future).
  3. When running with --inPlace mode, the file will linger after Stryker is done.

I've looked through jest's readConfig source code and I don't see an easy fix. It convinced me that it is actually a bug in Jest, rather than a feature. Maybe we can open an issue on their end?

@nicojs
Copy link
Member

nicojs commented Mar 19, 2021

I've opened jestjs/jest#11213

@swist
Copy link
Contributor Author

swist commented Mar 23, 2021

Ok, I've added an e2e test.

As for the solution direction, I don't like test runners just writing files to the sandbox.

Agreed and I thought I had a solution that didn't need this (just pass the original config) but then it turned out that striker sometimes rewrites the jest configs :(

This is problematic for a couple of reasons:

  1. Test runners run in parallel on the same sandbox. So 2 test runners might be trying to access the same file and get an error.

Is this strictly true? In my testing I get ~(# cpu cores/2)-1 sandboxes. Because the stryker modifications to the config are idempotent I figured it's just fine to timestamp them per sandbox instance

  1. Since test runners share the sandbox, they might be writing different configuration to the file resulting in race conditions (this is currently not the case because dryRun and mutantRun run in different phases of the mutation testing process, but we might want to change this in the future).

I guess the solution could be to namespace these configs by stage? Although I'd guess that because each stage generates a new, unique(ish) config that gets passed on, it doesn't really matter that much

  1. When running with --inPlace mode, the file will linger after Stryker is done.

Is there any way to add a clean-up hook after each run? Stryker already leaves the sandboxes behind, so from my perspective it wasn't a big deal

I've looked through jest's readConfig source code and I don't see an easy fix. It convinced me that it is actually a bug in Jest, rather than a feature. Maybe we can open an issue on their end?

The issue at jest has been open for a couple of years now, unlikely they will fix it :)

@nicojs
Copy link
Member

nicojs commented Apr 16, 2021

The issue at jest has been open for a couple of years now, unlikely they will fix it :)

Only 28 days, right? 😉 I've opened a PR, let's see what they do with it.

I agree that it is a small hack, but in my experience, hacks like these tend to linger and pile up. With all respect, but once it is in we will have to maintain it for as long as Jest is being used. Let's hope they agree to merge the PR.

Stryker already leaves the sandboxes behind

It shouldn't. After a successful run, it is cleaned. Also with --inPlace mode Stryker tries its best. See https://stryker-mutator.io/docs/stryker-js/configuration#cleantempdir-boolean

@swist
Copy link
Contributor Author

swist commented Apr 16, 2021

Hmm, maybe it's the failed runs that leave sandboxes behind. Jest have known about the problem since 2018: jestjs/jest#7415

It's linked from stryker already (#2574)

@nicojs
Copy link
Member

nicojs commented Apr 17, 2021

Cool, all the more reasons we should be able to get this merged in jest 💪

@nicojs
Copy link
Member

nicojs commented Apr 23, 2021

My PR fix just got merged in Jest 🃏 jestjs/jest#11307

So it should simply work out-of-the-box from the next release. Closing this PR

@nicojs nicojs closed this Apr 23, 2021
@MishaTyupa
Copy link

MishaTyupa commented Apr 13, 2022

Hey @nicojs . Thanks for contributing to this project.
It turns out that stryker js 5.6.1 version and jest runner 5.6.1 are not working with jest test path wildcards if you need to match, for instance, ~2k tests.
Here is my package.json
"devDependencies": {
"@stryker-mutator/core": "^5.6.1",
"@stryker-mutator/jest-runner": "^5.6.1"
},

and what I have in test path in my jest.config:
testMatch: [ '<rootDir>/**/test.js', '<rootDir>/**/**/*spec.js', '<rootDir>/**/**/tests/*spec.js' ]

It injects mutants to all source files successfully and then run the tests and fails with the following error:
Test runner crashed. Tried twice to restart it without any luck. Last time the error message was: Error: Child process [pid 19908] exited unexpectedly with exit code 1 (without signal).

Could you set up a similar project and try to debug and fix such an issue?
Thank you man for all your work on this project,
Best regards,
Mike

P.S here is my stryker.config
`/**

  • @type {import('@stryker-mutator/api/core').StrykerOptions}
    /
    module.exports = {
    mutate: [
    '/**/src/
    .js'
    ],
    logLevel: 'debug',
    fileLogLevel: 'trace',
    packageManager: 'npm',
    reporters: ['html'],
    testRunner: 'jest',
    tempDirName: 'stryker-tmp',
    coverageAnalysis: "perTest",
    jest: {
    projectType: 'custom',
    configFile: 'jest-stryker.config.js',
    enableFindRelatedTests: true
    },
    concurrency: 2
    };`

And jest.config
module.exports = { testTimeout: 60000, testEnvironment: 'jsdom', testRunner: 'jest-jasmine2', testMatch: [ '<rootDir>/**/test.js', '<rootDir>/**/**/*spec.js', '<rootDir>/**/**/tests/*spec.js' ], modulePathIgnorePatterns: [ 'npm-cache', '.npm' ] }

@nicojs
Copy link
Member

nicojs commented Apr 14, 2022

Thanks for coming back with this. I'll reopen this issue. We should add an e2e test for jest with a projects setup.

@nicojs nicojs reopened this Apr 14, 2022
@nicojs
Copy link
Member

nicojs commented Apr 14, 2022

Ooops, reopened the PR instead of the issue 😅

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.

3 participants