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(mutators) Avoid creating some unnecessary mutators for (a && b) … #3346

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

edi9999
Copy link
Contributor

@edi9999 edi9999 commented Jan 8, 2022

Fix #3339

Will be adding a few comments inline

Before:

- if (a && b) {}
+ if (false) {}
+ if (false && b) {}
// And others

- if (a || b) {}
+ if (true) {}
+ if (true && b) {}
// And others

After:

- if (a && b) {}
+ if (false) {}
// And others

- if (a || b) {}
+ if (true) {}
// And others

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.

Looks incredibly simple if you put it like this 🙂

Got some small remarks. Also CI is failing

@edi9999
Copy link
Contributor Author

edi9999 commented Jan 14, 2022

@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.

@nicojs
Copy link
Member

nicojs commented Jan 14, 2022

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 logicalOperatorMutator

@edi9999
Copy link
Contributor Author

edi9999 commented Jan 16, 2022

@nicojs is there a way to update the e2e tests quickly ?

I'm running :

npm run e2e

Since it stops after the first fail, I have to manually relaunch the tests and copy paste the output.

@nicojs
Copy link
Member

nicojs commented Jan 16, 2022

You can run them manually by running npm test in an e2e/test/xxx directory. But every time you change something in Stryker, you need to reinstall with npm install from the e2e root dir

@edi9999
Copy link
Contributor Author

edi9999 commented Jan 17, 2022

Tests are now passing, can you please review @nicojs ?

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.

LGTM 👍

@nicojs nicojs merged commit 0f60ecf into stryker-mutator:master Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants