test(compliance): split the BHoM-free unit tests into a runnable project - #6
Merged
Merged
Conversation
ComplianceRunner drives five of the seven checks and none of its tests have ever executed. Compliance.Tests ProjectReferences Compliance.Shared, whose HintPaths need a BHoM install plus a built Test_Toolkit, so the whole project has been inert since it was written. Compliance.Unit.Tests carries the three test files that reference no BHoM type and compiles the four source files they cover directly, so it builds and runs on a bare runner. 62 tests now execute in test-tools.yml. The BHoM-dependent files stay where they are, with a comment recording what each one needs and how to enable it. No runner behaviour changed. The three test files moved verbatim.
One conflict, .github/workflows/test-tools.yml. Both sides added to it at the same anchor: this branch adds a step to dotnet-tests, develop adds the compliance-tests job. Both belong, so both are kept, with the step inside dotnet-tests where it runs with nothing installed, and the job after it. The file header is rewritten rather than taken from either side. Develop's version described two jobs. This branch's version said Compliance.Tests is not run and explained what running it would need, which is now what the second job does, so that paragraph is dropped. The result keeps develop's job descriptions, its measurement of which HintPaths resolve on a machine carrying the installer, and its reasoning for keeping the jobs separate, and folds in the split across two test projects. One trailing clause of develop's is dropped with it: the nine test files it describes as inert are now six, and they run. setup-dotnet keeps both 8.0 and 10.x. Compliance.Unit.Tests targets net10.0 and does not build without it. No test moved and no assertion changed by this resolution.
The comment added by this branch opened "does not run in CI" and closed with what a workflow would have to do to make it run. Both were written when nothing ran this project. It now runs in test-tools.yml's compliance-tests job, which does exactly what that closing paragraph described, so the paragraph is cut rather than reworded. The middle section is unchanged: what stays in this project, why each item cannot move, and the note that the boundary is not unit-versus-integration.
PathspecFilterPairingTests cites FileFilterTests twice, and one of those citations is load-bearing: line 20 is the NotAssemblyInfo.cs case this file pairs against. Moving FileFilterTests into Compliance.Unit.Tests leaves both references pointing at a file that is no longer a sibling. The header now names the project the file moved to, and the line citation no longer carries a path, so it survives this move without acquiring one that rots again. The line number is unchanged because the move is verbatim. This touches a file that was not previously part of this pull request. It is here because merging this branch is what makes the existing comment wrong, and leaving a broken pointer for someone else to find is worse than the wider diff.
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.
Why this matters
Nothing in this repository stops someone adding a BHoM dependency to
Annotation.cs,ArgParser.cs,FileFilter.csorSarifBuilder.cs. Those four files are the part ofCompliance.Sharedthat has no business needing a BHoM install, and that property iscurrently maintained by nobody noticing.
This PR makes it enforced.
Compliance.Unit.Testscompiles those four files directlyrather than project-referencing
Compliance.Shared, so a BHoM type appearing in any ofthem stops the project building on a bare runner and turns
dotnet-testsred. That isthe strongest reason to merge this, and it is a guarantee no other check provides.
What changed since this PR was opened
#7 merged, and it changed the premise.
Compliance.Testsnow runs: itscompliance-testsjob resolves the Test_Toolkit dependency graph first, so all 114 of its tests execute,
including the 62 this PR moves. So this PR no longer un-inerts anything. What it does is
relocate those 62 into a job that needs nothing installed.
That is still worth doing, for two reasons beyond the boundary above:
resolve-dependenciesbreakage takes all 114 tests down withit. After this, 62 survive it.
resolve-dependenciesis one of the actions thisrepository ships, so the suite that would catch a regression in it should not be
suppressed by that same regression. That is the argument test(compliance): make ComplianceRunner.Main observable #7 already makes for keeping
the jobs separate; it applies to the projects too.
dependency graph, with a 45-minute timeout, into one that finishes in about a minute.
What changed
A new project,
tools/ComplianceRunner/tests/Compliance.Unit.Tests, containing the threetest files that reference no BHoM type:
ArgParserTests,FileFilterTests,SarifBuilderTests. They moved verbatim — git records all three as pure renames at 100%similarity.
The new project deliberately has no
ProjectReferencetoCompliance.Shared. Insteadit compiles the four source files those tests cover directly via
Compile Include:Annotation.cs,ArgParser.cs,FileFilter.cs,SarifBuilder.cs. That is what lets itbuild without a BHoM install, and it is what makes the boundary enforceable — referencing
the shared project would drag in the BHoM-dependent files regardless of which tests are
present.
Those four files were checked line by line, not selected by name: every "BHoM" occurrence
in them is a comment or a string literal, such as the
BHoM.<check>SARIF rule-id prefix.test-tools.ymlgains a step indotnet-tests, andsetup-dotnetthere now installs both8.0 and 10.x — the two existing suites target
net8.0-windows, this one targetsnet10.0.Where each half runs now
Compliance.Unit.Testsdotnet-tests, no BHoM installCompliance.Testscompliance-tests, after the dependency graph builds114 either way. This moves tests between jobs; it does not add or remove coverage, and no
assertion changed.
Compliance.Testskeeps the six BHoM-dependent files: three unit files that take a BHoMtype as a parameter, and three integration files that invoke the built runner executables.
Each is named in a comment in its
.csprojwith what it needs. Note the boundary is notunit-versus-integration — three of the six original unit test files are BHoM-dependent, and
for two of them the whole dependency is a single enum parameter.
Verification
62 tests passing in the hermetic job, on a clean
windows-latestrunner with no BHoMassemblies present, and the build output contains no BHoM,
Test_*orCodeCompliance*assemblies. 52 passing in
compliance-tests.CI on this PR:
dotnet tests, including Compliance.Unit.Tests ·
ComplianceRunner tests
No runner behaviour changed.
Note on framework choice
NUnit, not the xUnit used by
SerialiserRunner.TestsandVersioningRunner.Tests. Thethree files moved verbatim from an NUnit project, and rewriting ~350 lines of assertions
into xUnit would risk changing what they assert — the point of the change was to find out
what they assert. Harmonising the two frameworks is a separate decision. The reasoning is
recorded in the new
.csproj.What to look at
Compliance.Unit.Tests.csproj— specifically theCompile Includelist, which must stayin step with
Compliance.Shared. A new file there that references no BHoM type belongshere too; one that does must not be added, or the project stops building on a clean
runner. Verified still correct against
Compliance.Sharedas it stands on develop: themerges touched nothing under
src/.test-tools.yml, in particular that the new step isinside
dotnet-testsand not insidecompliance-tests. It parses either way and passeseither way; only one of them is hermetic.