Share the build settings, enforce warnings everywhere, measure coverage - #5
Merged
Merged
Conversation
Kentarohakase
force-pushed
the
tests/close-coverage-gaps
branch
from
August 12, 2026 00:56
e894726 to
e6c1686
Compare
Kentarohakase
force-pushed
the
build/shared-properties-and-coverage
branch
from
August 12, 2026 00:56
876c30f to
ad373ef
Compare
Kentarohakase
force-pushed
the
tests/close-coverage-gaps
branch
from
August 12, 2026 01:03
e6c1686 to
90d55cd
Compare
Both projects declared TargetFramework, Nullable and ImplicitUsings themselves, and the test project inherited neither the analyzers nor the warning policy, so test code was warning free only by omission. The shared settings move into Directory.Build.props and both project files keep just what is specific to them. Warnings are errors in every configuration now, not only in Release. A local Debug build accepted what the Release build in CI rejected, so a warning showed up after the push instead of before it. Both projects build clean in Debug and Release, so this costs nothing today. AnalysisMode stays at its default on purpose. Raising it to Recommended surfaces about 300 analyzer warnings and All about 1100, mostly CA1305, CA1822, CA2007 and CA1515. That is a cleanup in its own right and would only block every later change if it were switched on here. The SDK is pinned to the 10.0 feature band with rollForward so a future major does not silently change the build. Finally, coverlet was referenced but never used: the test run collects coverage now and CI keeps the report as an artifact. The current line rate is 49 percent.
Kentarohakase
force-pushed
the
build/shared-properties-and-coverage
branch
from
August 12, 2026 01:06
ad373ef to
ffc4ba1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #4. The build-infrastructure items from the improvement plan's low-priority list.
Shared build settings
Both project files declared
TargetFramework,NullableandImplicitUsingsseparately, and the test project inherited neitherAnalysisLevelnorEnforceCodeStyleInBuildnor the warning policy — its code was warning-free by omission rather than by rule. Those settings move intoDirectory.Build.props; each project file keeps only what is genuinely its own.Warnings are errors everywhere, not only in Release
TreatWarningsAsErrorswas set underCondition="'$(Configuration)' == 'Release'". A localdotnet buildaccepted what CI rejected, so a warning surfaced after the push rather than before it. Measured before changing it — both projects build clean in Debug and Release, and the test project builds clean with the analyzers and the warning policy applied:AnalysisMode stays off, deliberately
The plan noted that
AnalysisModeis unset, so most CA rules sit at suggestion severity and never become warnings — and therefore never become errors underTreatWarningsAsErrors. I measured the blast radius before deciding:AnalysisModeRecommendedAllThat is a cleanup in its own right. Switching it on inside a build file would block every later change until someone worked through it, so it stays at the default and the reasoning is recorded in
Directory.Build.propsrather than lost.The same applies to
.editorconfig: adding one that merely restates the defaultsdotnet formatalready enforces would be noise, and adding one that changes severities is the same cleanup by another route.Coverage is measured for the first time
coverlet.collectorhas been referenced by the test project all along, butci.ymlnever collected anything. The test step now collects it and CI keeps the Cobertura report as an artifact.Current state, measured locally:
That number is mostly explained by
MainViewModel— about 2,100 lines across eleven partials with no tests, for the reason described in #4.SDK pinned
global.jsonpins the 10.0 feature band withrollForward: latestFeature, so a future major SDK cannot silently change the build. CI installs10.0.x, which satisfies it.No changelog entry
Nothing here changes behaviour for a user of the application, so the changelog is untouched.
Verification
Coverage collection was run locally with the exact command CI now uses, and it produced
coverage.cobertura.xmlas expected.