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

Track different sorts of timeouts differently #1145

Open
sanmai opened this issue Mar 9, 2020 · 1 comment
Open

Track different sorts of timeouts differently #1145

sanmai opened this issue Mar 9, 2020 · 1 comment

Comments

@sanmai
Copy link
Member

sanmai commented Mar 9, 2020

Is your feature request related to a problem? Please describe.

It is a lot said these days about "timeout" mutations.

  • Some say that if Infection changes the code for something that makes it time out, it's not something a consumer should be bother with.
  • Others might say that timeout mutations are testable and should be grouped together with other escapee. There's even a proposal to make this happed with a switch: Add config option to count timeouts as mutant escapes #1133.

I gave it a long though, and, after swinging back and forth, I left with a mixed feeling about accepting or rejecting either approach. They're both correct, depending on circumstances.

The problem here is that there are different timeout mutations with different causes.

  • Sometimes tests are just too long, and CI machine is busy, so a test can naturally time out not because it wouldn't succeed, but because Infection isn't configured to wait that long. A user can hardly do anything about these. Let's call them type A. For example, I've seen this happen may times when testing Psalm. (There is a way to work around these, but let's think like there isn't for now.)

  • Sometimes code really goes bonkers, goes onto rampage in an infinite loop, and such. This is a different situation. In my experience I was able to isolate and test these mutations, sometimes by simply mocking a producer. It is unfair to group these "timeouts" with others since these are worthy of actionable feedback to the user. These are actual test defects Infection promised to help find. Let's call them type B.

And they are. I you don't know what you're looking at, and you propose to raise a timeout, you might be making the problem worse. On the contrary, if you know that these are type A mutations, it is correct to propose to raise timeout.

Describe the solution you'd like.

I'd like Infection to discern between different types of timeouts. Actual test-originated type A timeouts may be left alone, where code-originated timeout type B can be grouped as runaway mutations, or as R in the report. Runaways in a sense that they not just escaped, but went really long way across the imaginable country doing their worst deeds. Think Bonnie and Clyde, but mutated code.

Or they might be tracked as your typical escaped mutation. In my eyes this will also solve the root problem of not giving a proper actionable feedback about this class of mutations.

Not to say this knowledge should be ignored in case of type A timeouts: Infection may offer to increase the value for the timeout setting, provided it is clear it would help.

Ways to do it.

We can do as, say, Travis CI does to detect stuck jobs: by tracking output. If there's no output for a specified period of time, it is a runaway. Yes, a broken test may output something themselves, but there's --disallow-test-output to make PHPUnit fail gracefully.

It may worth exploiting timeout tracking capabilities PHPUnit offers with --enforce-time-limit and --default-time-limit=N, although it may be better to instruct a consumer to routinely apply these during testing. There's also --disallow-resource-usage but it may not work with untagged tests.

If we'd go deep into processes territory, it should be possible to know exactly how much a process uses, setting some limits. PHPUnit does that somehow.

Fixing this problem fixes #1133. Going forward with #1134 will offer only a temporary relief.

@maks-rafalko
Copy link
Member

I like how these 2 groups are split, makes sense. Explaining this to our users is a nice thing as well.

Didn't dig into all these options of PHPUnit and have no idea how possible it is to implement, but in general idea of introducing R and differentiate these timeouts sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants