Run the WPF-free half of the GUI tests on Linux and macOS - #2729
Conversation
GuiFunctions targets net10.0-windows because parts of it use WPF, so the GUI and MetaDraw tests only run on Windows even though most of what they cover has nothing to do with WPF. This splits the UI-free half out and runs its tests everywhere. GuiFunctions.Core (net10.0) holds the 25 files that reference no UI type at all. The set was found by compiling candidates against TaskLayer alone and iterating to a fixed point, not by reading imports - four files with no System.Windows reference anywhere still fail to compile without it, so an import scan would have been wrong. Namespaces are unchanged, so this is not a source-breaking change for callers, and GuiGlobalParams keeps its internal setters through InternalsVisibleTo rather than widening its public API. Deliberately no PlatformTarget, unlike GuiFunctions.csproj which sets x64. Verified the output is architecture-neutral: `file` reports "PE32 ... Intel 80386 Mono/.Net assembly" rather than "PE32+ ... x86-64", so an arm64 runtime can load it (smith-chem-wisc#2503, smith-chem-wisc#2504). GuiFunctions.Core.Tests links the existing test files rather than copying them - the same source compiles into Test.csproj on Windows and into this project everywhere, so there is one copy of each test and no way for the two to drift. The Remove list is the set that still needs WPF, found the same way. 187 tests run: all of GuiTests bar three files, and eight of the MetaDraw files. They pass on macOS arm64, and a new CI job runs them on ubuntu, macOS and Windows. Two things worth knowing for whoever picks this up. The [SetUpFixture] has to be in namespace Test, not one of its own: NUnit applies it to its own namespace and below, and the linked tests are in Test.GuiTests and Test.MetaDraw. Getting that wrong fails silently - 175 of 187 passed anyway, and only the twelve tests that actually read global state failed, which reads like a porting problem rather than a missing fixture. And `dotnet sln add` maps UbuntuMac to Debug and drops Build.0, so the solution rows are set by hand to match the other projects. This is additive: GuiFunctions, GUI and Test are otherwise untouched, and nothing about the Windows build or the installer changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I verified the extraction with `dotnet build -c UbuntuMac`, which is precisely the configuration that skips the WPF GUI, so I never compiled the thing that broke. In Release, MetaDraw.xaml fails with MC3050: Cannot find the type 'LoadingProgressViewModel' Keeping the namespaces was enough for C# callers, which recompile against GuiFunctions.Core through the project reference. It is not enough for XAML: every markup file says clr-namespace:GuiFunctions;assembly=GuiFunctions, and the markup compiler resolves that against GuiFunctions.dll specifically. Three moved types are named from GUI markup - LoadingProgressViewModel, DeconHostViewModel and IsoDecDeconParamsViewModel. TypeForwardedTo for all 33 public types rather than only those three, so the extraction is non-breaking at the binary level and a future markup or reflection reference to any of them keeps working. Verified this time in the configuration that matters: GUI.csproj builds clean in Release with EnableWindowsTargeting, and the whole solution builds bar the WiX installer, which cannot run off Windows. 187 cross-platform tests still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The job ran 187 tests and reported none of them to Codecov, so the extracted code sat outside the coverage denominator on every platform. Two parts, and the second is the one that is easy to get wrong: the workflow needs --collect:"XPlat Code Coverage", and the project needs a coverlet.collector reference to supply that collector. Without the package the flag is accepted, the run prints "Unable to find a datacollector with friendly name 'XPlat Code Coverage'" on a line nobody reads, and exits 0 having produced nothing - a job that looks like it collects coverage and does not. Referenced in the csproj rather than added by a workflow step, so a local run collects it too. Verified a report is actually written rather than assuming the flag was enough: GuiFunctions.Core comes out at 77.5% line coverage from these 187 tests. Its own Codecov flag, so it is not confused with the Windows-only unittests run that covers some of the same source through Test.csproj. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2729 +/- ##
==========================================
- Coverage 93.32% 93.11% -0.22%
==========================================
Files 214 214
Lines 21804 21854 +50
Branches 4083 4089 +6
==========================================
Hits 20349 20349
- Misses 901 951 +50
Partials 554 554
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Validate Installer Contents caught a real defect rather than a stale list: GuiFunctions.dll references GuiFunctions.Core.dll, so the installed GUI cannot start without it, and the MSI was not carrying it. Whitelisting it as build-only would have shipped a broken install. Product.wxs lists every file as an explicit Component with a fixed GUID, so GuiFunctions.Core.dll and its pdb are added the same way GUIFunctions.dll is, sourced from the GUI target directory. Fresh GUIDs; verified no duplicate Component Id, Component Guid or File Id in the file afterwards. Not verified locally: WiX is MSBuild-and-Windows-only and `wix.exe` cannot run on macOS, so the MSI itself has to be built by CI. The check that failed is the one that will confirm this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
trishorts
left a comment
trishorts
left a comment
There was a problem hiding this comment.
Reviewed at 36e34ef1, with the /oracle consulted on prior art. Nothing here blocks — all 14 checks are green and the extraction itself is sound.
On the installer: I verified the Validate Installer Contents failure independently before seeing your fix, and the fix is right — both components with fresh explicit GUIDs, sourced from $(var.GUI_TargetDir), adjacent to src_GUIFunctions.pdb, pdb parity included. Worth noting the PR body still says "nothing about the Windows build or the WiX installer changes"; that sentence is now false and is the kind of claim a future reader would trust.
The oracle backs the design: MetaMorpheus master has no UI-free MVVM home (EngineLayer/TaskLayer/CMD host zero view models), no source-linking pattern anywhere in the repo, and no test project has ever run on Linux/macOS. GuiFunctions.Core duplicates nothing. I also checked TypeForwards.cs exhaustively — 33 public types in the new assembly, 33 forwards, exact set match.
Four inline notes below. Three more that don't anchor to the diff:
ColorMapping/README.md:26 is now stale — its file tree still lists ColorGradientType.cs under GuiFunctions. More interesting than the stale line: the enum now sits alone in Core, across an assembly boundary from ColorGradientFactory and all five palette classes that switch on it. That split may be unavoidable, but it is worth a word in the README so the next person does not "fix" it by moving the enum back.
The body's file counts do not match the csproj. It says "all of Test/GuiTests bar three files" — the Remove list drops five, so 11 of 16 are linked. And "eight of the Test/MetaDraw files" — actually 3 of 24 (21 removed). The 187-test figure may well be right; it is the two ratios that overstate coverage.
DownloadUniProtDatabaseFunctions.GetUniprotFilename hardcodes a \ path separator, and has now moved into an assembly whose entire premise is platform neutrality. Pre-existing and moved unchanged, so out of scope for this PR — but it is a good argument for filing it now, since the file's new address implies a guarantee it does not yet meet.
Nice piece of work — the fixed-point method for finding the file set, and linking the tests rather than copying them, are both the right calls.
| Global | ||
| GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
| Debug|Any CPU = Debug|Any CPU | ||
| Debug|x64 = Debug|x64 |
There was a problem hiding this comment.
This adds six new solution platforms — x64 and x86 across Debug, Release and UbuntuMac. Master has exactly three (Debug|Any CPU, Release|Any CPU, UbuntuMac|Any CPU); I checked against upstream/master.
The PR body mentions hand-fixing the UbuntuMac rows but not this, and it has a real consequence further down: the new UbuntuMac|x64 and UbuntuMac|x86 rows give .Build.0 to GUI, MetaMorpheusSetup and Bootstrapper — the three projects that UbuntuMac|Any CPU deliberately withholds it from. Selecting UbuntuMac|x64 in Visual Studio would try to build the WPF GUI and both WiX projects under a configuration that exists to exclude them.
CI is unaffected (no workflow passes -p:Platform, so everything resolves to Any CPU), which is exactly what makes it easy to miss. This is the same class of sln churn as smith-chem-wisc/mzLib#1127.
Suggest dropping the x64/x86 solution platforms entirely and keeping only the two new project rows under the three existing Any CPU configs.
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net10.0</TargetFramework> | ||
| <Nullable>disable</Nullable> |
There was a problem hiding this comment.
Every other project in the solution declares <Configurations>Debug;Release</Configurations> — GuiFunctions.csproj:5, GUI.csproj:15, TaskLayer.csproj:5. That declaration is why the sln maps their UbuntuMac|Any CPU to Release|Any CPU: there is no UbuntuMac configuration for them to build.
Neither new project declares it, and the sln correspondingly maps both to UbuntuMac|Any CPU. So under dotnet build MetaMorpheus.sln -c UbuntuMac — which build.yml runs — these two land in bin/UbuntuMac/net10.0/ while every sibling lands in bin/Release/.
It builds today, so this is consistency rather than breakage. But InstallRunAndArtifact.yml's $foldersToCheck and BuildReleaseArtifact.yml both reach into bin/Release, so a project that sometimes writes elsewhere is a trap worth closing now: add <Configurations>Debug;Release</Configurations> to both csproj files and point the two UbuntuMac|* sln rows at Release|Any CPU like their siblings.
| dotnet-version: ${{ env.DOTNET_VERSION }} | ||
|
|
||
| - name: Run GuiFunctions.Core tests with coverage | ||
| run: dotnet test ./MetaMorpheus/GuiFunctions.Core.Tests/GuiFunctions.Core.Tests.csproj --configuration Release --verbosity normal --collect:"XPlat Code Coverage" |
There was a problem hiding this comment.
This omits --filter "Category!=ExternalService", which the Windows job a few blocks up carries with a comment explaining why, and which the repo applies at every other run site.
Harmless today — I checked, and the only file under the linked directories carrying that category is Test/GuiTests/GuiFunctionsTest.cs, which is already in your <Compile Remove> list. So the filter would currently be a no-op.
The reason to add it anyway is that the link glob is recursive and opt-out. A future test added to Test/GuiTests/ that is external-service-flavoured but not WPF-bound gets adopted automatically, and lands in a three-OS job that is blocking — turning a UniProt outage into red CI on someone else's PR. That is precisely the failure the non-blocking external-service-tests job exists to prevent.
One-line fix, and it keeps this job honest with the convention.
| // them. Splitting this assembly out of GuiFunctions put those writers on the other side of an | ||
| // assembly boundary, so grant them access rather than widening the public API. | ||
| [assembly: InternalsVisibleTo("GuiFunctions")] | ||
| [assembly: InternalsVisibleTo("Test")] |
There was a problem hiding this comment.
GuiFunctions.Core.Tests is not granted access here, so the two projects do not compile the linked sources under the same visibility — which narrows the "same source, one copy, no drift" property slightly.
Nothing is broken today: every internal setter is on GuiGlobalParams, and the only test exercising them (GuiGlobalParamsTests.cs) is legitimately WPF-excluded — it goes through GuiGlobalParamsViewModel, which stays in GuiFunctions and imports System.Windows. So the asymmetry is latent.
But it means a future linked test touching an internal compiles in Test.csproj and fails only in the cross-platform project, with an error that points at visibility rather than at the real cause. Adding [assembly: InternalsVisibleTo("GuiFunctions.Core.Tests")] costs a line and keeps the two compilation contexts genuinely identical.
(The "GuiFunctions" grant above is definitely needed — GuiGlobalParamsViewModel.cs:45 writes those setters.)
OxyPlot.Wpf 2.2.0 split OxyPlot.Wpf.Shared.dll out of OxyPlot.Wpf.dll, so the version bump added an assembly the MSI did not know about. Caught by Validate Installer Contents; without it an installed GUI cannot load OxyPlot.Wpf and MetaDraw does not open, which is the same class of defect as the missing GuiFunctions.Core.dll on smith-chem-wisc#2729. The component sits in ExecutablesAndLibrariesComponentGroup, which the feature already references, so no ComponentRef was needed. No .pdb entry: those exist only for our own projects, and the check only asks for the DLL. Also corrects a comment in the phase 3 tests that still described reading ActualBarRectangles - that was the first draft, before it turned out not to be public and the assertions moved to the axis transforms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
🤖
Run the WPF-free half of the GUI tests on Linux and macOS
GuiFunctionstargetsnet10.0-windowsbecause parts of it use WPF. That means the GUI and MetaDrawtests only run on Windows, even though most of what they cover has nothing to do with WPF. This splits
the UI-free half into its own project and runs its tests on all three platforms.
Offered separately from #2709 and independent of it. @trishorts recommended exactly this as the next
step there: "the cross-platform
GuiFunctionsextraction, alone: it retains full value even if theGUI is abandoned, and doing it before Milestone 1 stops the duplicate being hardened." It stands on
its own merits whatever happens to the Avalonia front end.
What moves
GuiFunctions.Core(net10.0) holds the 25 files that reference no UI type at all — the deconvolutionview models,
BaseViewModel,GuiGlobalParams,MzLibExtensions,SpectralMatchComparer,HungarianAlgorithm,PlotModelStatParametersand friends.The set was found by compiling candidates against
TaskLayeralone and iterating to a fixed point,not by reading imports. That distinction matters: four files with no
System.Windowsreference anywherestill fail to compile without WPF, so an import scan would have produced the wrong answer and a broken
build.
Namespaces are unchanged, so this is not a source-breaking change for callers.
GuiGlobalParamskeepsits internal setters through
InternalsVisibleTorather than having them widened to public to crossthe new assembly boundary.
The tests are linked, not copied
GuiFunctions.Core.Tests(net10.0)<Compile Include>s the existing test files out ofTest/. Thesame source compiles into
Test.csprojon Windows and into this project everywhere, so there is onecopy of each test and no way for the two to drift. The
<Compile Remove>list is the set that stillneeds WPF, found the same fixed-point way.
187 tests run and pass on macOS arm64 — all of
Test/GuiTestsbar three files, and eight of theTest/MetaDrawfiles. A new CI job runs them on ubuntu-latest, macos-latest and windows-latest.Deliberately no
PlatformTargetGuiFunctions.csprojsetsx64; this project sets nothing. An x64 stamp cannot be loaded by an arm64runtime (#2503, #2504), and this project exists to be loadable anywhere. Verified rather than assumed:
filereportsPE32 ... Intel 80386 Mono/.Net assembly, notPE32+ ... x86-64. Left unset ratherthan set to
AnyCPU, whichMicrosoft.MLrejects at build time.Two notes for review
[SetUpFixture]is in namespaceTeston purpose. NUnit applies one to its own namespaceand below, and the linked tests live in
Test.GuiTestsandTest.MetaDraw. I had it in a namespaceof its own first, and it failed silently — 175 of 187 tests still passed, and only the twelve that
actually read global state failed, which reads like a porting problem rather than a missing fixture.
Ten of those twelve looked like genuine macOS incompatibilities (
FileShare.Noneis a mandatory lockon Windows and advisory on Unix) and I nearly annotated them as Windows-only before finding the real
cause.
dotnet sln addmapsUbuntuMactoDebugand dropsBuild.0, which makes the solution build"successfully" while skipping the project. The rows here are set by hand to match the other projects.
Scope
Additive.
GuiFunctions,GUIandTestare otherwise untouched, nothing about the Windows build orthe WiX installer changes, and no test is deleted or rewritten — they are the same tests, now compiled
somewhere they can run.
This is also a prerequisite for the MetaDraw port: the remaining WPF coupling in
GuiFunctionsismostly
System.Windows.Mediacolour types, and separating the UI-free half first makes that boundaryvisible.