fix: resolve build warnings in solution#5386
Conversation
- Remove net6.0 target from TUnit.TestProject.Library (EOL, packages no longer support it) - Suppress MSB3277 in source generator test projects (benign netstandard2.0 assembly version conflicts) - Suppress NU1701 in TUnit.SourceGenerator.Benchmarks (Microsoft.Build only ships .NET Framework binaries) - Suppress NU1504 in template content Directory.Build.props (duplicate TUnit.Assertions.FSharp from auto-import) - Fix trailing whitespace in F# template fsproj files
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Code Review
This is a clean, focused housekeeping PR that brings the solution down from 18 warnings to 0. The changes are well-reasoned and the comments explain each suppression clearly. A few observations and one architectural suggestion below.
Positive Notes
- The
net6.0removal fromTUnit.TestProject.Libraryis the right call — net6.0 is EOL, and keeping it was causing real package compatibility warnings from 9.0.x packages. - All
NoWarnadditions correctly use$(NoWarn);CODEto accumulate rather than replace, avoiding accidental suppression of other warnings. - Comments on each suppression explain the why, which is important for future maintainers.
Architectural Observation: MSB3277 suppression is scattered across three projects
The MSB3277 suppression (older Microsoft.Bcl.AsyncInterfaces from netstandard2.0 source generator projects) is now duplicated in three separate .csproj files:
TUnit.Core.SourceGenerator.Tests/TUnit.Core.SourceGenerator.Tests.csprojTUnit.Assertions.SourceGenerator.Tests/TUnit.Assertions.SourceGenerator.Tests.csprojTUnit.SourceGenerator.IncrementalTests/TUnit.SourceGenerator.IncrementalTests.csproj
The root cause is the same in all three: any project that takes a ProjectReference to a netstandard2.0 source generator project encounters this conflict. If a fourth test project for source generators is ever added, it will need the same suppression added manually.
Consider centralizing this in Directory.Build.props with a condition on the presence of source generator project references, or more practically, just add it to the root Directory.Build.props NoWarn property alongside the existing NU1507;NU1903;CS9107. Since MSB3277 is a build-time-only conflict that is benign (it affects only the test host, not the shipped assemblies), suppressing it globally for this repo would be safe and avoids the scatter pattern.
<!-- In root Directory.Build.props -->
<NoWarn>NU1507;NU1903;CS9107;MSB3277</NoWarn>The trade-off is that a global suppression is less targeted. The per-project approach in this PR is more precise and safer, so it's a reasonable choice — just worth considering for maintainability if more source generator test projects are added in the future.
Observation: NU1504 root cause — suppression vs. fix
The NU1504 warning in the F# templates exists because TUnit.Assertions.props (shipped as part of the TUnit.Assertions NuGet package, which TUnit depends on) auto-adds TUnit.Assertions.FSharp for .fsproj projects, and the template .fsproj files also add it explicitly with a pinned version:
<!-- TUnit.FSharp/TestProject.fsproj -->
<PackageReference Include="TUnit.Assertions.FSharp" Version="1.25.0" />Suppressing NU1504 is a valid workaround and the comment explains it well. However, an alternative worth considering is whether the template .fsproj files could omit the explicit PackageReference for TUnit.Assertions.FSharp and rely entirely on the auto-injection from TUnit.Assertions.props. The benefit would be one fewer thing for template maintainers to keep in sync (the pinned version would need updating alongside TUnit every release). The downside is reduced discoverability for users reading the template — they wouldn't see TUnit.Assertions.FSharp listed explicitly.
This is a judgment call and the current approach (explicit + suppress) is fine. Just flagging the alternative in case the auto-injection path is considered reliable enough.
Minor: trailing whitespace removal in template .fsproj files
The two hunks in TUnit.FSharp/TestProject.fsproj and TUnit.AspNet.FSharp/TestProject/TestProject.fsproj only remove trailing whitespace from the TUnit.Assertions.FSharp line. This is a good cleanup bundled in naturally.
Summary
The PR is correct and achieves its goal cleanly. The per-project MSB3277 suppression approach is slightly more verbose than a centralized one, but it is safe and precise. No blocking issues — this is good to merge.
The NU1504 duplicate PackageReference warning only affects F# template projects, so scope the suppression with a condition on MSBuildProjectExtension rather than applying it to all template content projects.
90a3209 to
8da3d96
Compare
There was a problem hiding this comment.
Follow-up Review
The second commit (8da3d96) addresses the scoping concern from my previous review: the NU1504 suppression in TUnit.Templates/content/Directory.Build.props is now correctly conditioned on $(MSBuildProjectExtension) == '.fsproj', so it only applies to F# template projects rather than all template content projects. Good fix.
Remaining observations (from previous review — still open)
The MSB3277 suppression for the netstandard2.0 Microsoft.Bcl.AsyncInterfaces version conflict is still scattered across three separate .csproj files:
TUnit.Core.SourceGenerator.TestsTUnit.Assertions.SourceGenerator.TestsTUnit.SourceGenerator.IncrementalTests
This is not blocking — the per-project approach is precise and safe. But if a fourth source-generator test project is added, it will need the same suppression manually. Worth tracking if the project grows.
Summary
Both commits together produce a clean result. The NU1504 scoping fix in the second commit resolves the main concern from the first review. The PR is good to merge.
There was a problem hiding this comment.
Follow-up Review (third commit)
The third commit (bd0e6f2) fixes the snapshot test failures introduced by the trailing-whitespace cleanup in the first commit. This is the expected flow: when template source files change, the corresponding .verified.txt snapshot files in TUnit.Templates.Tests/Snapshots/ must be updated to match.
What was fixed
The InstantiationTestWithFSharp snapshot files for both the TUnit.FSharp and TUnit.AspNet.FSharp templates had a trailing tab on the TUnit.Assertions.FSharp <PackageReference> line. The first commit removed that trailing whitespace from the template source .fsproj files, but the .verified.txt snapshots still contained the old tab — causing snapshot comparison failures. This commit aligns the snapshots with the updated template output.
Assessment
This is the correct resolution. Per the project's snapshot testing rules, .verified.txt files must be committed when generator/template output changes, and .received.txt files must never be committed. The commit message explains the cause clearly.
Overall summary across all three commits
| Commit | Change | Status |
|---|---|---|
05fd406 |
Initial warning suppressions + trailing whitespace fix in template source files | Good |
90a3209 / 8da3d96 |
Scope NU1504 suppression to F# projects only |
Good (addressed previous review feedback) |
bd0e6f2 |
Fix snapshot files to match updated template output | Good (correctly completes the whitespace cleanup) |
The PR is complete and coherent. All three commits form a logical chain: fix warnings → scope suppressions precisely → update snapshots to match. No outstanding concerns. Ready to merge.
The verified .fsproj snapshots had trailing tab characters that didn't match the actual template output, causing template tests to fail.
bd0e6f2 to
17d4634
Compare
Summary
net6.0target fromTUnit.TestProject.Library— net6.0 is EOL and 9.0.x packages no longer support it (5 TFM support warnings)MSB3277in source generator test projects — benign assembly version conflict caused by netstandard2.0 source generators bundling olderMicrosoft.Bcl.AsyncInterfacesNU1701inTUnit.SourceGenerator.Benchmarks—Microsoft.Build 18.4.0only ships .NET Framework binaries, expected incompatibilityNU1504in template contentDirectory.Build.props— duplicateTUnit.Assertions.FSharpPackageReference caused byTUnit.Assertions.propsauto-adding it for F# projectsBrings the solution from 18 warnings down to 0.
Test plan
dotnet build TUnit.slnxsucceeds with 0 warnings and 0 errors