Enforce ADR-0009's rules with architecture tests (task 128) #25
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/architecture-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
tests/PlaceMark.Architecture.Testsenforces four of ADR-0009's five rules: both leaf projects reference nothing,PlaceMark.WebUI's transitive closure is exactlyPlaceMark.Contracts, and neither leaf carries a NuGet package. Seven tests, each failure message naming the rule and why it exists. Recorded in ADR-0030.The mechanism is
XDocumentover the.csprojfiles rather than NetArchTest. NetArchTest runs on .NET 10, but it reported success for a project whose unusedProjectReferencethe compiler had dropped from the assembly manifest — and no IL-based tool can see aPackageReferenceat all, which rules 4 and 5 are entirely about. Rejected alternatives are in the ADR.Rule 3 is deliberately not implemented. Domain entities never being exposed over the wire cannot be asserted yet: the only route is
/healthandPlaceMark.Domainhas no types, so the test could not fail and the acceptance criterion "introducing the violation makes it fail" could not be met. A test that passes vacuously reads as coverage while providing none. The README still lists rule 3 as review-enforced, and it needs a follow-up ticket against the first endpoint that returns something persisted.docs/adr/README.mdand theCLAUDE.mdone-line edit will conflict with the three PRs in flight — keep the ADR rows in numeric order.Solution.IncludedBymatches item names case-sensitively, but MSBuild does not:<projectreference Include="..\PlaceMark.Contracts\PlaceMark.Contracts.csproj" />inPlaceMark.Domain.csprojbuilds a real edge (PlaceMark.Contracts.dlllands in Domain's output, 0 warnings,-getItem:ProjectReferenceconfirms it) and all seven tests pass — match onName.LocalNamewithOrdinalIgnoreCase.Same hole in
ImportedBuildFilesTests._referenceItemNames, which comparesLocalNamewithStringComparer.Ordinal: aDirectory.Build.targetsdeclaring<packagereference Include="Dapper" Version="2.1.79" />puts Dapper in every project including Domain, and the vacuity guard passes.XDocument.Descendants(itemName)is also namespace-sensitive:<Project Sdk="Microsoft.NET.Sdk" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">onPlaceMark.Domain.csprojhides both aProjectReferenceto Contracts and aPackageReferenceto Dapper while MSBuild honours both — switching toDescendants()filtered onLocalNamecloses this and the case hole together.Keep
Attribute("Include")case-sensitive when you fix the above, and say so in the comment: MSBuild rejectsinclude=outright with MSB4232, so widening it would be noise.Reference(a raw assembly reference with aHintPath) is in neither_referenceItemNamesnor the project reader — add it to the imported-file list and name it in ADR-0030's "what the mechanism cannot see".ADR-0030 and the README defer rule 3 without citing task #140, which exists and carries the acceptance criteria — reference it in both so the deferral is traceable from the repository.
Verdict: changes required
Fixed in
55e1a7eat the root: item names are matched onLocalNamewithOrdinalIgnoreCase, in one place, so case and namespace close together;Includestays case-sensitive with MSB4232 cited beside it. Looking for a fourth foundFrameworkReference—Microsoft.AspNetCore.Appin a plain class library compiles and puts all of ASP.NET Core in scope with no package in the file — soReference,FrameworkReference,COMReferenceandNativeReferencejoin one shared item list rather than only the build-file guard, and rules 4 and 5 are now about depending on nothing outside the framework. All seven new violations verified failing; ADR-0030 and the README cite #140.The
Sdkattribute is a way of naming something outside the project that is not an item, so ADR-0030's "every way" is overstated:PlaceMark.Domain.csprojas<Project Sdk="Microsoft.NET.Sdk.Web">with<OutputType>Library</OutputType>and nothing else builds clean underTreatWarningsAsErrors, getsFrameworkReference Microsoft.AspNetCore.Appimplicitly (-getItem:FrameworkReferenceconfirms), compiles domain code againstMicrosoft.AspNetCore.Http, and all seven tests pass — assert the SDK of the two leaf projects (attribute,<Sdk Name>element andImport Sdk=alike), or name it in "what the mechanism cannot see" and drop the "every way" claim.Solution.DependencyItemNamesispublicbut called only fromDependencyItemsIntwo members below it — make it private.Verdict: changes required
Fixed in
c5c4062: both leaves are pinned toMicrosoft.NET.Sdkacross all three spellings (Sdkattribute,<Sdk Name>,Sdk=on anImport), each confirmed to bring the SDK in, andDependencyItemNamesis private. One level out, the remaining construct that cannot be read at all is<Import>itself, so the three projects these rules judge may not declare one rather than the limit being documented;Directory.Solution.propswas measured and does not reach projects. ADR-0030 drops "every way" and now states what genuinely cannot be read here — an SDK's or an import's contents, and any import named from outside the repository. Five new violations verified failing, all fifteen earlier ones still do.Paused. No verdict on
c5c4062— the re-review was stopped before it reported. Resume by reviewing55e1a7e..c5c4062; the three earlier rounds are above.An implicitly imported build file may itself declare an
<Import>, and the file it names is never scanned — so the construct this round refuses in the three judged.csprojfiles is still available one level up, in the files that are imported into them. RootDirectory.Build.propswith<Import Project="Analysers.props" />, and<PackageReference Include="Dapper" />inAnalysers.props, puts Dapper inPlaceMark.Domain:-getItem:PackageReferenceandproject.assets.jsonboth show it, domain code compiles againstDapper.SqlMapper, 0 warnings underTreatWarningsAsErrors, and all twelve tests pass.ImportedBuildFilesTestsreads only dependency items out of those files. Hold them to the same rule as the projects — noImportand noSdkin an implicitly imported build file — and fix the class comment, which says the two things that could put a reference in are an implicit build file and a project's own<Import>; the third is an<Import>in an implicit build file.Directory.Build.rspis listed in ADR-0030's limits under "any import named from outside the repository", justified as "None of these is XML in this repository" — it is in this repository, and it is the one item of that bullet that this round's own principle covers. A rootDirectory.Build.rspholding-p:CustomBeforeMicrosoftCommonProps=…puts aPackageReferenceinto every project, whatever the working directory, with the twelve tests green. Assert the tree holds none, and move it out of that bullet to the one above it.Verdict: changes required
Fixed in
62c474aby replacing the list with the boundary: the repository holds exactly one build file —Directory.Build.propsat the root, matched by extension anywhere in the tree — and it declares no dependency, noImportand no SDK. That covers both holes plusDirectory.Build.targets,Directory.Packages.props,Directory.Solution.props,before./after.the solution and any file anImportnames, none of them enumerated. ADR-0030 now tabulates every file MSBuild can be made to read from inside this repository as scanned, refused or out of reach, withDirectory.Build.rspmoved to refused; two negative measurements are recorded there —Directory.Solution.propsreaches no project, and an SDK in a build file breaks evaluation rather than adding anything quietly, so that clause is belt-and-braces. Five new violations verified failing, all twenty earlier ones still do.$(MSBuildProjectFullPath).useris a second file MSBuild imports into every project without being asked, and the extension sweep does not carry it:src/PlaceMark.Domain/PlaceMark.Domain.csproj.userholding<PackageReference Include="Dapper" />puts Dapper inPlaceMark.Domain—-getItem:PackageReferenceconfirms — with all thirteen tests green. It sits outsidebin/,obj/and.git, and has no row in the table. Adding.userto_buildFileExtensionswould fail the run for anyone whose IDE writes one, so the fix is more likely to draw the boundary at the tracked tree (git ls-files) and give what git ignores a row of its own, than to extend the list.obj/holds more than what restore wrote. MSBuild wildcard-imports$(MSBuildProjectExtensionsPath)$(MSBuildProjectFile).*.propsand.targets, so any file put there is imported:src/PlaceMark.Domain/obj/PlaceMark.Domain.csproj.zzz.propswith aPackageReferenceputs Dapper in Domain, thirteen tests green. The table's stated reason — "derived from the package references that are scanned" — is the half that is wrong, and a wrong reason is what stops an exclusion being re-examined. Say instead that it is a directory MSBuild will import anything from, excluded because it is untracked build output.Verdict: changes required
Fixed in
3d98365by deleting the exclusion list rather than extending it: what the repository holds is nowgit ls-files --cached --others --exclude-standard, sobin/,obj/,.gitand an IDE's.csproj.userneed no mention and the reason is the true one — not that nothing is read from them, but that nothing put there reaches a commit, a clone or CI..userjoins the extension list on that principle: untracked leaves thirteen tests green, committed fails, both verified. ADR-0030's table gains rows for.csproj.userand forbin//obj/with the wildcard-import fact, and now states that the no-SDK clause of the build-file rule is enforced by MSBuild going red (MSB4011, MSB4184) rather than by a test; the<Import>rule was re-verified against an import naming a file that exists, so its evidence is a failing test rather than MSB4019.Nothing to act on.
Verdict: mergeable
Nothing to act on: the merge adds no tracked build file, leaves
PlaceMark.Domainand the project graph untouched, andGenerateDocumentationFileis a property rather than an item, soPlaceMark.Contractsstill depends on nothing — the reader was re-checked against the reshaped file, and aPackageReferenceorImportadded beside that property still reddens.Verdict: mergeable