Fix the build, and give SchemaEditor a headless test harness (#128) - #139
Merged
Conversation
…rp reference The .NET workflow has been failing on main with three NU1608 errors. SchemaEditor pulls Hexa.NET.ImGui.Widgets.Extras transitively, which pins Microsoft.CodeAnalysis.CSharp.Scripting 4.14.0, and that in turn pins Microsoft.CodeAnalysis.Common and .CSharp to exactly 4.14.0. The project also names Microsoft.CodeAnalysis.CSharp directly, which central package management resolves at 5.9.0, and NuGet rejects the conflict as an error because warnings are errors here. Nothing in SchemaEditor uses Roslyn: the only "CodeAnalysis" in its source is System.Diagnostics.CodeAnalysis, which is in the base class library. The reference is removed rather than downgraded, so the transitive constraint resolves on its own. Schema.Test keeps its own reference at 5.9.0 - it genuinely compiles generated source - and has no ImGui dependency to conflict with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jc8o5zF3cmfCGjzWvQdGDE
SchemaEditor had never had a test. Every line of it is either an ImGui draw call or logic interleaved with one, so none of it ran without a live ImGui context - which is why #125 landed at 19% coverage of new code and the fix was to exclude the whole application from Sonar's coverage measurement. ktsu.ImGui.App.Testing supplies that context with no window, no display and no GPU: it rasterizes in software and injects input straight into ImGui's event queue. So the editor's real draw code now runs on a headless CI runner, and disturbs nothing on a desktop. Two fixtures wrap it. EditorHarness starts a real editor from Program.CreateConfig - the same configuration the application itself starts with, so a callback renamed or dropped there breaks the tests rather than leaving them passing against a host that no longer exists - after redirecting AppDataStorage to an in-memory file system, so a test never reads or writes the settings of whoever is running it. WidgetHarness draws one widget and nothing else. 46 tests cover what the issue listed: - RecordRecentFile: ordering, deduplication (including a list that already held duplicates), bounding, and an empty path. - EditField: that an editing session is one write however many frames it spans, that leaving a field untouched or abandoning an edit with Escape writes nothing, that an edit spanning frames keeps every character, and that a field not being edited follows the model so an undo is not written back over. - The unsaved-changes guard: that an unmodified document is discarded without asking, a modified one is not, and that Save, Discard and Cancel each do what they say - driven by clicking the prompt in a rendered frame, since the sequencing being tested is what those callbacks do. Also that a save with no path defers to the file browser rather than running the continuation, that a failed save does not run it, and that closing is refused while work would be lost without stacking a prompt per attempt. - Diagnostics: that validation waits for the schema to settle, that each edit restarts the debounce, that it does not re-run unprompted, and that clicking an issue selects the right element for every element kind. TwoRowsSharingALabelDoNotShareABuffer is the one that needed pixels. ImGui keeps its own copy of the text while a widget is being edited, so with the buffers wrongly shared the row being typed into still commits the right value - the row that is wrong is the other one, handed its sibling's text to draw. The test compares that row's pixels before and during the edit; reintroducing the label-keyed buffer fails it, and nothing else in the suite notices. Production changes are confined to what the tests need to reach: Program.CreateConfig is split out of Main, and a handful of members on SchemaEditor widen from private to internal. Both InternalsVisibleTo names are given on each non-test project, since there are two test assemblies now and KTSU0002 is not checking which one reads what. The Sonar coverage exclusion narrows from SchemaEditor/**/*.cs to the panel and tree files, which are still pure draw code that this harness does not reach yet. Docs and the roadmap are updated to match, including Phase 2, which still described #116 as blocked after it landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jc8o5zF3cmfCGjzWvQdGDE
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Three findings from the code quality bot, all the same rule: Path.Combine silently discards everything before a rooted segment, so a name that turns out not to be a leaf puts the file somewhere other than the scratch directory - and so outside what the cleanup deletes. Every name involved is a literal or a hex GUID today, so none of the three can actually be rooted. Reducing each to its leaf first is what says so in the code rather than in the caller's head, and costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jc8o5zF3cmfCGjzWvQdGDE
Reducing each name to its leaf did not satisfy the rule - the bot flagged all three sites again - because Path.Combine cannot be proven safe from the shape of its arguments at the call site. So there is no Path.Combine in the file now. ktsu.Semantics.Paths composes with '/' over FileName and DirectoryName, neither of which can be rooted, so the scratch directory cannot be dropped by construction rather than by the caller passing the right thing. scratchDirectory becomes an AbsoluteDirectoryPath, which is what it always was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jc8o5zF3cmfCGjzWvQdGDE
SonarCloud's gate failed at 66.7% coverage on new code. The whole of that shortfall was one statement: Main, which calls ImGuiApp.Start, opens a window and does not return. It cannot be executed by a test, and Sonar's exclusions are per file, so it was dragging CreateConfig - which the harness does drive - down with it. So CreateConfig moves to EditorHost.cs, and Program.cs holds nothing but Main. The coverage exclusion names Program.cs, which now makes an honest claim: not "nobody has tested this yet", but "this cannot run outside a real window". The configuration itself stays measured, and is covered. That is the same separation Program.cs already argued for one level up - the host's contract with ImGuiApp does not belong in the editor class - applied again: describing the host is not the same as starting it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jc8o5zF3cmfCGjzWvQdGDE
|
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
2 participants
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.
Two things:
maindoes not currently build, and #128 — the editor has never had a test.1. The build (first commit)
The
.NET Workflowhas been failing onmain(run 33815693131) with threeNU1608errors.SchemaEditorpullsHexa.NET.ImGui.Widgets.Extrastransitively, which pinsMicrosoft.CodeAnalysis.CSharp.Scripting4.14.0, and that in turn pinsMicrosoft.CodeAnalysis.Commonand.CSharpto exactly 4.14.0. The project also namesMicrosoft.CodeAnalysis.CSharpdirectly, resolved at 5.9.0 by central package management, and NuGet rejects the conflict as an error because warnings are errors here.Nothing in
SchemaEditoruses Roslyn — the onlyCodeAnalysisin its source isSystem.Diagnostics.CodeAnalysis, which is in the BCL. So the reference is removed rather than downgraded, and the transitive constraint resolves on its own.Schema.Testkeeps its own reference at 5.9.0 (it genuinely compiles generated source) and has no ImGui dependency to conflict with.2. A test harness for the editor (#128)
ktsu.ImGui.App.Testingsupplies an ImGui context with no window, no display and no GPU: it rasterizes in software and injects input straight into ImGui's event queue. The editor's real draw code now runs on a headless CI runner, and disturbs nothing on a desktop.Two fixtures wrap it:
EditorHarnessstarts a real editor fromEditorHost.CreateConfig— the same configuration the application itself starts with, so a callback renamed or dropped there breaks the tests rather than leaving them passing against a host that no longer exists. It redirectsAppDataStorageto an in-memory file system first, so a test never reads or writes the settings of whoever is running it.WidgetHarnessdraws one widget and nothing else.Frames are advanced by the test, never by wall-clock time, so a loaded runner is slower rather than flakier.
What is covered
46 tests, over what the issue listed:
AppData.RecordRecentFileEditFieldSchemaEditor.FilesSchemaEditor.DiagnosticsThe one that needed pixels
TwoRowsSharingALabelDoNotShareABufferguards the bug review found in #125. ImGui keeps its own copy of the text while a widget is being edited, so with the buffers wrongly shared the row being typed into still commits the right value — the row that is wrong is the other one, handed its sibling's text to draw. The test compares that row's pixels before and during the edit. Reintroducing the label-keyed buffer fails it, and nothing else in the suite notices; I checked both ways.Production changes
Confined to what the tests need to reach:
Program.csintoEditorHost.CreateConfig, taking the editor as a parameter so the harness can drive the same configuration the application runs.Program.csis left holding onlyMain.SchemaEditorwiden fromprivatetointernal.InternalsVisibleTonames are given on each non-test project, since there are two test assemblies now andKTSU0002is not checking which one reads what.Coverage exclusion
Narrowed from
SchemaEditor/**/*.csto the panel and tree files, which are still pure draw code this harness does not reach yet — the issue's third acceptance criterion. Extending it to them is now a matter of writing tests, not of building anything, and is recorded as the next step in the roadmap.The one other exclusion is
SchemaEditor/Program.cs, and it is there for a different reason: it holds onlyMain, which callsImGuiApp.Start, opens a window and does not return. That is a line that cannot be executed by a test rather than one nobody has got to yet — which is why the configuration it passes lives inEditorHost.csand stays measured.Also
docs/ROADMAP.mdPhase 2 still described #116 as blocked on upstreamktsu.ImGui.Appchanges; both landed and the editor implements them, so that is corrected, along with the project lists inREADME.md,docs/README.mdandCLAUDE.md.Verification
345 tests, 0 failures (299 library + 46 editor), green on both
ubuntu-latestandwindows-latestin CI. The editor suite passing on a Linux runner is what makes the "no window, no display, no GPU" claim more than a hope.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jc8o5zF3cmfCGjzWvQdGDE