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

Beta: Mutations are creating invalid code #2469

Closed
gramster opened this issue Sep 9, 2020 · 16 comments · Fixed by #2481
Closed

Beta: Mutations are creating invalid code #2469

gramster opened this issue Sep 9, 2020 · 16 comments · Fixed by #2481
Labels
🐛 Bug Something isn't working
Milestone

Comments

@gramster
Copy link

gramster commented Sep 9, 2020

Summary

The mutations that are being created have strange errors in them.

Unterminated Template Literals

Below is the tail end of a file are being mutated:

          const url = __global_69fa48.__activeMutant__ === 706 ? `` : (__global_69fa48.__coverMutant__(706), `https:
          this.browserService.launch(url);
        }
        break;
    }
  }

}

Note that the \https:` template is unterminated, so when this is compiled by TypeScript compiler you get the error:

error TS1160: Unterminated template literal.

Unterminated String Literal

Tail end of another file:

  protected getExcludedFiles(): string[] {
    switch (__global_69fa48.__activeMutant__) {
      case 1086:
        {}
        break;

      default:
        __global_69fa48.__coverMutant__(1086);
        {
          const list: string[] = __global_69fa48.__activeMutant__ === 1087 ? [] : (__global_69fa48.__coverMutant__(1087), [__global_69fa48.__activeMutant__ === 1088 ? "" : (__global_69fa48.__coverMutant__(1088), '**/Libsite-packages

It's just truncated at that point, and so the compiler error is:

error TS1002: Unterminated string literal.

Original unmutated code:

   protected getExcludedFiles(): string[] {
        const list: string[] = ['**/Lib/**', '**/site-packages/**'];
        this.getVsCodeExcludeSection('search.exclude', list);
        this.getVsCodeExcludeSection('files.exclude', list);
        this.getVsCodeExcludeSection('files.watcherExclude', list);
        this.getPythonExcludeSection(list);
        return list;
    }

(There's another 60 lines of code after that I left out that was just dropped from the mutated file).

')' Expected

Another tail end:

  private openPythonExtensionPage() {
    switch (__global_69fa48.__activeMutant__) {
      case 11468:
        {}
        break;

      default:
        __global_69fa48.__coverMutant__(11468);
        {
          env.openExternal(Uri.parse(__global_69fa48.__activeMutant__ === 11469 ? `` : (__global_69fa48.__coverMutant__(11469), `https:
        }
        break;
    }
  }

}

Again, the \https` is unterminated, but that leads to the parentheses being unmatched too, which is the compiler error reported here.

Original unmutated code:

    private openPythonExtensionPage() {
        env.openExternal(Uri.parse(`https://marketplace.visualstudio.com/items?itemName=ms-python.python`));
    }
}

There's a bunch more of these but the above are illustrative.

Stryker config

{
  "$schema": "./node_modules/@stryker-mutator/core/schema/stryker-schema.json",
  "packageManager": "npm",
  "reporters": [
    "html",
    "clear-text",
    "progress"
  ],
  "testRunner": "mocha",
  "buildCommand": "npx gulp prePublishNonBundle",
  "mutate": ["src/client/**/*.ts"],
  "coverageAnalysis": "perTest"
}

Test runner config

Stryker environment

➜  vscode-python-me git:(stryker) ✗ npm ls | grep stryker
├─┬ UNMET PEER DEPENDENCY @stryker-mutator/core@4.0.0-beta.4
│ ├─┬ @stryker-mutator/api@4.0.0-beta.4
│ ├─┬ @stryker-mutator/instrumenter@4.0.0-beta.4
│ │ ├─┬ @stryker-mutator/api@4.0.0-beta.4
│ │ ├── @stryker-mutator/util@4.0.0-beta.4 deduped
│ ├─┬ @stryker-mutator/util@4.0.0-beta.4
├── @stryker-mutator/javascript-mutator@4.0.0-beta.4
├─┬ @stryker-mutator/mocha-framework@4.0.0-beta.4
│ ├─┬ @stryker-mutator/api@4.0.0-beta.4
│ └── @stryker-mutator/util@4.0.0-beta.4 deduped
├─┬ @stryker-mutator/mocha-runner@4.0.0-beta.4
│ ├─┬ @stryker-mutator/api@4.0.0-beta.4
│ ├── @stryker-mutator/util@4.0.0-beta.4 deduped
├── @stryker-mutator/typescript@4.0.0-beta.4
├─┬ @stryker-mutator/typescript-checker@4.0.0-beta.4
│ ├─┬ @stryker-mutator/api@4.0.0-beta.4
│ ├── @stryker-mutator/util@4.0.0-beta.4 deduped
├─┬ @stryker-mutator/webpack-transpiler@3.3.1
│ ├─┬ @stryker-mutator/api@3.3.1
+-- mocha@x.x.x
+-- jest@x.x.x
+-- karma@x.x.x
+-- angular-cli@x.x.x
+-- react-scripts@x.x.x

Test runner environment

# Test command

Your Environment

software version(s)
node 12.18.0
npm 6.14.4
Operating System MacOS Catalina

Add stryker.log

@gramster gramster added the 🐛 Bug Something isn't working label Sep 9, 2020
@nicojs
Copy link
Member

nicojs commented Sep 9, 2020

Thanks a lot @gramster ! Will have a look tomorrow

@nicojs nicojs added this to the 4.0 milestone Sep 9, 2020
@nicojs
Copy link
Member

nicojs commented Sep 9, 2020

Update: I just read #2466 and that triggered me. I think this issue is already fixed, but we didn't release a new beta version. I just released v4.0.0-beta.5. Could you please try it again?

Sorry for the work you put in, we should be better about updating our beta's 😢

It was fixed with #2438

EDIT:

I've added some more instructions in #2466

@gramster
Copy link
Author

gramster commented Sep 9, 2020

No worries, I am very excited about getting this to work as it solves a big problem for me around measuring the effectiveness of our test suites :-), and you have been very responsive. I'll update and try again.

@gramster
Copy link
Author

gramster commented Sep 9, 2020

Looks like I am now past the build step! The next thing I need to figure out is how to customize the mocha command.

@gramster
Copy link
Author

I got somewhat further. I had to remove a bunch of tests that failed in the initial test run, but now I get:

20:56:48 (72658) ERROR DryRunExecutor Initial test run timed out!
20:56:48 (72658) ERROR Stryker an error occurred Error: Something went wrong in the initial test run
    at DryRunExecutor.validateResultCompleted (/Users/grwheele/repos/vscode-python-me/node_modules/@stryker-mutator/core/src/process/3-DryRunExecutor.js:67:15)
    at DryRunExecutor.execute (/Users/grwheele/repos/vscode-python-me/node_modules/@stryker-mutator/core/src/process/3-DryRunExecutor.js:39:14)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Stryker.runMutationTest (/Users/grwheele/repos/vscode-python-me/node_modules/@stryker-mutator/core/src/Stryker.js:35:49)

and I have no idea what tests I should remove to get past this (there are several thousand tests).

@nicojs
Copy link
Member

nicojs commented Sep 10, 2020

Well... the initial test run should pass if your normal test run also passes. Instrumented code without an activated mutant should of course run the same way as non-instrumented code would.

There can be a number of things that are problematic. The best way to debug this is to kill the process during the initial test run, cd into the sandbox directory cd .stryker-tmp/sandbox1234 and then run the tests yourself.

I can also give it a try. Is this the repo? https://github.com/Microsoft/vscode-python

EDIT: Found your remarks in the other issue, see that that's the repo :)

@nicojs
Copy link
Member

nicojs commented Sep 10, 2020

Alternatively, you can run with --logLevel trace or --fileLogLevel trace to see the stdout output from the child process (the test runner process).

@nicojs
Copy link
Member

nicojs commented Sep 10, 2020

I've taken a look. Nice bug! 🐛

This:

switch (item.severity) {
    case DiagnosticSeverity.Error: {
        traceError(message);
        this.outputChannel.appendLine(message);
        break;
    }
    // ...
    default: {
        traceInfo(message);
    }
}

Is instrumented as this:

switch (item.severity) {
    case DiagnosticSeverity.Error:
    {
        switch (__global_69fa48.__activeMutant__) {
        case 1775:
            {}
            break;

        default:
            __global_69fa48.__coverMutant__(1775);
            {
            traceError(message);
            this.outputChannel.appendLine(message);
            break;
            }
            break;
        }
    }

    // ... 

    default:
    {
        switch (__global_69fa48.__activeMutant__) {
        case 1779:
            {}
            break;

        default:
            __global_69fa48.__coverMutant__(1779);
            {
            traceInfo(message);
            }
            break;
        }
    }
}

Which... is obviously wrong. The inner break won't break out of the case it was designed to break out of.

@hugo-vrijswijk I know you use the same case statement syntax in Stryker4s. How do you handle these kinds of mutant placements?

EDIT: For reference, stryker config I used:

{
    "$schema": "./node_modules/@stryker-mutator/core/schema/stryker-schema.json",
    "buildCommand": "tsc -b",
    "mochaOptions": {
        "config": "./build/.mocha.unittests.js.json"
    },
    "testRunner": "mocha"
}

@hugo-vrijswijk
Copy link
Member

I know you use the same case statement syntax in Stryker4s. How do you handle these kinds of mutant placements?

@nicojs In Scala you don't have to 'break' from pattern matches, because they are already scoped and there's no fall-through. So there's no issue with a break going off in the wrong statement.

For placing mutations, a single mutation-switch pattern match is created 'above' any other pattern-matching. So this:

list.nonEmpty match {
  case true => "nonEmpty"
  case _    => otherValue
}

Would have 3 mutations (.nonEmpty -> .isEmpty, true -> false and "nonEmpty" -> "") and is mutated to:

_root_.scala.sys.props.get("ACTIVE_MUTATION") match {
  case Some("0") =>
    list.isEmpty match {
      case true => "nonEmpty"
      case _    => otherValue
    }
  case Some("1") =>
    list.nonEmpty match {
      case false => "nonEmpty"
      case _     => otherValue
    }
  case Some("2") =>
    list.nonEmpty match {
      case true => ""
      case _    => otherValue
    }
  case _ =>
    list.nonEmpty match {
      case true => "nonEmpty"
      case _    => otherValue
    }
}

@gramster
Copy link
Author

Thanks for diving into this, Nico! Really appreciate it. Just BTW, I am wanting to use Stryker because I think our test suite has a lot of anti-patterns in it, especially over-use of mocking, and want to come up with metrics for a baseline that we can improve. I'd be interested in knowing of over metrics you may be aware of. Right now, I am thinking of these:

  • code coverage (obviously not very useful just on its own)
  • the mutation index score from Stryker (I'm thinking of this as "test effectiveness")
  • time taken to run the tests (this is one where mocking can improve the score, so I want metrics in tension with that)
  • "test efficiency" - I'm kind of inventing here, but was thinking of looking at the ratio of lines of real code covered by a test to lines of test code covered by a test. Are we able to write concise tests that cover a good amount of real code? This is one where I expect the overuse of mocking will hurt the metric. This seems like something Stryker could compute too if it was useful.

@nicojs
Copy link
Member

nicojs commented Sep 11, 2020

@hugo-vrijswijk thanks for pitching in. So scala doesn't have a break statement, this is interesting... and such a good idea! 👍

Since JS does have a break statement, I think the easiest thing to do is to use an if-statement instead of a switch ... case. This is quite a big change, so it might cost me a day or 2 to implement.

BTW, I am wanting to use Stryker because I think our test suite has a lot of anti-patterns in it [...]

Yeah, I saw some of them already at a first glance (for example Math.random() in tests. Made me scratch my head 😉) .

the mutation index score from Stryker (I'm thinking of this as "test effectiveness")

Yep. Definitely "test effectiveness", that's what I call it as well. It might be difficult to get Stryker running on the entire test suite since it takes a very long time.

Worst case scenario: Tmutation-testing = Ttest run * n.
Where n = number of mutants. Per test coverage analysis + running in parallel will speed that up considerably, but still a big time. You might want to think about sharding your mutation runs. This is what we do for Stryker, see for example https://github.com/stryker-mutator/stryker/actions/runs/228545052

Why don't you join our slack, so we can discuss the other points further? Always nice to talk about tests 😍 join slack

nicojs added a commit that referenced this issue Sep 16, 2020
Place mutants in a statement with a `if` statement instead of a `switch` statement.

Fixes #2469
nicojs added a commit that referenced this issue Sep 17, 2020
Place mutants in a statement with an `if` statement instead of a `switch` statement.

Fixes #2469
@nicojs
Copy link
Member

nicojs commented Sep 17, 2020

@gramster the issue with case switch statements is fixed. However, I still run into an issue during the dry run of the vscode-python, this is because #2474, I'll be fixing that next

@nicojs
Copy link
Member

nicojs commented Sep 17, 2020

Ok after I fixed #2474, I ran into #2485 . After I fixed that I managed to start a run properly:

nicojs@nicoj03 ~/vscode-python (main)$ npx stryker run
14:48:35 (8032) INFO ConfigReader Using stryker.conf.json
14:48:36 (8032) INFO InputFileResolver Found 1041 of 2380 file(s) to be mutated.
14:49:04 (8032) INFO Instrumenter Instrumented 1041 source file(s) with 72054 mutant(s)
14:49:05 (8032) INFO ConcurrencyTokenProvider Creating 7 test runner process(es).
14:49:06 (8032) INFO Sandbox Running build command "tsc -b" in the sandbox at "/home/nicojs/vscode-python/.stryker-tmp/sandbox9640422".
14:50:50 (8032) INFO DryRunExecutor Starting initial test run. This may take a while.
14:52:41 (8032) INFO DryRunExecutor Initial test run succeeded. Ran 8441 tests in 4 minutes 6 seconds (net 84892 ms, overhead 8504 ms).
Mutation testing  [                                                  ] 0% (elapsed: ~1m, remaining: ~5h 14m) 92/24878 tested (7 survived, 8 timed out)

Used this config:

{                                                                                   
    "$schema": "./node_modules/@stryker-mutator/core/schema/stryker-schema.json",   
    "buildCommand": "tsc -b",                                                       
    "mochaOptions": {                                                               
        "config": "./build/.mocha.unittests.js.json"                                
    },                                                                              
    "testRunner": "mocha",                                                          
    "coverageAnalysis": "perTest"
}                                                                                   

Hope you have some decent hardware to run this on since it will take a couple of hours. Or run mutation testing on a part of the code.

I will release a new version of the beta and report back here.

@nicojs
Copy link
Member

nicojs commented Sep 17, 2020

Just released 4.0.0-beta.7. Happy mutation testing @gramster 😁

@gramster
Copy link
Author

gramster commented Sep 17, 2020 via email

@nicojs
Copy link
Member

nicojs commented Sep 17, 2020

And thank you for the bug reports. 🐛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants