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

Improve handling of frozen VsTest runs #1977

Merged
merged 14 commits into from
Apr 11, 2022

Conversation

dupdob
Copy link
Member

@dupdob dupdob commented Mar 25, 2022

  • Add Stryker controlled timeout for VsTest to detect frozen VsTest runs and retry them. Fix test runner frozen after test discovery. May fix: Stryker freezes with too many concurrent test runners #1665 , Stryker hangs when using the multi test project feature #1136
  • Fix a bug causing the loss of runner due to early disposing
  • Fix existing VsTest recycling logic which was not working (post initial PR)
  • Restore integration tests in the pipeline (added)
  • Detect when VsTest fails to start and stop the execution altogether (as nothing can be done) (added)
  • Reduce the computation time of test runs via fixing and improving the existing optimization logic
  • Perform minor refactoring (cleaning of the no longer needed RunAllmethod)

@dupdob dupdob force-pushed the improve_handling_of_frozen_vstest branch from da27a28 to 1e27d48 Compare March 25, 2022 09:10
@dupdob dupdob force-pushed the improve_handling_of_frozen_vstest branch from 1e27d48 to bf69d1c Compare March 25, 2022 09:49
@eNeRGy164
Copy link
Contributor

Something is really broken in this branch as almost no test coverage is found anymore.

Running on FluentAssertions with the following config file:

{
    "stryker-config":
    {
        "project": "Src\\FluentAssertions\\FluentAssertions.csproj",
        "test-projects": [
            "Tests\\FluentAssertions.Specs\\FluentAssertions.Specs.csproj",
            "Tests\\FluentAssertions.Equivalency.Specs\\FluentAssertions.Equivalency.Specs.csproj"
        ],
        "reporters": [
            "progress",
            "html"
        ],
        "verbosity": "info",
        "concurrency": 4,
        "mutation-level": "Complete"
    }
}

Running with 1.4.2

[19:25:16 INF] 242 mutants got status CompileError. Reason: Mutant caused compile errors
[19:25:16 INF] 666 mutants got status NoCoverage. Reason: Mutant has no test coverage
[19:25:16 INF] 1 mutants got status Ignored. Reason: Unable to inject back mutations in source code.
[19:25:16 INF] 909 total mutants are skipped for the above mentioned reasons
[19:25:16 INF] 8873 total mutants will be tested

Running with 1.5.0

[19:30:30 INF] 242 mutants got status CompileError. Reason: Mutant caused compile errors
[19:30:30 INF] 666 mutants got status NoCoverage. Reason: Mutant has no test coverage
[19:30:30 INF] 1 mutants got status Ignored. Reason: Unable to inject back mutations in source code.
[19:30:30 INF] 909 total mutants are skipped for the above mentioned reasons
[19:30:30 INF] 8873 total mutants will be tested

Running with this branch

[12:30:14 INF] 244 mutants got status CompileError. Reason: Mutant caused compile errors
[12:30:14 INF] 9529 mutants got status NoCoverage. Reason: Mutant has no test coverage
[12:30:14 INF] 1 mutants got status Ignored. Reason: Unable to inject back mutations in source code.
[12:30:14 INF] 9774 total mutants are skipped for the above mentioned reasons
[12:30:14 INF] 8 total mutants will be tested

With only 8 tests, it is fast 😉 but not what I expected

@dupdob
Copy link
Member Author

dupdob commented Mar 26, 2022

Thanks for the feedback. It is not clear how the changes could have this effect.
I tried to reproduce it with FluentAssertions solution, but It does not build (yet?) on my machine.

@dupdob
Copy link
Member Author

dupdob commented Mar 27, 2022

Could you provide some logs ? I always checked my PRs against NFluent (4K mutants, 1.7K tests) and it works normally.
I did another check using multiple test projects to have a similar configuration and everything works as expected.

It looks like only one of the test projects was taken into account here. Anyway, I do not think your issue relates to this PR (at this stage).

@eNeRGy164
Copy link
Contributor

Thanks for the feedback. It is not clear how the changes could have this effect. I tried to reproduce it with FluentAssertions solution, but It does not build (yet?) on my machine.

