-
Notifications
You must be signed in to change notification settings - Fork 250
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(mutators) Avoid creating some unnecessary mutators for (a && b) … #3346
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks incredibly simple if you put it like this 🙂
Got some small remarks. Also CI is failing
packages/instrumenter/test/unit/mutators/conditional-expression-mutator.spec.ts
Outdated
Show resolved
Hide resolved
packages/instrumenter/src/mutators/conditional-expression-mutator.ts
Outdated
Show resolved
Hide resolved
@nicojs about the failed test, I had written the following in an autoreview ( I don't know if you can see it, I am not used to github reviews). This test is not passing, c1 || c2 || c3 is, as the current state in master, generating only those mutators : if (true), if (false), if(true || c3), if (false || c3) It should in my opinion generate those mutants instead; if (true), if (false), if(false || c2 || c3), if (c1 || false || c3), if (c1 || c2 || false) This is I think linked to the fact that babel will parse a "3-way" boolean operation as two separate nested || operations. In this test, I could see that the condition if (isBooleanExpression(path)) { is actually just hit once. Maybe this test should simply be removed from this pull request, and a new issue created for this issue. |
Yes, that's probably best. Probably a good idea to discuss it in a separate issue as well, as I think the test cases are already covered largely by the |
6ac42b2
to
c802671
Compare
@nicojs is there a way to update the e2e tests quickly ? I'm running :
Since it stops after the first fail, I have to manually relaunch the tests and copy paste the output. |
You can run them manually by running |
Tests are now passing, can you please review @nicojs ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Fix #3339
Will be adding a few comments inline
Before:
After: