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

Warn when Mutator in directive does not exist #3842

Conversation

Giovds
Copy link
Contributor

@Giovds Giovds commented Nov 5, 2022

Closes #3812

When a directive references an unknown Mutator, log a warning that this directive is not used.

Mutated code

const compare = (a, b) => {
  // Stryker disable HelloThere
  // Stryker disable next-line Equality: Wrong Mutator name
  return a - b >= 0 ? 1 : -1;
}

module.exports = { compare };

module.exports = { compare };

Cli
image

@Giovds
Copy link
Contributor Author

Giovds commented Nov 5, 2022

A couple of questions/issues I still have:

  1. I've made an assumption here: https://github.com/stryker-mutator/stryker-js/pull/3842/files#diff-87b4b61b8bd5767c34cf6b141cc3721b722a730e884627e7adc127c8449a07b4R79. Is this correct?
  2. It probably is confusing that the ignored mutant in the HTML view shows a diff on the comment but has no 'addition line'.
  3. It seems that the line number is off by 1. There is a toBabelLineNumber() method which adds 1 to the line number. Should this be used to re-convert the lines to their original numbers?

@nicojs Thoughts?

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.

It is pretty clever to see how you report mutants for unused stryker directives, but I don't think it's a good idea :/. Mutants are mutations in the code, not unused directives.

I think logging them as a warning is good enough for now. For example, users might want Stryker to fail fast instead of showing them in the report 🤷‍♀️.

@Giovds Giovds marked this pull request as ready for review November 10, 2022 00:34
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.

Looking good! 🤩 , still some stuff left.

@Giovds Giovds force-pushed the issues-3812-warn-user-when-disabled-mutator-does-not-exist branch from 02131ba to 8a78d88 Compare November 13, 2022 19:31
@Giovds Giovds force-pushed the issues-3812-warn-user-when-disabled-mutator-does-not-exist branch from 8a78d88 to cff5c8c Compare November 13, 2022 19:32
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.

This looks awesome; just a couple of remarks left. Let me know if you need more help.

@nicojs nicojs merged commit fe79d49 into stryker-mutator:master Nov 22, 2022
@nicojs
Copy link
Member

nicojs commented Nov 22, 2022

Thanks! 🎉

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.

Stryker Equality ignore once not working
2 participants