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

JavaScript mutator improvements #1898

Merged
merged 3 commits into from
Dec 11, 2019

Conversation

brodybits
Copy link

@brodybits brodybits commented Nov 27, 2019

  • JavaScript mutator code improvements
    • use raw string mutations internally whenever possible
    • generate some of the mutations as AST nodes from the Babel types API
    • use some ternaries
    • remove some intermediate const declarations no longer needed
    • remove a couple of internal utility functions no longer needed
  • add Arrow Function Mutator for JavaScript (already supported by the existing TypeScript mutator)
  • remove lodash.clonedeep from dependencies in packages/javascript-mutator/package.json

These improvements should help reduce the amount of consistency between the JavaScript and TypeScript mutators that I raised in #1893.

/cc @kmdrGroch

@brodybits brodybits changed the title JavaScript mutator improvements [WIP] JavaScript mutator improvements Nov 29, 2019
@brodybits
Copy link
Author

WIP for now, waiting for PR #1903 to be reviewed and merged before I finish resolving the review comments.

@kmdrGroch it was not clear to me whether we need to go back to if statements for all mutators or not.

@bartekleon
Copy link
Member

bartekleon commented Nov 29, 2019

@brodybits Thanks. I only meant these 2 I review. Simple short if's are ok, but if you have multiple ones, it isn't very clean.
I have looked through and it looks nice

Christopher J. Brody added 2 commits December 1, 2019 19:49
- use raw string mutations internally, whenever possible
- generate some of the mutations as AST nodes from the Babel types API
- use some ternaries
- remove some intermediate const declarations no longer needed
- remove a couple of internal utility functions no longer needed
@brodybits brodybits changed the title [WIP] JavaScript mutator improvements JavaScript mutator improvements Dec 2, 2019
@brodybits
Copy link
Author

I just merged from master and pushed some fixes to ArrayDeclarationMutator.ts:

  • revert use of a ternary
  • resolve issue with mutating Array() function call
  • resolve lint issues on ArrayDeclarationMutator.ts (again)

This PR is no longer WIP.

Thanks again to @kmdrGroch for the review comments, final review would be much appreciated.

Thanks to @simondel for merging the other recent PRs.

@bartekleon
Copy link
Member

bartekleon commented Dec 2, 2019

Looks good to me. One question /.thing to check: is "lodash.deepClone" dependency required after these changes? It could.be removed from package.json if it's not.

@brodybits
Copy link
Author

I just did a clean rebase and removed lodash.clonedeep from dependencies in packages/javascript-mutator/package.json.

@bartekleon
Copy link
Member

And it still works. So 1 dependency less 🚀

@brodybits brodybits changed the title JavaScript mutator improvements [WIP] JavaScript mutator improvements Dec 4, 2019
@brodybits
Copy link
Author

Back to [WIP] again, since it is now expected to have a conflict with PR #1912 and I would like to break the implementation of JavaScriptMutator.mutate() down into smaller parts again (someday).

The one thing is that I find it to be a bit of a waste if I would have to keep working around conflicts due to a slow merge cycle. I am also thinking that if we can unify the JavaScript and TypeScript mutators, as discussed in #1893, it could help reduce the efforts needed for enhancements such as PR #1912 and custom external mutators.

@bartekleon
Copy link
Member

@brodybits, I was thinking, if some of the changes introduced here, could be merged into my PR, especially from this file:
packages/javascript-mutator/src/JavaScriptMutator.ts

And I was also looking at your code, and I think, there might be an issue since you are not a coping object in some cases. It can lead to some problems with references. (But I am not sure).

@brodybits
Copy link
Author

@kmdrGroch I would downvote that idea at this point since I think these 2 PRs are really doing different things and should be mostly orthogonal. I think it would be ideal if we could somehow improve the internal interfaces to keep these 2 PRs completely independent.

I think a quicker solution would be to merge this PR now so that you can continue without dealing with a conflict in the future.

@bartekleon
Copy link
Member

I rather only meant this part where you consolidate generateMutants and mutate function. But well, yea, it makes sense too

@brodybits brodybits changed the title [WIP] JavaScript mutator improvements JavaScript mutator improvements Dec 6, 2019
@brodybits
Copy link
Author

Got it. I would (still) favor getting this one merged first, for the sake of maintaining more linear progress.

@bartekleon
Copy link
Member

@simondel could you take a look? I guess this PR is ready to be checked and merged

@simondel
Copy link
Member

simondel commented Dec 8, 2019

Hi! Thanks for making these changes :)

I'm reviewing this and I'll also do a diff check between a report from the current master branch and your branch to see if anything other than the arrow mutator has changed.

@simondel simondel merged commit 898d38b into stryker-mutator:master Dec 11, 2019
@brodybits brodybits deleted the js-mutator-improvements branch February 12, 2020 14:55
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

3 participants