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

Remove race condition during testruns #135

Closed
richardwerkman opened this issue Oct 2, 2018 · 4 comments
Closed

Remove race condition during testruns #135

richardwerkman opened this issue Oct 2, 2018 · 4 comments
Labels
🐛 Bug Something isn't working hacktoberfest https://hacktoberfest.digitalocean.com

Comments

@richardwerkman
Copy link
Member

The new feature parallel testruns caused race conditions to occur during mutation testruns, as seen in #122. Changing the testrunner interface and implementation should fix this.

Currently the method SetActiveMutation() sets a property inside the testrunner instance. This property is used during the RunAll() method call to start an sub-process with the id from the property in the environment of the sub-process.

If the property changed in between the calls SetActiveMutation() and RunAll() the sub-process will use the wrong id in it's environment.

The interface

    public interface ITestRunner
    {
        TestRunResult RunAll(int? timeoutMS);
        void SetActiveMutation(int? id);
    }

Should become

    public interface ITestRunner
    {
        TestRunResult RunAll(int? timeoutMS, int? activeMutationId);
    }

The DotnetTestRunner implementation should be updated and the unit tests as well.

@richardwerkman richardwerkman added 🐛 Bug Something isn't working to do hacktoberfest https://hacktoberfest.digitalocean.com labels Oct 2, 2018
@Valdas3
Copy link
Contributor

Valdas3 commented Oct 2, 2018

I can start working on these changes.

@richardwerkman
Copy link
Member Author

Hey @Valdas3 thanks in advance for your help! Feel free to find a suitable solution to fix the race conditions. The above is just my option :)

@shadow-cs had another suggestion as to create a new testrunner instance for each mutant/thread. That would work as wel and maybe be more future proof.

@Valdas3
Copy link
Contributor

Valdas3 commented Oct 3, 2018

I have created an initial pull request with the updated signature of RunAll. Testing locally, this seems to have fixed the issue with the test runner, so at least that's good. Next step would be to create a new test runner instance for each "iteration" of Parallel.Foreach. I guess we should avoid new'ing up MutationTestExecutor and instead create something like a MutationTestExecutorFactory with accompanying interface?

Also, I noticed a small inconsistency in the order which results are displayed when tests are run. The
_reporter.OnMutantTested(mutant); executes as soon as individual test is completed which might result in a different order from what gets shown when _reporter.OnAllMutantsTested(_input.ProjectInfo.ProjectContents); is executed. Here is an example of what I mean:
image
SS...S is displayed where it would make a bit more sense to display ..SS.S to keep the same order as in the next lines with score calculations. Not sure if it is an actual issue but fixing this could a nice-to-have task for someone to look at during the hacktoberfest :)

@simondel
Copy link
Member

simondel commented Oct 4, 2018

@Valdas3 Thanks for fixing this and testing it! Could you make a separate issue for the order in which reporter events are handled? It shouldn't be this way :)

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

No branches or pull requests

3 participants