Find unit test assemblies whose names differ from the project directory name#58194
Find unit test assemblies whose names differ from the project directory name#58194RikkiGibson merged 6 commits intodotnet:mainfrom
Conversation
src/Tools/Source/RunTests/Program.cs
Outdated
| { | ||
| list.Add((filePath, targetFramework)); | ||
| } | ||
| else if (Directory.Exists(fileContainingDirectory) && Directory.GetFiles(fileContainingDirectory, searchPattern: "*.UnitTests.dll") is { Length: not 0 } matches) |
There was a problem hiding this comment.
This in effect allows the build to have multiple assemblies in an output folder matching the pattern *.UnitTests.dll, only if one of those assemblies matches the project folder name. When the name doesn't match exactly we disallow multiple.
There wasn't a specific reason to disallow multiple unit test assemblies in an output folder except that I wanted to reduce the likelihood of accidentally including the same assembly multiple times e.g. via a project reference.
There was a problem hiding this comment.
Worth adding this comment in the code?
| throw new IOException(message); | ||
| } | ||
| list.Add((matches[0], targetFramework)); | ||
| } |
There was a problem hiding this comment.
Should there be some "else" block here to flag anything else that's missing? It seems false positives could be filtered by updating the exclude rules earlier?
There was a problem hiding this comment.
I'll probably experiment with this in future PRs. Primarily want to just plug the gap for now.
src/Tools/Source/RunTests/Program.cs
Outdated
| if (matches.Length > 1) | ||
| { | ||
| var message = $"Multiple unit test assemblies found in '{fileContainingDirectory}'. Please adjust the build to prevent this. Matches:{Environment.NewLine}{string.Join(Environment.NewLine, matches)}"; | ||
| throw new IOException(message); |
There was a problem hiding this comment.
| throw new IOException(message); | |
| throw new Exception(message); |
In general throw Exception except for argument validation
src/Tools/Source/RunTests/Program.cs
Outdated
| { | ||
| list.Add((filePath, targetFramework)); | ||
| } | ||
| else if (Directory.Exists(fileContainingDirectory) && Directory.GetFiles(fileContainingDirectory, searchPattern: "*.UnitTests.dll") is { Length: not 0 } matches) |
There was a problem hiding this comment.
| else if (Directory.Exists(fileContainingDirectory) && Directory.GetFiles(fileContainingDirectory, searchPattern: "*.UnitTests.dll") is { Length: not 0 } matches) | |
| else if (Directory.Exists(fileContainingDirectory) && Directory.GetFiles(fileContainingDirectory, searchPattern: "*.UnitTests.dll") is { Length: > 0 } matches) |
crazy nit
|
Thanks for looking into the issue. It seems that |
jmarolf
left a comment
There was a problem hiding this comment.
Generally fine with the changes here. but let's file a follow issue to figure out how to add something to build correctness to fail if we have test assemblies that we never run. Not an easy problem to solve but its clear we should try.
Yeah, I'm trying to make sense of all this. Some of it may have come down to my artifacts directory being stale, but I am finding a disturbing amount of variation between CI and local runs of Test.cmd, as well as local runs using the vscode launch task. |
src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ResultPropertiesTests.cs
Outdated
Show resolved
Hide resolved
…ropertiesTests.cs
|
There's further investigation to do here but I'm going to merge this PR because I think it moves us in the right direction. |
Resolves #58030
Some failing tests seemed to have non-trivial underlying causes. Filed #58198 to fix those tests.
@jaredpar @dotnet/roslyn-compiler @dotnet/roslyn-infrastructure for review.