Exclude mutations that are over specified time limit

[sanmai]

Looking at Psalm's median testing time (#1153) of about 10 minutes, and looking at the anticipated number of timed out mutations for these circumstances, I can't help but notice that since we know about how much time a mutation needs to be tested thoroughly, we can exclude these definitely out -of-time-budget mutations from even running.

This change considerably speeds up MT for projects with incoherent test coverage, while also lowering detection rates (which is bad), while leaving the door open either to raise the timeout, or to factor in @covers annotations (which is good).

For Psalm I get 6m 17s. of runtime with -j2 on a complete test suite. Before it was unfeasible to run.

Psalm results for @default set

{
    "timeout": 15,
    "source": {
        "directories": [
            "src"
        ]
    },
    "logs": {
        "text": "build\/infection.log"
    },
    "mutators": {
        "@default": true
    }
}

.: killed, M: escaped, U: uncovered, E: fatal error, T: timed out, S: skipped

...SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSMSSSSSSSSSSSSSS   (   50)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS   (  100)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS   (  150)

...

4441 mutations were generated:
      47 mutants were killed
       0 mutants were not covered by tests
      18 covered mutants were not detected
       0 errors were encountered
      79 time outs were encountered
    4297 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 87%
         Mutation Code Coverage: 100%
         Covered Code MSI: 87%

Please note that some mutants will inevitably be harmless (i.e. false positives).

Time: 6m 16s. Memory: 1.28GB

This PR:

• Adds time limit limiting
• Blocked by U for uncovered #1233
• Covered by tests
• Doc PR: Update usage.md to mention skipped mutations site#167 (just a mention)

Related: #1145, #1133

[tests-checker]

Thank you for contributing to Infection. We noticed you didn't add any tests. Could you please add them to make sure everything works as expected?

[theofidry]

How do you think those should be reported?

I've been thinking for a while that we might need a DetectionStatus::SKIPPED in which case providing a reason would be appropriate as to why it was skipped. This would allow us to report it properly to the users in the logs

[sanmai]

I don't think it makes sense to report them differently in the console report (T is fine), but having them as a different number in the final report is a good idea, UX-wise.

[sanmai]

@maks-rafalko what's your take on this idea?

[sanmai]

We can report these time-constrained mutations as, naturally, C for constrained.

[maks-rafalko]

In general, it makes sense. Let me ask you a couple of question to build the whole picture.

1. What's the final goal of this PR? As I understand, for big projects this change reduces the number of mutation processes executed, which speeds up MT considerably (good) by not running (read killing) some of the mutants (bad). Should the user of Infection treat it like a call to action? To:
   1.1 add @covers annotations to reduce the number of tests covering particular line (am I right here?)
   1.2 to think about timeout value and clearly understand how much time can particular mutation take

   If the above is true and we will decide to go with this PR, I think we need to add this information somewhere in the output to instruct the user what to do with all these C mutants

2. We can report these time-constrained mutations as, naturally, C for constrained.

   I vote for this. It distinguishes general T mutants and C ones, which IMO have completely different meaning. T is considered as Killed, while C is not, because tests weren't even executed.

3. but having them as a different number in the final report is a good idea

   👍

[theofidry]

Also not 100% sure to understand the relationship with the configurable timeout or how it should be

[sanmai]

Should the user of Infection treat it like a call to action?

Most definitely. We'll report these C mutations mentioning the configured timeout:

.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out, C: constrained

CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC   (   50)
CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC   (  100)
CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC   (  150)
...

4441 mutations were generated:
      47 mutants were killed
       0 mutants were not covered by tests
      18 covered mutants were not detected
       0 errors were encountered
      79 time outs were encountered
    4297 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 87%
         Mutation Code Coverage: 100%
         Covered Code MSI: 87%

...

Time: 6m 20s. Memory: 1.28GB

So that a user would (hopefully) see these mutations were constrained by the timeout they set. Next they either raise a timeout, or be directed to the docs for further instruction.

Also we won't include these constrained/"ignored" mutations in the MSI: this will make the final metric more meaningful: we assume these constrained mutations have same average effect on the resulting metrics. Conversely, if we are to include these mutations in the uncovered part, or in covered but uncaught, the metrics will be skewed to either side, giving wrong impression (you'll get either 98% or 2% MSI, which are both far from truth).

Note this above was a run over Psalm, and it took only modest 6 minutes. It's like a breath of fresh air even after all changes we added up to 0.16.

[sanmai]

Also not 100% sure to understand the relationship with the configurable timeout or how it should be

This is the user choice. If they know they have several very long tests, they can sacrifice some mutations by shortening time needed for the MT process.

E.g. if one has a 15 second long integration test, setting time limit to 10 second will effectively exclude all mutations covered by the test. Now one has a choice of either raising the timeout, or tagging this test with @coversNothing.

[sanmai]

@maks-rafalko the build is green

[maks-rafalko]

Thanks, looks good to me.

The only one thing that we need to discuss is about what users should do with all those S mutants.

As I said here, users will 100% want to know what are Skipped mutants are and what to do with them

I think we need to add this information somewhere in the output to instruct the user what to do with all these S mutants

Also, many of the recommendations from your comment look related to the documentation needs to be added to support this PR.

So, from the code point of view: good to be merged from my side. But I think we still need to add a very descriptive documentation about what to do with S mutants, to avoid such issues like #1143 It is not clear what to do with MSI for new users

[sanmai]

We're mentioning this new class of mutations here in the final report:

4441 mutations were generated:
...
    4297 mutants required more time than configured

You're right that docs would need more than just a mention. How do you think if we'd get back at this closer to the release time?

Another place where this new feature should be mentioned is the configuration master. It does not have a step to configure the timeout, where it really could. And there we can explain what's the deal with timeouts in great detail. I think I can get to this next thing.

[maks-rafalko]

How do you think if we'd get back at this closer to the release time?

Sounds good. Will need your help though ;)

Another place where this new feature should be mentioned is the configuration master.

this is also a good idea.

Thank you, @sanmai.

[DaveLiddament]

What is the unit for this time, seconds, milliseconds, etc?

Suggest:

1. update name to nominalTimeInSecondsToTest or whatever unit actually is.
2. or better still create a value object Seconds, 'MiliSeconds` to aid clarity here.

[sanmai]

It's all seconds here.