Don't mutate declare types

[JoshuaKGoldberg]

Summary

If you have a .d.ts file that includes a declare module "foo" { }, the module name ("foo") will be mutated during tests.

Mutant survived!
C:\Code\Project\src\typings\b2a.d.ts:3:15
Mutator: StringLiteral
-   declare module "b2a" {
+   declare module "" {

Stryker config

module.exports = function(config) {
  config.set({
    coverageAnalysis: "off",
    files: [
      "tsconfig.test.json",
      {
        included: false,
        pattern: "test/unit/jest.config.js",
        transpiled: false,
      },
      "src/**/*.ts",
      "src/**/*.tsx",
    ],
    jest: {
      config: require("./test/unit/jest.config.js"),
    },
    mutate: [
      "src/**/*.ts",
      "!src/**/*.test.ts",
    ],
    mutator: "typescript",
    reporter: ["clear-text", "progress"],
    testRunner: "jest",
    transpilers: ["typescript"],
    tsconfigFile: "tsconfig.json",
  });
};

Stryker environment

+-- stryker@0.19.3
+-- stryker-api@0.13.0

Your Environment

software	version(s)
node	9.8.0
npm	5.6.0
Operating System	Windows 10

[nicojs]

@JoshuaKGoldberg thanks for reporting this issue.

Just to be sure: you do want to mutate .d.ts files? If not, you can just ignore that by adding "!src/**/*.d.ts" in your mutate array.

[JoshuaKGoldberg]

That's a great point - I should have done that.

[nicojs]

Yeah, but you're point still is valid. It doesn't make sense to mutate declare statements 👍

[simondel]

We decided we will not fix this for now due to the fact that it can be fixed in your case by removing the .d.ts files from the mutate array.

If someone in the future has this same issue, but with regular ts files. Please ask us to reopen this issue!

[JoshuaKGoldberg]

Just tried to set up Stryker again and hit this issue 😄. The default stryker.config.js has mutate: ["src/**/*.ts"]. I think we should add "!src/**/*.d.ts".

[ankhzet]

Possibly related (type decl-s are mutated): stryker mutates boolean constants in type decls, which produces false-negatives:

class A {
    methodB(): SomeType | false { // 'false' here is mutated to 'true'

     }
}

Haven't tested, but have suspicion, that string literal typedefs would be also mutated (const foo: Type | 'enum1' | 'enum2' = ...).
Also haven't tested boolean literals in interfaces.

config:

module.exports = function(config) {
  config.set({
    files: [
      '**/*.json',
      'src/**/*.ts?(x)',
      'tests/**/*',
    ],
    mutate: ['**/compiler/**/*.ts', '!**/*Spec.ts'],
    mutator: 'typescript',
    testFramework: 'jasmine',
    testRunner: 'jasmine',
    reporters: ['progress', 'clear-text', 'html'],
    // coverageAnalysis: 'perTest',
    transpilers: [
      'typescript'
    ],
    tsconfigFile: 'tsconfig.json',
    jasmineConfigFile: './jasmine.json',
  });
};

Possible solutions:
a) when walking source's AST, omit any kinds of type declarations entirely
b) mutate sources with stripped type information (akin to babel's 'strip-flow-types' plugin behavior)

[nicojs]

@ankhzet we're thinking behind the scenes about a "TypeCheckerPlugin". That way, we'd be able to validate mutants better. I.e.

class A {
    methodB(): SomeType | false { // 'false' here is mutated to 'true'
     }
}

When mutating false to true, it would be caught by the type checker and marked as CompileError. If it doesn't result in a CompileError, you're probably missing a test that uses the specific return type.

Could this solve your issue?

[ankhzet]

@nicojs checked mutations with some variations of interfaces/implementations.

It seems, like:

1. sometimes mutations of typedefs (uncaught by typechecker) are useful indeed 👍 They point to the unreasonably wide types/insufficiently test-covered code areas.
2. sometimes, when uncaught mutants cause false-positives (can't be killed by valid testset), they can be killed by enforcing more strict types (thus forcing CompileError), which by addition also improves type strictness of the code in some situations
3. ... but if wider (and they intended to be wide/permissive) type constraints are mutated, it would produce false-positives, even on properly test-covered cases (AFAIKT). F.e., this mutant in a type-guard cannot be killed by a valid test (or i just can't figure it out due to my insufficient experience with mutation-testing, and it is certainly confusing):

// src
// 'false' here is mutated (also, '' isn't, for some reason), and causes false-negative ('true' argument is considered in 'should accept Truthy arguments' spec)
export const isFalsy = (param?: any): param is never | null | undefined | false | '' => { 
    return !param;
};

// spec

describe('TypeDecl mutation', () => {
    describe('isFalsy', () => {
      it('should accept Falsy arguments and return "true"', () => {
        expect(isFalsy('')).toBeTruthy();
        expect(isFalsy(false)).toBeTruthy();
        expect(isFalsy(null)).toBeTruthy();
        expect(isFalsy(undefined)).toBeTruthy();
        expect(isFalsy()).toBeTruthy();
      });

      it('should accept Truthy arguments and return "false"', () => {
        expect(isFalsy('str')).toBeFalsy();
        expect(isFalsy(true)).toBeFalsy();
        expect(isFalsy({})).toBeFalsy();
        expect(isFalsy(1)).toBeFalsy();
      });
    });
});

If this is indeed a false-positive situation, I have no idea if it could be handled automatically though.

Is there a way in stryker to disable mutations on certain lines/statements?
Something like // eslint-disable-next-line <rule-name>.

[nicojs]

If this is indeed a false-positive situation, I have no idea if it could be handled automatically though.

Indeed, I understand what you're saying. This test will kill the mutant, but I don't think we should force people to write it:

const foo = true;
if (isFalsy(foo)) {
  (function (foo: never) { })(foo);
}

We might need to skip declare and type definitions entirely to prevent this and recommend other tools to test the quality of your type definitions. What do you think @simondel? We can also make it configurable.

Is there a way in stryker to disable mutations on certain lines/statements?
Something like // eslint-disable-next-line .

No not a.t.m.

[simondel]

I don't think we should change type definitions directly. 👍

[nicojs]

Ok, I'm reopening the issue. Anyone up for the task?

I think the best way is to filter out the declare nodes and halt mutating in this method here:

https://github.com/stryker-mutator/stryker/blob/13a10581ce4771e966c6110bb8af3f79644075e4/packages/typescript/src/TypescriptMutator.ts#L32-L39

[JoshuaKGoldberg]

👋 I can probably take a look this week! Any tips would be appreciated 😀

[bartekleon]

I do believe it can be closed :)

[JoshuaKGoldberg]

Ha, good catch @kmdrGroch. Thanks 😄