Conversation
…ord gains - add IDataTreePainter seam and offscreen paint test harness (RecordingPainter, OffscreenGraphicsContext) - add DataTree Wave 4 tests for OnPaint/paint branches and HandleLayout1 branch paths - add Slice branch tests for IsObjectNode, LabelIndent, and GetBranchHeight - fix Slice IsObjectNode test setup to avoid NRE by setting DataTree root directly - update OpenSpec coverage matrix with 2026-02-26 checkpoint (DataTree 67.51/52.02, Slice 34.5/22.4)
|
|
There was a problem hiding this comment.
Pull request overview
This PR prepares for an upcoming DataTree/Slice refactor by significantly expanding characterization testing and adding a small painting seam (IDataTreePainter) to enable offscreen rendering tests and better coverage/diagnostics workflows.
Changes:
- Introduces
IDataTreePainterand updatesDataTreepainting to allow test-injected painters. - Adds substantial new DetailControls characterization tests (DataTree offscreen UI + ObjSeqHashMap + additional Slice test partials) and extends test layout fixtures.
- Adds local managed coverage collection/assessment wrapper scripts and updates supporting docs/VS Code tasks.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Src/Common/Controls/DetailControls/DataTree.cs |
Implements IDataTreePainter, adds Painter property, refactors paint-line logic entrypoint. |
Src/Common/Controls/DetailControls/IDataTreePainter.cs |
New interface enabling painter injection for tests. |
Src/Common/Controls/DetailControls/DetailControlsTests/DataTreeTests.Wave4.OffscreenUI.cs |
New offscreen UI test suite covering painting/layout seam behavior. |
Src/Common/Controls/DetailControls/DetailControlsTests/OffscreenGraphicsContext.cs |
Bitmap-backed graphics helper for offscreen paint assertions. |
Src/Common/Controls/DetailControls/DetailControlsTests/RecordingPainter.cs |
Test double for capturing paint calls via IDataTreePainter. |
Src/Common/Controls/DetailControls/DetailControlsTests/ObjSeqHashMapTests.cs |
Adds standalone tests for slice reuse map behaviors and current quirks. |
Src/Common/Controls/DetailControls/DetailControlsTests/Test.fwlayout |
Adds new fixture layouts for characterization scenarios. |
Build/Agent/Run-TestCoverage.ps1 / Build/Agent/Run-ManagedCoverageAssessment.ps1 |
Adds repo wrappers to collect coverage and generate gap assessments. |
Comments suppressed due to low confidence (1)
Src/Common/Controls/DetailControls/DataTree.cs:1686
- The body of
PaintLinesBetweenSliceshas several indentation anomalies (e.g., theforloop andcontinuealignment inside theusingblock). Reformatting this block would reduce the chance of misreading control flow in a method that’s already fairly branchy.
{
using (var thinPen = new Pen(Color.LightGray, 1))
using (var thickPen = new Pen(Color.LightGray, 1 + HeavyweightRuleThickness))
{
for (int i = 0; i < Slices.Count; i++)
{
var slice = Slices[i] as Slice;
if (slice == null)
continue;
// shouldn't be visible
Slice nextSlice = null;
| base.OnPaint(e); | ||
| HandlePaintLinesBetweenSlices(e); | ||
| Painter.PaintLinesBetweenSlices(e.Graphics, Width); |
There was a problem hiding this comment.
DataTree.OnPaint now calls Painter.PaintLinesBetweenSlices(...) directly. Since Painter is a public settable property, it can be set to null, which would cause a NullReferenceException during painting. Consider null-guarding in the setter (e.g., reject null) or falling back to this in OnPaint/HandlePaintLinesBetweenSlices when Painter is null.
| <layout class="LexEntry" type="detail" name="ManySenses"> | ||
| <part ref="Senses" param="GlossOnly" expansion="expanded"/> | ||
| </layout> | ||
| <layout class="LexEntry" type="detail" name="ManySensesExpanded"> | ||
| <part ref="Senses" param="GlossOnly" expansion="expanded"/> | ||
| </layout> |
There was a problem hiding this comment.
ManySenses and ManySensesExpanded layouts are currently identical (both include expansion="expanded"). If these are meant to exercise the threshold vs. override behavior, ManySenses likely should omit the expansion override so the two layouts cover distinct code paths.
| [Test] | ||
| public void Values_DuplicatesSliceAfterReuse_CurrentBehavior() | ||
| { | ||
| var map = new ObjSeqHashMap(); | ||
| var key = new ArrayList { 7 }; | ||
| var slice = MakeSlice(7); | ||
|
|
||
| map.Add(key, slice); | ||
|
|
||
| var values = map.Values.ToList(); | ||
| Assert.That(values.Count(v => ReferenceEquals(v, slice)), Is.GreaterThan(1), | ||
| "Current behavior: Values can include duplicate references from table + reuse map"); | ||
| } |
There was a problem hiding this comment.
Values_DuplicatesSliceAfterReuse_CurrentBehavior is currently demonstrating duplication immediately after Add (since ObjSeqHashMap.Add stores the slice in both m_table and m_slicesToReuse), not specifically “after reuse”. Renaming the test and/or adjusting the setup to actually involve GetSliceToReuse would make the characterized behavior clearer and avoid misleading future readers.
| function Ensure-Tool { | ||
| param( | ||
| [string]$ExeName, | ||
| [string]$PackageId | ||
| ) | ||
|
|
||
| $exePath = Join-Path $toolsDir "$ExeName.exe" | ||
| if (-not (Test-Path -LiteralPath $exePath)) { | ||
| Write-Host "Installing $PackageId to $toolsDir..." -ForegroundColor Cyan | ||
| $null = & dotnet tool install --tool-path $toolsDir $PackageId | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to install tool '$PackageId'" | ||
| } | ||
| } |
There was a problem hiding this comment.
Run-TestCoverage.ps1 installs dotnet-coverage and reportgenerator without pinning versions. This can make coverage output and even script behavior non-deterministic over time (and can break in offline/locked-down environments). Consider pinning tool versions (or using a dotnet tool manifest + dotnet tool restore) so CI/dev runs are reproducible.
This pull request is really a preparation pull request for a more extensive refactoring of datatree and slice. It is to take them down from a mammoth 4000 line file to more manageable chunks. What this does is fill out the testing in a really big way. Test coverage is up hugely, but we should make sure that we are actually testing intention, not just coverage.
This change is