Skip to content

NUnit bugfixes#36

Open
mbartelsm wants to merge 68 commits intoFaultify:mainfrom
mbartelsm:main
Open

NUnit bugfixes#36
mbartelsm wants to merge 68 commits intoFaultify:mainfrom
mbartelsm:main

Conversation

@mbartelsm
Copy link
Copy Markdown

No description provided.

cdadashovkhandan and others added 25 commits March 17, 2021 22:05
Implement rudimentary duplication, fix NUnit "in use" bug
Remove initial duplication pool, implement test project duplication b…
…xcessive test duplication before coverage analysis.

Co-Authored-By: JFaber151 <[email protected]>
Copy link
Copy Markdown
Member

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I see various good improvements. Also, I see various things that are changed with a future note that it is going to be fixed. When we merge this PR we will likely break the dotnet/xunit support? I do not mind this to be broken for a bit in master while I do worry wether it will be fixed before the end of the project. What are the ideas from your side on this? I am happy to merge this if the comments are resolved, various good improvements have been done which is nice.

Comment thread Faultify.Cli/Program.cs Outdated
ITestHostRunFactory testHost = settings.TestHost switch
{
_ => new DotnetTestHostRunnerFactory() // TODO: Use Faultify.TestRunner.XUnit/NUnit for in memory testing.
_ => new NUnitTestHostRunnerFactory() // TODO: Use Faultify.TestRunner.XUnit/NUnit for in memory testing.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand NUnit is the focus for now. Let's keep in mind this is enabled currently, only when XUNIT is working we can do a release. Ideally, we want dotnet test to remain working. Does it work currently? Perhaps we can put it as default sucht that faultify works for everyone using the master branch version of faultify.

"commandName": "Project",
"commandLineArgs": "-t E:\\programming\\stryker-net\\src\\Stryker.Core\\Stryker.Core.UnitTest\\Stryker.Core.UnitTest.csproj -p 9"
"commandName": "Project",
"commandLineArgs": "-t C:\\Users\\Vxvl4\\source\\repos\\Faultify\\Benchmark\\Faultify.Benchmark.NUnit\\Faultify.Benchmark.NUnit.csproj -f pdf"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we need to figure a way out to have a relative path here instead of an absolute path. Not mandoatory


_fileStream = new FileStream(FullFilePath(), FileMode.Open, FileAccess.Read, FileShare.ReadWrite);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be removed?

}

testProject.FreeTestProject();
testProject.FreeTestProject(); //TODO: replace with deletion
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
catch (Exception e)
{
Console.WriteLine(e.Message);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we want to use the logger instead. Would this be possible?


public List<TestProjectDuplication> MakeInitialCopies(IProjectInfo testProject, int count)
/// <summary>
/// Create the very first duplication of the project that will be used for coverage calculations
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Create the very first duplication of the project that will be used for coverage calculations
/// Creates the very first duplication of the project that will be used for coverage calculations.

I usually use a plural beginning for public methods. Just for the sake of consistency.



/// <summary>
/// Delete the test project completely
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Delete the test project completely
/// Deletes the test project completely.

}

/// <summary>
/// Create a copy based on the initial duplication's data
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Create a copy based on the initial duplication's data
/// Creates a copy based on the initial duplication's data

Copy link
Copy Markdown
Member

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why did you switch from build in dotnet core log to NLog ? If for static reference reasons we can easily introduce a wrapper such as:
   internal static class ApplicationLogging
    {
        internal static ILoggerFactory LoggerFactory { get; set; }// = new LoggerFactory();
        internal static ILogger CreateLogger<T>() => LoggerFactory.CreateLogger<T>();        
        internal static ILogger CreateLogger(string categoryName) => LoggerFactory.CreateLogger(categoryName);

    }

to facilitate the static reference part. The static part is I think an improvement from the current implementation. Staying with build in Microsoft libraries has my preference.

  • I see the open PR is growing in size 2K lines of changes. This one becomes un reviewable. Is there something we can do to split it up in smaller chunks or is it to late for that? 200 lines is recommended on average

}
catch (Exception e)
{
_logger.Fatal(e, "Fatal exception reading file");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_logger.Fatal(e, "Fatal exception reading file");
_logger.Fatal(e, "Fatal exception reading mutation coverage file.");

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.

4 participants