Conversation
# Conflicts: # src/Stryker.Core/Stryker.Core/Initialisation/InputFileResolver.cs
…nsibility to new class 'ProjectAnalyzerContext' (WIP)
# Conflicts: # src/Stryker.TestRunner.MicrosoftTestPlatform.UnitTest/MicrosoftTestPlatformRunnerPoolTests.cs
There was a problem hiding this comment.
Pull request overview
Refactors project analysis/initialisation to streamline Buildalyzer usage and improve how Stryker identifies mutable projects + their associated test projects (especially across solution/project modes and target frameworks).
Changes:
- Introduces
TargetsForMutation,ProjectAnalyzerContext, and mutable project/target tracking types to centralize analysis state and framework/platform selection. - Refactors
InputFileResolverto use the new analysis context and to map test projects to source projects via assembly/project references. - Updates solution/platform handling (“AnyCPU” vs “Any CPU”), logging behavior, and adjusts unit tests accordingly.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Stryker.Utilities/Buildalyzer/IAnalyzerResultExtensions.cs | Adds helper extensions for validating overall analysis results across frameworks. |
| src/Stryker.TestRunner.MicrosoftTestPlatform.UnitTest/MicrosoftTestPlatformRunnerPoolTests.cs | Makes runner creation synchronization explicit to reduce flakiness in disposal tests. |
| src/Stryker.Solutions/SolutionFile.cs | Adjusts solution/platform normalization and adds per-project configuration lookup helper. |
| src/Stryker.Solutions.Test/SolutionFileShould.cs | Updates assertions for normalized solution platform naming (“Any CPU”). |
| src/Stryker.Core/Stryker.Core/ProjectComponents/SourceProjects/SourceProjectInfo.cs | Replaces SolutionInfo with TargetsForMutation on source project metadata. |
| src/Stryker.Core/Stryker.Core/Initialisation/TargetsForMutation.cs | New: encapsulates chosen config/platform, selected projects, and restore-at-solution-level behavior. |
| src/Stryker.Core/Stryker.Core/Initialisation/ProjectAnalyzerContext.cs | New: wraps Buildalyzer project analysis, framework selection, and detailed logging. |
| src/Stryker.Core/Stryker.Core/Initialisation/MutableProjectTree.cs | New: tracks a project’s candidate targets and their validity for mutation. |
| src/Stryker.Core/Stryker.Core/Initialisation/MutableProjectTarget.cs | New: links a mutable target to the test-project analysis results that cover it. |
| src/Stryker.Core/Stryker.Core/Initialisation/InputFileResolver.cs | Major refactor: drives analysis via contexts and builds a test↔source mapping using the new tree types. |
| src/Stryker.Core/Stryker.Core/Initialisation/InitialisationProcess.cs | Switches build step to read config/platform from TargetsForMutation. |
| src/Stryker.Core/Stryker.Core.UnitTest/Mutators/CollectionExpressionMutatorTests.cs | Whitespace-only test fixture cleanup. |
| src/Stryker.Core/Stryker.Core.UnitTest/Initialisation/ProjectOrchestratorTests.cs | Updates mocks/verifications to assert EnvironmentOptions passed into Buildalyzer builds. |
| src/Stryker.Core/Stryker.Core.UnitTest/Initialisation/InputFileResolverTests.cs | Updates expected framework selection behavior and Buildalyzer invocation assertions. |
| src/Stryker.CLI/Stryker.CLI/Logging/LoggingInitializer.cs | Tweaks diagnostic-mode console log level selection (at least Debug). |
| src/.run/FullFrameworkApp.Test.run.xml | Updates IDE run configuration working directory. |
Comments suppressed due to low confidence (1)
src/Stryker.Core/Stryker.Core/Initialisation/InputFileResolver.cs:499
- ArgumentNullException is constructed with
pathas the parameter name, but it should usenameof(path)so tooling reports the correct parameter name regardless of runtime value.
private string FindProjectFile(string path)
{
if (string.IsNullOrEmpty(path))
{
throw new ArgumentNullException(path, "Project path cannot be null or empty.");
}
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| <File Path="..\docs\technical-reference\fsharp\classes.md" /> | ||
| <File Path="..\docs\technical-reference\fsharp\current-state.md" /> | ||
| <File Path="..\docs\technical-reference\fsharp\packages.md" /> | ||
| <File Path="..\docs\technical-reference\fsharp\_category_.yml" /> | ||
| <File Path="..\docs\technical-reference\introduction.md" /> | ||
| <File Path="..\docs\technical-reference\mutant-schemata.md" /> | ||
| <File Path="..\docs\technical-reference\Mutation Orchestration Design.md" /> | ||
| <File Path="..\docs\technical-reference\Project Analysis step.md" /> | ||
| <File Path="..\docs\technical-reference\project-components.md" /> | ||
| <File Path="..\docs\technical-reference\research.md" /> | ||
| <File Path="..\docs\technical-reference\testing-framework.md" /> | ||
| <File Path="..\docs\technical-reference\VsTest internal.md" /> | ||
| <File Path="..\docs\technical-reference\_category_.yml" /> |
There was a problem hiding this comment.
The newly added <File Path> entries use Windows-style ..\ backslashes while existing entries use ../ forward slashes. .slnx files are used cross-platform; inconsistent separators can break tooling on non-Windows. Consider switching these paths to ../docs/... with forward slashes for consistency and portability.
| var rootComponent = AddRootFolderIfMultiProject(_mutationTestProcesses.Select(x => x.Input.SourceProjectInfo.ProjectContents).ToList(), options); | ||
| var combinedTestProjectsInfo = _mutationTestProcesses.Select(mtp => mtp.Input.TestProjectsInfo).Aggregate((a, b) => (TestProjectsInfo)a + (TestProjectsInfo)b); | ||
| var combinedTestProjectsInfo = _mutationTestProcesses.Select(mtp => mtp.Input.SourceProjectInfo.TestProjectsInfo).Aggregate((a, b) => (TestProjectsInfo)a + (TestProjectsInfo)b); | ||
|
|
There was a problem hiding this comment.
SourceProjectInfo.TestProjectsInfo is nullable, but it’s aggregated and cast to TestProjectsInfo without null checks. If any process has a null TestProjectsInfo, this will throw at runtime (either from Aggregate or the cast). Either enforce non-null TestProjectsInfo in SourceProjectInfo, or filter/guard here with a clear error when it’s missing.
|
|
||
| // Enrich test projects info with unit tests | ||
| EnrichTestProjectsWithTestInfo(input.InitialTestRun, input.TestProjectsInfo); | ||
| EnrichTestProjectsWithTestInfo(input.InitialTestRun, input.SourceProjectInfo.TestProjectsInfo); | ||
|
|
There was a problem hiding this comment.
EnrichTestProjectsWithTestInfo requires a non-null ITestProjectsInfo, but this call passes input.SourceProjectInfo.TestProjectsInfo which is nullable. If it’s null, this will throw during initialization. Either make TestProjectsInfo non-nullable on SourceProjectInfo (preferred) or guard here before enriching.
| public void SelectAllProjects() => _selectedProjects = Solution?.GetProjects(Configuration, Platform).ToList() ?? []; | ||
|
|
||
| // the method is at solution level because it needs only be called once | ||
| internal void RestoreSolution(IAnalyzerResults results) | ||
| { | ||
| lock (_selectedProjects) | ||
| { |
There was a problem hiding this comment.
RestoreSolution/BuildSolution lock on _selectedProjects, but _selectedProjects is reassigned in SelectAllProjects() and is also exposed publicly (SelectedProjects). Locking on a mutable/replaced object can make the lock ineffective and lead to concurrent solution restore/build being executed multiple times. Use a dedicated private readonly lock object (or SemaphoreSlim) and avoid exposing/replacing the lock target.
| public class SourceProjectInfo(IAnalyzerResult analyzerResult, ITestProjectsInfo testProjectsInfo) | ||
| : IProjectAndTests | ||
| { | ||
| private readonly List<string> _warnings = []; | ||
|
|
||
| public Action OnProjectBuilt { get; set; } | ||
|
|
||
| public SolutionInfo SolutionInfo { get; set; } | ||
| public IAnalyzerResult AnalyzerResult { get; init; } = analyzerResult; | ||
|
|
||
| public IAnalyzerResult AnalyzerResult { get; init; } | ||
| public ITestProjectsInfo? TestProjectsInfo { get; init; } = testProjectsInfo; | ||
|
|
||
| /// <summary> | ||
| /// The Folder/File structure found in the project under test. | ||
| /// </summary> | ||
| public IReadOnlyProjectComponent ProjectContents { get; set; } | ||
|
|
||
| public bool IsFullFramework => AnalyzerResult.TargetsFullFramework(); | ||
|
|
||
| public string HelperNamespace => CodeInjector.HelperNamespace; | ||
|
|
||
| public CodeInjection CodeInjector { get; } = new(); | ||
|
|
||
| public ITestProjectsInfo TestProjectsInfo { get; set; } | ||
|
|
||
| public IReadOnlyCollection<string> Warnings => _warnings; | ||
|
|
||
| public IReadOnlyList<string> GetTestAssemblies() => | ||
| TestProjectsInfo.GetTestAssemblies(); | ||
| TestProjectsInfo?.GetTestAssemblies() ?? []; |
There was a problem hiding this comment.
SourceProjectInfo implements IProjectAndTests, whose TestProjectsInfo contract is non-nullable, but the implementation makes it nullable (ITestProjectsInfo?) and uses null-conditional access. Multiple call sites (e.g., building and reporting) dereference TestProjectsInfo without null checks, so allowing null here can lead to runtime NullReferenceExceptions. Prefer making TestProjectsInfo non-nullable and enforcing it via the constructor (use an empty/null-object implementation when needed).
| return true; | ||
| } | ||
| var testProjects = SourceProjectInfos.SelectMany(p => p.TestProjectsInfo.AnalyzerResults) | ||
| .Distinct().ToList(); | ||
| for (var i = 0; i < testProjects.Count; i++) |
There was a problem hiding this comment.
BuildTestProjects dereferences p.TestProjectsInfo without a null check, but SourceProjectInfo.TestProjectsInfo is currently nullable. If any SourceProjectInfo is created without test info (which is possible with the current constructor/property types), this will throw at runtime. Either enforce non-null TestProjectsInfo on SourceProjectInfo or handle nulls here explicitly.
|
| private ProjectsTracker BuildTracker(IStrykerOptions options, SolutionFile? solution) => | ||
| new(solution, options, _analyzerProvider, _nugetRestoreProcess, FileSystem, _logger) | ||
| { TargetFramework = options.TargetFramework }; | ||
|
|
There was a problem hiding this comment.
BuildTracker accepts SolutionFile? solution, but ProjectsTracker currently requires a non-null SolutionFile in its constructor. This creates a nullability contract mismatch (and warnings) because callers legitimately pass null (e.g., when no solution is provided or loading fails). Consider updating ProjectsTracker to accept SolutionFile? and handle null explicitly.
| private ProjectsTracker BuildTracker(IStrykerOptions options, SolutionFile? solution) => | |
| new(solution, options, _analyzerProvider, _nugetRestoreProcess, FileSystem, _logger) | |
| { TargetFramework = options.TargetFramework }; | |
| /// <summary> | |
| /// Creates the project tracker used to analyze source and test projects. | |
| /// </summary> | |
| /// <param name="options">Stryker options that determine how project discovery should run.</param> | |
| /// <param name="solution">The loaded solution when available; can be <see langword="null"/> outside solution mode.</param> | |
| /// <returns>A configured <see cref="ProjectsTracker"/> instance.</returns> | |
| /// <exception cref="InputException">Thrown when solution mode is requested but no solution could be loaded.</exception> | |
| private ProjectsTracker BuildTracker(IStrykerOptions options, SolutionFile? solution) | |
| { | |
| if (options.IsSolutionContext && solution is null) | |
| { | |
| throw new InputException("Solution mode requires a valid solution file, but no solution could be loaded."); | |
| } | |
| return new(solution!, options, _analyzerProvider, _nugetRestoreProcess, FileSystem, _logger) | |
| { | |
| TargetFramework = options.TargetFramework | |
| }; | |
| } |
| public ProjectsTracker(SolutionFile file, IStrykerOptions options, IBuildalyzerProvider buildalyzerProvider, | ||
| INugetRestoreProcess nugetRestoreProcess, IFileSystem fileSystem, ILogger logger) | ||
| { | ||
| _options = options ?? throw new ArgumentNullException(nameof(options)); | ||
| _buildalyzerProvider = buildalyzerProvider ?? throw new ArgumentNullException(nameof(buildalyzerProvider)); | ||
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| _nugetRestoreProcess = nugetRestoreProcess ?? throw new ArgumentNullException(nameof(nugetRestoreProcess)); | ||
| _fileSystem = fileSystem; | ||
| Solution = file; | ||
| SelectConfiguration(); | ||
| } | ||
|
|
||
| private SolutionFile? Solution { get; } |
There was a problem hiding this comment.
ProjectsTracker stores the solution in a nullable property (SolutionFile? Solution) and call sites pass null, but the constructor currently requires a non-null SolutionFile. This mismatch leads to nullable warnings and makes it unclear whether null is supported. Make the constructor parameter nullable (or throw if null is not allowed).
| private string Configuration { get; set; } | ||
|
|
||
| private string Platform { get; set; } | ||
|
|
||
| public string? TargetFramework { get; init; } | ||
|
|
||
| public int ProjectCount => _selectedProjects.Count; | ||
|
|
||
| public List<string> SelectedProjects => _selectedProjects; | ||
|
|
||
| public void AddProjects(IEnumerable<string> projectFiles) => _selectedProjects.AddRange(projectFiles); | ||
|
|
||
| private void SelectConfiguration() | ||
| { | ||
| var configuration = _options.Configuration; | ||
| var platform = _options.Platform; | ||
| if (Solution != null) | ||
| { | ||
| // we use the solution to determine the configuration and platform to use, as the project files may not contain all configurations and platforms that are defined in the solution | ||
| (Configuration, Platform) = Solution.GetMatching(configuration, platform); | ||
| if ((!string.IsNullOrEmpty(configuration) && configuration != Configuration) || | ||
| (!string.IsNullOrEmpty(platform) && platform != Platform)) | ||
| { | ||
| _logger.LogWarning("Using solution configuration/platform '{ActualBuildType}|{ActualPlatform}' instead of requested '{Configuration}|{Platform}'.", | ||
| Configuration, Platform, configuration, platform); | ||
| } | ||
| else | ||
| { | ||
| _logger.LogInformation("Using solution configuration/platform '{Configuration}|{Platform}'.", Configuration, Platform); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| Configuration = configuration; | ||
| // "Any CPU" is default platform at solution level, but in project files it is "AnyCPU", so we need to convert it to match the project files | ||
| Platform = platform == "Any CPU" ? "AnyCPU" : platform; | ||
| _logger.LogInformation("Using project configuration/platform '{Configuration}|{Platform}'.", Configuration??"`default`", Platform ?? "`default`"); | ||
| } |
There was a problem hiding this comment.
Configuration and Platform are declared as non-nullable strings, but they are populated from options that can be unset (and are used with null-coalescing in logging). This is another nullable contract mismatch and can hide incorrect assumptions. Consider making these string? (and handling defaults explicitly) or ensuring they’re never null after SelectConfiguration().
|
|
||
| var projectInfo = Mock.Of<SourceProjectInfo>(); | ||
| projectInfo.ProjectContents = folder; | ||
| var info = new SourceProjectInfo(null, new TestProjectsInfo(null)) { ProjectContents = folder }; |
There was a problem hiding this comment.
This test constructs SourceProjectInfo with a null IAnalyzerResult and a TestProjectsInfo created with a null filesystem. With nullable enabled, this introduces warnings and will break if SourceProjectInfo starts validating inputs (which would be reasonable given its non-nullable API). Prefer using a minimal IAnalyzerResult mock (via TestHelper.SetupProjectAnalyzerResult) and a real/mock IFileSystem instance.
| var info = new SourceProjectInfo(null, new TestProjectsInfo(null)) { ProjectContents = folder }; | |
| var analyzerResultMock = new Mock<IAnalyzerResult>(MockBehavior.Strict); | |
| var testProjectsInfo = new TestProjectsInfo(new MockFileSystem()); | |
| var info = new SourceProjectInfo(analyzerResultMock.Object, testProjectsInfo) { ProjectContents = folder }; |
| var mutationTestInput = new MutationTestInput() | ||
| { | ||
| SourceProjectInfo = new SourceProjectInfo() | ||
| SourceProjectInfo = new SourceProjectInfo(null, null) | ||
| { | ||
| ProjectContents = folder | ||
| }, |
There was a problem hiding this comment.
This test passes null for both IAnalyzerResult and ITestProjectsInfo into SourceProjectInfo, even though the constructor’s parameters are non-nullable. This undermines nullable correctness and risks runtime NREs if any code path touches AnalyzerResult/TestProjectsInfo. Prefer supplying minimal non-null test doubles instead of null (or update the production API to be explicitly nullable).
| public ProjectsTracker(SolutionFile file, IStrykerOptions options, IBuildalyzerProvider buildalyzerProvider, | ||
| INugetRestoreProcess nugetRestoreProcess, IFileSystem fileSystem, ILogger logger) | ||
| { | ||
| _options = options ?? throw new ArgumentNullException(nameof(options)); | ||
| _buildalyzerProvider = buildalyzerProvider ?? throw new ArgumentNullException(nameof(buildalyzerProvider)); | ||
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| _nugetRestoreProcess = nugetRestoreProcess ?? throw new ArgumentNullException(nameof(nugetRestoreProcess)); | ||
| _fileSystem = fileSystem; | ||
| Solution = file; | ||
| SelectConfiguration(); |
There was a problem hiding this comment.
ProjectsTracker is constructed with solution potentially being null (e.g., BuildTracker(options, null)), but the constructor parameter is SolutionFile (non-nullable). Either accept SolutionFile? in the constructor or avoid passing null; otherwise this creates nullable warnings and makes the null-handling contract unclear.
| var rootComponent = AddRootFolderIfMultiProject(_mutationTestProcesses.Select(x => x.Input.SourceProjectInfo.ProjectContents).ToList(), options); | ||
| var combinedTestProjectsInfo = _mutationTestProcesses.Select(mtp => mtp.Input.TestProjectsInfo).Aggregate((a, b) => (TestProjectsInfo)a + (TestProjectsInfo)b); | ||
| var combinedTestProjectsInfo = _mutationTestProcesses.Select(mtp => mtp.Input.SourceProjectInfo.TestProjectsInfo). | ||
| Aggregate(new TestProjectsInfo(new FileSystem(), _logger), (a, b) => a + b); |
There was a problem hiding this comment.
combinedTestProjectsInfo is seeded with new TestProjectsInfo(new FileSystem(), _logger), which forces a real filesystem even if the run is using an injected/mock IFileSystem elsewhere. This can make unit/integration testing harder and can cause unexpected disk access. Prefer aggregating starting from an existing TestProjectsInfo instance (or pass an IFileSystem into StrykerRunner) instead of instantiating FileSystem here.
| public RelatedSourceProjectsInfo ResolveSourceProjectInfos(IStrykerOptions options) | ||
| { | ||
| var normalizedProjectUnderTestNameFilter = NormalizePath(options.SourceProjectName); | ||
|
|
There was a problem hiding this comment.
With nullable enabled, ResolveSourceProjectInfos assigns null to a local declared as non-nullable (SolutionFile solution; ... solution = null;). Consider changing the local to SolutionFile? (and updating dependent code accordingly) so the “no solution provided” case is explicit and doesn’t violate nullability contracts.
Massive refactor of initialization/project discovery/project analysis phase.
So that