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

feat(test runner): Support for disable bail #3074

Merged
merged 39 commits into from
Sep 1, 2021

Conversation

Edgpaez
Copy link
Contributor

@Edgpaez Edgpaez commented Aug 14, 2021

As discussed in #2996 and #3012 this PR enables the user to set a disableBail option to collect all tests killing a mutant. Continuation of #3061

Notes on the implementation:

  1. Initially only implemented for jest. The rest of the test runners should continue to work without changes
  2. We only track the first error message of the first failing test.

To be done:

  • enableBail should be deprecated.
  • In the integration tests, jest doesn't bail and always report two tests killing a mutant, although I am sure (and the tests confirm) I'm telling Jest to run with bail = true. I am not sure what to do about this or if this is a problem with my implementation.
    [@nicojs]: This is a problem of our jest-runner implementation. Running with bail apparently never worked 🤷‍♀️. I've removed the implementation and mimicked the behavior of reporting the first failing test when bail is enabled.
  • merge jasmine2-node-no-mocks into jest-circus
  • add disableBail to the RunOptions (for both DryRunOptions and MutantRunOptions)
  • Implement disableBail in the cucumber runner
  • Implement disableBail in the jasmine runner
  • Implement disableBail in the karma runner
  • Implement disableBail in the mocha runner

Sorry, something went wrong.

@Edgpaez
Copy link
Contributor Author

Edgpaez commented Aug 16, 2021

Ok. So I progressed a little:

  1. I added the deprecation warning for enableBail option in the jest config
  2. I added disableBail to MutantRunOptions making sure it's not a breaking change. Once everything is implemented we can remove the optional. Now it'd be easier for other test runners to add support to disableBail. I did not added it to RunOptions because we were bailing on the DryRun anyway. I didn't see much value in configuring the dryRun not to bail. Mutant coverage would not be affected and it would only make running stryker slower. You think that makes sense? or should I move it to RunOptions?

@Edgpaez
Copy link
Contributor Author

Edgpaez commented Aug 17, 2021

I've been trying to make jest bail in the integration tests based on config but I haven't succeeded.

Calling jest's runCLI with runInBand: true makes no difference. Neither does passing it in the config option. maxConcurrency=1 and maxWorkers=1 don't seem to make a difference either.

I added a test that should pass if jest bailed on the first fail. It's skipped for now because jest does not bail

Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your continued work on this. I've started implementing the e2e tests for jest (and prepared it for adding mocha, jasmine, karma and cucumber as well).

Please take a look at my remarks with regards to jest.

@nicojs
Copy link
Member

nicojs commented Aug 26, 2021

I've opened an issue in jest: jestjs/jest#11766. I'll try to implement programmatical --bail support there.

We've discussed this in the community meetup and decided to investigate in which version of jest the 'monkey patch' was working. If we can't find a version of jest in which it worked, we'll just remove the feature entirely.

We can mimic the behavior of bail by simply only reporting on the first failing test when bail is enabled.

@nicojs nicojs changed the title WIP - Create the disableBail option feat(test runner): Support for disable bail Sep 1, 2021
@nicojs nicojs enabled auto-merge (squash) September 1, 2021 13:26
@nicojs nicojs merged commit 0962232 into stryker-mutator:master Sep 1, 2021
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

2 participants