Stryker: Ignore Function With Comment

[guiyllw]

Feature: Ignore Function With Comment

Add a comment that ignores a statement from being mutated, added to the coverage and considered in mutation score.

Something like:

/* stryker ignore next */

Based on instanbul ignore code from coverage

/* istanbul ignore next */

---

The Problem

I use unit testing for lambda functions, for example, and there are some helper functions that I'm not interested in testing like a Logger or a Wrapper I created for a library.

For semantic reasons I don't want to move this functions to another file and ignore the file, because I should not have to alter my project structure just to ignore some functions.

I also have some deprecated functions that precedes our quality policies.

Example:

/**
 * @deprecated
 */
function myDeprecatedFunction() { ... }

// deprecated
function myOlderVersionDeprecatedFunction() { ... }

And I not interested in mutating or testing this functions, but I need to keep them in my project for compatibility reasons, and this lower my mutation score to a level thats is not accepted by QA team and CI tool in addition to not reflecting the actual quality of the code.

So if I this feature was implemented I will be able to do somenthing like:

/**
 * @deprecated
 */
/* stryker ignore next */
function myDeprecatedFunction() { ... }

// deprecated
/* stryker ignore next */
function myOlderVersionDeprecatedFunction() { ... }

[legopiraat]

Hi, This is an interesting way to exclude mutators!

I understand the reason some wrappers or loggers are not tested. But can you give some more background information on why the deprecated functions are not being tested?

@nicojs what do you think of this suggestion?
Would this also be a good way to exclude methods that give a lot of false positives?

(Was looking into some ways to exclude mutations for Stryker4s :) )

[vitfgarcia]

In relation to the deprecated functions, we recently started rewriting functions with testing.
The project with this functions is used on other projects as a dependency, so we can't simply delete all old functions.
Since the rewriting process is slow, we wanna be able to evaluate the quality of the new functions without the old ones bringing the mutation score down and without breaking the semantic organization of the project.
Because we implemented pipelines with git, the score is important so the CI process works correctly.
By the way, me and @guilherme-vasconcellos opened this issue together

[nicojs]

@guilherme-vasconcellos thanks for opening this issue!

But in general, I think it is a bad idea to use these hidden-to-the-public tactics of increase mutation score / code coverage. It's like saying:

We have a 100% code coverage... except for the code that isn't covered

However, I see where you are coming from. And I agree that, when using it carefully and sparsely, it can be helpful in some cases (you make pretty good arguments in your issue).

The technical way of implementing this by letting the 2 mutator plugins skip over the next statement as soon as they see the /* stryker-mutator ignore next */ comment nodes in the AST. Should not be too hard to do I think.

I'd be willing to except a PR that adds this functionality.

@legopiraat

Would this also be a good way to exclude methods that give a lot of false positives?

What false positives?

I understand the reason some wrappers or loggers are not tested.

This would be a bad case of using /* stryker-mutator ignore next /* in my opinion. If a log statement isn't tested, its not tested. Don't sweep that under a rug.

[legopiraat]

The false positives am talking about is for stryker4s specifically, for example, the indexOf and lastIndexOf. When applied to a list with all unique values they would always produce a false positive mutation.

Maybe something in the line of /* stryker-mutator ignore: lastIndexOf /* would be a nice way to exclude fine-grained mutators on a function.

[simondel]

We don't have plans to support this in the near future, so we're closing this issue for now.

[sswales]

I would probably recommend that it's approached in the following way:

confg

module.exports = function (config) {
  config.set({
    files: [
      'src/**/*',
      '!test/**/*',
    ],
    mutate: [
      'src/code1/**/*.js',
      'src/code2/**/*.js',
      '!src/common/**/*',
      '!src/**/config.js',
      '!src/**/*.test.js',
      '!src/**/*.json',
      '!src/**/*.ejs',
      '!src/**/__mocks__/*',
      '!test/**/*',
    ],
    timeoutMs: 10000,
    mutator: { name: 'javascript', excludedMutations: ['StringLiteral'] },
    packageManager: 'npm',
    reporters: ['html', 'clear-text', 'progress', 'dashboard'],
    testRunner: 'jest',
    transpilers: [],
    coverageAnalysis: 'off',
    jest: { config: require('./.jestrc.unit.json'), enableFindRelatedTests: true },
    htmlReporter: { baseDir: 'reports/mutation' },
    thresholds: { high: 80, low: 50, break: 40 },
  });
};

So in my example, the lines I would like to be ignored are the following:

 logger.info({ id, message }, 'Validating message');
 logger.info({ id }, 'Valid');

I would recommend adding to the config as everything else is maintained here for Stryker...

    ignore: [
      'logger.info',
      'logger.warn',
    ],

Or for the example at the beginning of the conversation...

    ignore: [
      'myDeprecatedFunction',
      'myOlderVersionDeprecatedFunction',
    ],

So the idea being Stryker before mutating can check if the line contains any values within the list of 'ignore'. If it DOES then DONT mutate the line.

[nicojs]

@shaunswales thanks for tuning in. Would you be against comments like /* stryker ignore next */? This is our preferred implementation for now.

Would that work for you? Ignore it from Stryker configuration in an ignore array could be confusing for the end users. It would also not be clear what exactly is ignored (it is less "in your face" while coding).

Like @simondel stated, we don't have plans to support this in the coming months. We first want to move to mutation switching

[sswales]

Thanks for the reply @nicojs, more than happy to share my thoughts as our company plan on using this tool moving forward in all of our repositories. So having direct contact with you is great, as you can see we have a lot of code bases to maintain and we are committed to try use your tooling.

I'm not against the idea of the comments...

The reason I was proposing it being used in the config was to make it more central and keep the tests clear of unneeded comments. It wouldn't have to be called 'ignore' could be 'excludeFunctionsStartingWith' or anything you deem acceptable.

I guess the idea is, the functionality to stop these lines of code being run is the MVP, then refining the process is the second stage.