To build everything running build.ps1/.sh should be sufficient. But is has some dependencies like the .NET 4.7 SDK, the .NET 6 SDK, including UWP.

@eNeRGy164
Copy link
Contributor

Could you provide some logs ? I always checked my PRs against NFluent (4K mutants, 1.7K tests) and it works normally. I did another check using multiple test projects to have a similar configuration and everything works as expected.

It looks like only one of the test projects was taken into account here. Anyway, I do not think your issue relates to this PR (at this stage).

I am comparing the master branch versus this branch, so the differences are only related to this PR, I think?

I'm running the tests again to get logs.

@dupdob
Copy link
Member Author

dupdob commented Mar 28, 2022

I pushed a new commit with the fix (obviously) as well as some added logic to detect similar condition.
This kind of issue should have been detected by integration tests. I will have a look in the coming days to understand why nothing was detected.

@dupdob dupdob force-pushed the improve_handling_of_frozen_vstest branch from 1375d2f to 4e9ca67 Compare March 28, 2022 21:56
@dupdob dupdob force-pushed the improve_handling_of_frozen_vstest branch 2 times, most recently from 3380fbc to f3ce357 Compare March 29, 2022 09:10
@dupdob dupdob force-pushed the improve_handling_of_frozen_vstest branch from f3ce357 to 8eb3459 Compare March 29, 2022 09:25
@dupdob dupdob force-pushed the improve_handling_of_frozen_vstest branch from 53179ca to 290f20a Compare March 29, 2022 10:31
@dupdob
Copy link
Member Author

dupdob commented Mar 29, 2022

I discovered that integration testing was no longer effective (incorrect filtering syntax in the pipeline). I have restored them and re established the score base lines (roughly twice as much mutations as before).
Also did some cleanup on the code on an opportunity basis.

@dupdob
Copy link
Member Author

dupdob commented Mar 29, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rouke-broersma
Copy link
Member

I discovered that integration testing was no longer effective (incorrect filtering syntax in the pipeline). I have restored them and re established the score base lines (roughly twice as much mutations as before). Also did some cleanup on the code on an opportunity basis.

That is... 😶

@eNeRGy164
Copy link
Contributor

Isn't this becoming a bit of scope creep?

…tor.cs

Co-authored-by: Rouke Broersma <mobrockers@gmail.com>
…ool.cs

Co-authored-by: Rouke Broersma <mobrockers@gmail.com>
@dupdob
Copy link
Member Author

dupdob commented Mar 30, 2022

Isn't this becoming a bit of scope creep?
What do you feel is scope creep here ? restoring the integration tests is part of the CI and is (obviously) top priority, especially since we could have released a faulty version if you have not tried it on your own (thanks again, btw)

@eNeRGy164
Copy link
Contributor

What do you feel is scope creep here ? restoring the integration tests is part of the CI and is (obviously) top priority, especially since we could have released a faulty version if you have not tried it on your own (thanks again, btw)

Yes, I understand the priority, but it's not introduced by this change, and like you discovered, already present for a while.
But now this PR is no longer only about VSTest anymore. Meanwhile people having issues with time-outs will be held back on a fix for something that's not affecting them.

Personally I would start fixing the integration tests in a separate PR, resulting also in a separate release note, etc.
But maybe that's just how I'm used to do PR's.

It's more of a warning, in the end, I'm just a consumer, not a maintainer, just trying to be helpful that you don't get a big ball of mud in changes that are hard to release until the next thing is fixed.

dupdob added 2 commits March 31, 2022 09:47
…m/stryker-mutator/stryker-net into improve_handling_of_frozen_vstest

# Conflicts:
#	src/Stryker.DataCollector/Stryker.DataCollector/CoverageCollector.cs
@dupdob
Copy link
Member Author

dupdob commented Mar 31, 2022

PR is ready for merge (fixed the problem with the FullFramework test project using an obsolete framework version).

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@rouke-broersma rouke-broersma merged commit 1d3310b into master Apr 11, 2022
@rouke-broersma rouke-broersma deleted the improve_handling_of_frozen_vstest branch April 11, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stryker freezes with too many concurrent test runners
3 participants