From 061c73e815430eab8e4a9a3dbf1613bd3ac1b5aa Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:14:44 +0000 Subject: [PATCH 1/2] [patch] Validate the ImGuiTableColumn layout before reading it SaveColumnWidth reads column widths out of ImGui's native structs with manual pointer arithmetic, using a stride of sizeof(ImGuiTableColumn) plus a hardcoded 8 bytes to compensate for a Hexa.NET.ImGui binding bug. Two Debug.Assert calls guarded the assumptions behind that offset. Debug.Assert is compiled out of a Release build, which is the only configuration users run. If Hexa.NET.ImGui fixed the binding, or Dear ImGui reordered the struct, Release would have gone on adding 8 bytes to a stride that no longer needed it and read a float from the wrong address -- silently, with nothing to notice it. The asserts only ever protected developers. Replaces them with ProbeNativeImGuiTableColumnSize, which runs once from a static initializer and returns a nullable stride. Null disables column width persistence, which GetColumnWidth already handles by falling back to the saved or default width, so the app keeps working and only stops tracking new widths. The probe measures the bug itself rather than inferring it from a size threshold. It sums the bytes the eight narrowly-bound index fields actually occupy: eight means the binding is still narrow and the stride is sizeof + 8, sixteen means it has been fixed so sizeof is correct and the workaround can be removed, and anything else is refused rather than guessed at. A missing field, a moved WidthGiven, or a Marshal failure all disable the read. Measuring rather than thresholding also settles an inconsistency in the original code, which asserted sizeof < 112 on the grounds that native is "about 112 bytes" while simultaneously adding 8 to a sizeof of 108, implying 116. Verified against Hexa.NET.ImGui 2.2.9: sizeof is 108, all eight affected fields are one byte at contiguous offsets 86-93, and the probe returns 116 -- the same stride the previous code computed, so behaviour is unchanged today. The Marshal calls are wrapped because they previously only ran under Debug.Assert and so never executed in Release. Running them unconditionally from a static initializer must not be able to fail the type initializer. Removes the dead "issues/XXX" placeholder link, which read as though an upstream issue existed. None has been filed; #258 tracks that. Part of #258 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- BuildMonitor/BuildMonitor.cs | 153 +++++++++++++++++++++++++++++------ CLAUDE.md | 20 +++++ 2 files changed, 147 insertions(+), 26 deletions(-) diff --git a/BuildMonitor/BuildMonitor.cs b/BuildMonitor/BuildMonitor.cs index ba7b20f..960120f 100644 --- a/BuildMonitor/BuildMonitor.cs +++ b/BuildMonitor/BuildMonitor.cs @@ -6,6 +6,7 @@ namespace ktsu.BuildMonitor; using System.Collections.Generic; using System.Diagnostics; using System.Globalization; +using System.Reflection; using System.Runtime.InteropServices; using Hexa.NET.ImGui; using ktsu.Extensions; @@ -117,37 +118,140 @@ private static float GetColumnWidth(string columnName) return DefaultColumnWidths.GetValueOrDefault(columnName, 80f); } - // TODO: Remove this workaround once Hexa.NET.ImGui fixes the ImGuiTableColumn struct layout. - // The binding incorrectly uses sbyte/byte for ImGuiTableColumnIdx/ImGuiTableDrawChannelIdx fields - // which should be short/ushort (2 bytes each). This makes the C# struct 8 bytes smaller than native. - // See: https://github.com/HexaEngine/Hexa.NET.ImGui/issues/XXX (report this issue) + // Workaround for a binding bug in Hexa.NET.ImGui's ImGuiTableColumn struct layout. // - // 8 fields are wrong: DisplayOrder, IndexWithinEnabledSet, PrevEnabledColumn, NextEnabledColumn, - // SortOrder (all ImGuiTableColumnIdx = ImS16), and DrawChannelCurrent, DrawChannelFrozen, - // DrawChannelUnfrozen (all ImGuiTableDrawChannelIdx = ImU16). Each should be 2 bytes but is 1 byte. + // The binding declares ImGuiTableColumnIdx and ImGuiTableDrawChannelIdx fields as sbyte/byte + // where native Dear ImGui uses ImS16/ImU16. Eight fields are affected; each should occupy + // 2 bytes and occupies 1, leaving the C# struct 8 bytes smaller than the native one. + // + // Measured against Hexa.NET.ImGui 2.2.9: sizeof(ImGuiTableColumn) is 108, and all eight + // affected fields sit contiguously at offsets 86-93 at one byte apiece. The native stride is + // therefore 116, so reading a column needs manual pointer arithmetic rather than + // sizeof(ImGuiTableColumn). + // + // Tracked in ktsu-dev/BuildMonitor#258, which also covers reporting the bug upstream; no + // upstream issue has been filed yet. private const int ImGuiTableColumnSizeDifference = 8; - private static unsafe int GetNativeImGuiTableColumnSize() + // WidthGiven sits immediately after Flags, which is 4 bytes. + private const int WidthGivenOffset = 4; + + /// + /// The fields Hexa.NET.ImGui binds one byte too narrow. Native Dear ImGui gives each of these + /// two bytes: the first five are ImGuiTableColumnIdx (ImS16), the last three are + /// ImGuiTableDrawChannelIdx (ImU16). + /// + private static readonly string[] NarrowlyBoundColumnFields = + [ + nameof(ImGuiTableColumn.DisplayOrder), + nameof(ImGuiTableColumn.IndexWithinEnabledSet), + nameof(ImGuiTableColumn.PrevEnabledColumn), + nameof(ImGuiTableColumn.NextEnabledColumn), + nameof(ImGuiTableColumn.SortOrder), + nameof(ImGuiTableColumn.DrawChannelCurrent), + nameof(ImGuiTableColumn.DrawChannelFrozen), + nameof(ImGuiTableColumn.DrawChannelUnfrozen), + ]; + + /// + /// The native stride of ImGuiTableColumn, or when the binding's layout + /// is not one this workaround understands and reading it would be unsafe. + /// + /// + /// This was previously two calls, which are compiled + /// out of a Release build. If the binding were fixed, or Dear ImGui reordered the struct, a + /// Release build would have gone on applying an offset that no longer matched and read from + /// the wrong address -- silently, with nothing to notice it. Probing once at first use and + /// degrading to null instead means the widths simply stop being tracked, which + /// already handles by falling back to the saved or default value. + /// + private static readonly int? NativeImGuiTableColumnSize = ProbeNativeImGuiTableColumnSize(); + + /// + /// Determines the native stride of ImGuiTableColumn by measuring the binding directly. + /// + /// The stride, or if the layout is not recognised. + /// + /// Measuring the affected fields is deliberate rather than comparing sizeof against a constant. + /// A size threshold only ever infers the bug: it cannot tell a fixed binding from one whose + /// struct grew for an unrelated reason, and the "native is about 112 bytes" figure this code + /// used to carry does not even agree with the measured 108 + 8. Counting the bytes those eight + /// fields actually occupy tests the bug itself, so a fixed binding is recognised as fixed and + /// anything else is refused rather than guessed at. + /// + private static unsafe int? ProbeNativeImGuiTableColumnSize() { int csharpSize = sizeof(ImGuiTableColumn); - int nativeSize = csharpSize + ImGuiTableColumnSizeDifference; + int narrowFieldBytes = 0; + + try + { + if (Marshal.OffsetOf(nameof(ImGuiTableColumn.WidthGiven)).ToInt32() != WidthGivenOffset) + { + Log.Warning( + $"ImGuiTableColumn.WidthGiven is not at the expected offset {WidthGivenOffset}, so the " + + "struct layout is not one this workaround understands. Column width persistence is " + + "disabled. See BuildMonitor#258."); + return null; + } + + foreach (string fieldName in NarrowlyBoundColumnFields) + { + FieldInfo? field = typeof(ImGuiTableColumn).GetField(fieldName); + if (field is null) + { + Log.Warning( + $"ImGuiTableColumn has no field named {fieldName}, so the struct layout is not one " + + "this workaround understands. Column width persistence is disabled. See BuildMonitor#258."); + return null; + } + + narrowFieldBytes += Marshal.SizeOf(field.FieldType); + } + } + catch (ArgumentException ex) + { + // Marshal rejects a type it cannot marshal. These calls previously sat inside a + // Debug.Assert and so never ran in a Release build; running them unconditionally must + // not be able to take the application down, and this runs from a static initializer + // where an escaping exception becomes a TypeInitializationException. + Log.Warning( + $"Could not read the ImGuiTableColumn layout ({ex.Message}), so column width " + + "persistence is disabled. See BuildMonitor#258."); + return null; + } + + // One byte each: the binding is still narrow, so native is this many bytes wider. + if (narrowFieldBytes == NarrowlyBoundColumnFields.Length) + { + return csharpSize + ImGuiTableColumnSizeDifference; + } - // Native struct should be ~112 bytes according to imgui comments - // If C# size >= 112, the fix has likely been applied - Debug.Assert( - csharpSize < 112, - $"ImGuiTableColumn C# struct size is {csharpSize} bytes (expected < 112). " + - "Check if Hexa.NET.ImGui fixed the struct layout and remove this workaround if so."); + // Two bytes each: the binding has been fixed and now matches native, so sizeof is correct + // and the workaround should be removed along with this whole probe. + if (narrowFieldBytes == NarrowlyBoundColumnFields.Length * 2) + { + Log.Info( + "Hexa.NET.ImGui now binds the ImGuiTableColumn index fields at their native width. " + + "The struct size workaround is no longer needed and can be removed. See BuildMonitor#258."); + return csharpSize; + } - return nativeSize; + Log.Warning( + $"The ImGuiTableColumn index fields occupy {narrowFieldBytes} bytes, which matches neither the " + + $"known-narrow binding ({NarrowlyBoundColumnFields.Length}) nor a corrected one " + + $"({NarrowlyBoundColumnFields.Length * 2}). Column width persistence is disabled rather than " + + "guessing at the stride. See BuildMonitor#258."); + return null; } private static unsafe void SaveColumnWidth(string columnName, int columnIndex) { - // Assert that WidthGiven is still at the expected offset (after Flags which is 4 bytes) - Debug.Assert( - Marshal.OffsetOf(nameof(ImGuiTableColumn.WidthGiven)).ToInt32() == 4, - "ImGuiTableColumn.WidthGiven offset changed. Update the workaround."); + // Null means the layout probe rejected the binding, so there is no address we can trust. + if (NativeImGuiTableColumnSize is not int nativeStructSize) + { + return; + } ImGuiTablePtr table = ImGuiP.GetCurrentTable(); if (table.Handle == null || columnIndex < 0 || columnIndex >= table.Handle->ColumnsCount) @@ -155,15 +259,12 @@ private static unsafe void SaveColumnWidth(string columnName, int columnIndex) return; } - // Use manual pointer arithmetic with the correct native struct size - // instead of relying on C# sizeof(ImGuiTableColumn) which is incorrect - int nativeStructSize = GetNativeImGuiTableColumnSize(); + // Manual pointer arithmetic against the probed native stride, which is wider than + // sizeof(ImGuiTableColumn) -- see ProbeNativeImGuiTableColumnSize above. byte* basePtr = (byte*)table.Handle->Columns.Data; byte* columnAddress = basePtr + (columnIndex * nativeStructSize); - // Read WidthGiven directly from offset 4 (after Flags which is 4 bytes) - const int widthGivenOffset = 4; - float currentWidth = *(float*)(columnAddress + widthGivenOffset); + float currentWidth = *(float*)(columnAddress + WidthGivenOffset); if (currentWidth < 1f || currentWidth > 10000f || !float.IsFinite(currentWidth)) { diff --git a/CLAUDE.md b/CLAUDE.md index 1b358a8..afe3c15 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -260,6 +260,26 @@ The UI uses radial progress bars (from `ImGuiWidgets.RadialProgressBar`) for: - Workaround for Hexa.NET.ImGui struct layout bug (8-byte size difference) - `SaveColumnWidth()` saves when width changes by more than 1px +**The struct-layout workaround validates itself before reading.** Hexa.NET.ImGui binds eight +`ImGuiTableColumn` index fields one byte narrower than native Dear ImGui does, so the C# struct is +8 bytes smaller and `sizeof(ImGuiTableColumn)` is the wrong stride. Measured against 2.2.9: `sizeof` +is 108, the eight fields sit contiguously at offsets 86-93 at one byte apiece, and the native stride +is 116. + +`ProbeNativeImGuiTableColumnSize` runs once from a static initializer and measures **the bug +itself** — how many bytes those eight fields actually occupy — rather than inferring it from a size +threshold. Eight bytes means the binding is still narrow and the stride is `sizeof + 8`; sixteen +means it has been fixed, so `sizeof` is correct and the whole workaround can go; anything else, a +missing field, a moved `WidthGiven`, or a `Marshal` failure disables column-width persistence and +logs a warning. + +This deliberately replaced two `Debug.Assert` calls. Those are compiled out of a Release build, so +a fixed binding or a reordered struct would have left Release silently applying an offset that no +longer matched and reading from the wrong address. The probe fails closed instead: `GetColumnWidth` +already falls back to the saved or default width, so the app keeps working and only stops tracking +new widths. Do not reintroduce an assert here — it must hold in Release, which is the only +configuration users run. + ### Context Menu Actions Right-clicking on any build row opens a context menu with the following actions: From af2e549402ba488b1afb95172ed6ba4238e7c0d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:28:27 +0000 Subject: [PATCH 2/2] [minor] Add a test project The solution had one project and no tests, so SonarCloud reported 0% coverage on new code and failed the quality gate on the preceding commit. That gate cannot be satisfied by any change while the repository has no test project, so this is the fix rather than a widening. Adds BuildMonitor.Test with 32 tests over the two pieces of provider-independent logic that most affect what users see and how hard the APIs get hit. DurationEstimatorTests covers the estimator that drives the Estimate and ETA columns and the adaptive polling interval in RunSync: the three-sample floor, the IQR outlier filter, exponential weighting toward recent runs, that failed, canceled, pending and ongoing runs are never sampled, branch-specific estimation and its fallback when a branch has too little history, determinism across repeated calls, and that the estimate stays inside its own sample range. ColumnStrideTests covers the ImGuiTableColumn layout decision. To make that testable, the decision is extracted from ProbeNativeImGuiTableColumnSize into ResolveNativeColumnStride, which takes three plain integers. The probe needs unsafe, reflection and a real Hexa.NET.ImGui type, none of which a test can vary; the decision is the part worth testing, and separating it makes the corrected-binding and unrecognised-layout branches reachable at all, since neither can be produced with the binding currently referenced. Verified by mutation, by substitution rather than deletion. Making the stride resolver subtract the adjustment instead of adding it fails 5 tests; lowering MinSamplesForEstimate from 3 to 1 fails 3; letting the estimator sample every completed run rather than only successes fails 2. Two mutations initially ran without effect and are worth recording. Replacing the adjustment with a bare sizeof left ImGuiTableColumnSizeDifference unused and failed the build before any test ran, so the substitution has to keep the constant referenced. The success filter appears twice in DurationEstimator, so a whole-file replace asserting a single occurrence did nothing; only the EstimateDuration copy is the one under test. Fixes #259 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- BuildMonitor.Test/BuildMonitor.Test.csproj | 15 + BuildMonitor.Test/ColumnStrideTests.cs | 133 +++++++++ BuildMonitor.Test/DurationEstimatorTests.cs | 296 ++++++++++++++++++++ BuildMonitor.sln | 6 + BuildMonitor/AssemblyInfo.cs | 3 + BuildMonitor/BuildMonitor.cs | 77 +++-- CLAUDE.md | 43 ++- 7 files changed, 547 insertions(+), 26 deletions(-) create mode 100644 BuildMonitor.Test/BuildMonitor.Test.csproj create mode 100644 BuildMonitor.Test/ColumnStrideTests.cs create mode 100644 BuildMonitor.Test/DurationEstimatorTests.cs create mode 100644 BuildMonitor/AssemblyInfo.cs diff --git a/BuildMonitor.Test/BuildMonitor.Test.csproj b/BuildMonitor.Test/BuildMonitor.Test.csproj new file mode 100644 index 0000000..ff39656 --- /dev/null +++ b/BuildMonitor.Test/BuildMonitor.Test.csproj @@ -0,0 +1,15 @@ + + + + + + true + net10.0 + + true + + + + + + diff --git a/BuildMonitor.Test/ColumnStrideTests.cs b/BuildMonitor.Test/ColumnStrideTests.cs new file mode 100644 index 0000000..b37914a --- /dev/null +++ b/BuildMonitor.Test/ColumnStrideTests.cs @@ -0,0 +1,133 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor.Test; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests for the decision half of the ImGuiTableColumn layout workaround. +/// +/// +/// SaveColumnWidth reads a float out of ImGui's native structs by pointer arithmetic. If the +/// stride it uses is wrong it reads from the wrong address, so the rule that picks that stride is +/// worth pinning. ResolveNativeColumnStride exists as a separate, plain-integer method for +/// exactly this reason: the probe around it needs unsafe, reflection and a real +/// Hexa.NET.ImGui type, none of which a test can vary, while the decision can be driven through +/// every branch -- including the ones unreachable with the binding currently referenced. +/// +[TestClass] +public sealed class ColumnStrideTests +{ + // Measured against Hexa.NET.ImGui 2.2.9. + private const int MeasuredCSharpSize = 108; + private const int MeasuredWidthGivenOffset = 4; + private const int NarrowFieldCount = 8; + + /// + /// The layout actually shipped by the referenced binding must produce the stride the + /// workaround has always used: 108 + 8. + /// + [TestMethod] + public void TheCurrentlyShippedLayoutResolvesToTheDocumentedStride() + { + // Act + int? stride = BuildMonitor.ResolveNativeColumnStride( + MeasuredCSharpSize, MeasuredWidthGivenOffset, NarrowFieldCount); + + // Assert + Assert.AreEqual(116, stride); + } + + /// + /// A binding whose index fields are two bytes apiece matches native, so sizeof is already the + /// stride and no adjustment must be applied. + /// + [TestMethod] + public void ACorrectedBindingResolvesToSizeofWithNoAdjustment() + { + // Act -- the eight fields now occupy two bytes each, so the struct is 8 bytes larger + int? stride = BuildMonitor.ResolveNativeColumnStride( + MeasuredCSharpSize + 8, MeasuredWidthGivenOffset, NarrowFieldCount * 2); + + // Assert + Assert.AreEqual(MeasuredCSharpSize + 8, stride); + } + + /// + /// A field width that is neither the known-narrow nor the corrected one must be refused rather + /// than guessed at, since any stride derived from it would be a fabrication. + /// + /// A total that matches neither known layout. + [TestMethod] + [DataRow(0)] + [DataRow(4)] + [DataRow(7)] + [DataRow(9)] + [DataRow(12)] + [DataRow(24)] + [DataRow(32)] + public void AnUnrecognisedFieldWidthIsRefused(int narrowFieldBytes) + { + // Act + int? stride = BuildMonitor.ResolveNativeColumnStride( + MeasuredCSharpSize, MeasuredWidthGivenOffset, narrowFieldBytes); + + // Assert + Assert.IsNull(stride); + } + + /// + /// A moved WidthGiven means the struct was reordered, so the hardcoded read offset no longer + /// points at the width and nothing may be read -- whatever the field widths say. + /// + /// An offset other than the expected one. + [TestMethod] + [DataRow(0)] + [DataRow(2)] + [DataRow(8)] + [DataRow(16)] + public void AMovedWidthGivenOffsetIsRefused(int widthGivenOffset) + { + // Act -- field widths are the known-good ones; only the offset moved + int? stride = BuildMonitor.ResolveNativeColumnStride( + MeasuredCSharpSize, widthGivenOffset, NarrowFieldCount); + + // Assert + Assert.IsNull(stride); + } + + /// + /// The offset check must take precedence: a reordered struct is refused even when the field + /// widths look like a corrected binding. + /// + [TestMethod] + public void AMovedOffsetIsRefusedEvenWhenTheFieldWidthsLookCorrected() + { + // Act + int? stride = BuildMonitor.ResolveNativeColumnStride( + MeasuredCSharpSize + 8, widthGivenOffset: 12, narrowFieldBytes: NarrowFieldCount * 2); + + // Assert + Assert.IsNull(stride); + } + + /// + /// The narrow-binding branch adjusts by exactly the documented difference, whatever the + /// struct's overall size, so a struct that grows for an unrelated reason still resolves. + /// + /// A plausible struct size. + [TestMethod] + [DataRow(96)] + [DataRow(108)] + [DataRow(120)] + [DataRow(160)] + public void TheNarrowBindingAdjustmentIsIndependentOfTheStructSize(int csharpSize) + { + // Act + int? stride = BuildMonitor.ResolveNativeColumnStride( + csharpSize, MeasuredWidthGivenOffset, NarrowFieldCount); + + // Assert + Assert.AreEqual(csharpSize + 8, stride); + } +} diff --git a/BuildMonitor.Test/DurationEstimatorTests.cs b/BuildMonitor.Test/DurationEstimatorTests.cs new file mode 100644 index 0000000..e18ba98 --- /dev/null +++ b/BuildMonitor.Test/DurationEstimatorTests.cs @@ -0,0 +1,296 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor.Test; + +using ktsu.Semantics.Strings; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests for . +/// +/// +/// The estimator drives the Estimate and ETA columns and the adaptive polling interval in +/// RunSync, so a mistake here shows up as wrong numbers on screen and as the wrong request +/// rate against a rate-limited API. It is pure numeric logic over a build's run history, which +/// makes it the cheapest high-value thing in this repository to test. +/// +[TestClass] +public sealed class DurationEstimatorTests +{ + /// + /// Builds a whose successful runs have the given durations, most recent + /// first, all on the same branch. + /// + private static Build BuildWithDurations(params double[] minutes) => + BuildWithDurations("main", minutes); + + private static Build BuildWithDurations(string branch, params double[] minutes) + { + Build build = new(); + DateTimeOffset start = new(2026, 1, 1, 0, 0, 0, TimeSpan.Zero); + + for (int i = 0; i < minutes.Length; i++) + { + // Most recent first: earlier entries get later start times. + DateTimeOffset started = start.AddHours(minutes.Length - i); + Run run = new() + { + Id = $"run-{i}".As(), + Name = $"run-{i}".As(), + Status = RunStatus.Success, + Started = started, + LastUpdated = started.AddMinutes(minutes[i]), + Branch = branch.As(), + }; + + _ = build.Runs.TryAdd(run.Id, run); + } + + return build; + } + + private static void AddRun(Build build, string id, RunStatus status, double minutes, string branch = "main") + { + DateTimeOffset started = new(2026, 2, 1, 0, 0, 0, TimeSpan.Zero); + Run run = new() + { + Id = id.As(), + Name = id.As(), + Status = status, + Started = started, + LastUpdated = started.AddMinutes(minutes), + Branch = branch.As(), + }; + + _ = build.Runs.TryAdd(run.Id, run); + } + + /// + /// A build with no runs cannot be estimated, and must report zero rather than guessing. + /// + [TestMethod] + public void ABuildWithNoRunsEstimatesZero() + { + // Arrange + Build build = new(); + + // Act & Assert + Assert.AreEqual(TimeSpan.Zero, DurationEstimator.EstimateDuration(build)); + } + + /// + /// Fewer than the minimum sample count must report zero rather than estimating from one or two + /// data points. + /// + /// How many successful runs the build has. + [TestMethod] + [DataRow(1)] + [DataRow(2)] + public void FewerThanThreeSamplesEstimatesZero(int sampleCount) + { + // Arrange + Build build = BuildWithDurations([.. Enumerable.Repeat(10.0, sampleCount)]); + + // Act & Assert + Assert.AreEqual(TimeSpan.Zero, DurationEstimator.EstimateDuration(build)); + } + + /// + /// Three identical samples must estimate exactly that duration, whatever weighting is applied. + /// + [TestMethod] + public void IdenticalSamplesEstimateThatExactDuration() + { + // Arrange + Build build = BuildWithDurations(10, 10, 10, 10, 10); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build); + + // Assert + Assert.AreEqual(TimeSpan.FromMinutes(10), estimate); + } + + /// + /// A single wild outlier must not drag the estimate toward it. This is the whole point of the + /// IQR filter. + /// + [TestMethod] + public void ASingleWildOutlierDoesNotDragTheEstimate() + { + // Arrange -- seven runs around ten minutes, and one that took five hours + Build build = BuildWithDurations(10, 11, 10, 9, 300, 10, 11, 10); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build); + + // Assert + Assert.IsLessThan(TimeSpan.FromMinutes(20), estimate, "The 300-minute outlier should have been filtered out."); + Assert.IsGreaterThan(TimeSpan.FromMinutes(5), estimate); + } + + /// + /// Recent runs must weigh more than older ones, so a build that has genuinely got slower is + /// estimated closer to its recent times than to its historical ones. + /// + [TestMethod] + public void RecentRunsWeighMoreThanOlderOnes() + { + // Arrange -- most recent first: the build recently doubled in duration + Build build = BuildWithDurations(20, 20, 20, 10, 10, 10); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build); + + // Assert -- an unweighted mean would be 15 minutes + Assert.IsGreaterThan(TimeSpan.FromMinutes(15), estimate, + "Exponential weighting should pull the estimate toward the recent, slower runs."); + } + + /// + /// Failed, canceled and pending runs carry no useful duration and must not be sampled. + /// + [TestMethod] + public void OnlySuccessfulRunsAreSampled() + { + // Arrange -- three successes at 10 minutes, plus noise at wildly different durations + Build build = BuildWithDurations(10, 10, 10); + AddRun(build, "failed", RunStatus.Failure, 120); + AddRun(build, "canceled", RunStatus.Canceled, 240); + AddRun(build, "pending", RunStatus.Pending, 480); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build); + + // Assert + Assert.AreEqual(TimeSpan.FromMinutes(10), estimate); + } + + /// + /// A build whose only runs are unsuccessful has nothing to estimate from. + /// + [TestMethod] + public void ABuildWithNoSuccessfulRunsEstimatesZero() + { + // Arrange + Build build = new(); + AddRun(build, "a", RunStatus.Failure, 10); + AddRun(build, "b", RunStatus.Failure, 11); + AddRun(build, "c", RunStatus.Canceled, 12); + + // Act & Assert + Assert.AreEqual(TimeSpan.Zero, DurationEstimator.EstimateDuration(build)); + } + + /// + /// When a branch has enough history of its own, its estimate must be used rather than the + /// build-wide one -- that is the point of the branch-specific overload. + /// + [TestMethod] + public void ABranchWithEnoughHistoryUsesItsOwnEstimate() + { + // Arrange -- main is fast, release is slow + Build build = BuildWithDurations("main", 5, 5, 5, 5); + AddRun(build, "rel-1", RunStatus.Success, 30, branch: "release"); + AddRun(build, "rel-2", RunStatus.Success, 30, branch: "release"); + AddRun(build, "rel-3", RunStatus.Success, 30, branch: "release"); + + // Act + TimeSpan releaseEstimate = DurationEstimator.EstimateDuration(build, "release".As()); + + // Assert + Assert.AreEqual(TimeSpan.FromMinutes(30), releaseEstimate); + } + + /// + /// A branch with too little history of its own must fall back to the build-wide estimate + /// rather than reporting zero. + /// + [TestMethod] + public void ABranchWithTooLittleHistoryFallsBackToTheBuildEstimate() + { + // Arrange -- four runs on main, a single one on a feature branch + Build build = BuildWithDurations("main", 10, 10, 10, 10); + AddRun(build, "feat-1", RunStatus.Success, 45, branch: "feature"); + + // Act + TimeSpan featureEstimate = DurationEstimator.EstimateDuration(build, "feature".As()); + + // Assert -- not zero, and not the lone 45-minute sample + Assert.AreNotEqual(TimeSpan.Zero, featureEstimate); + Assert.IsLessThan(TimeSpan.FromMinutes(45), featureEstimate); + } + + /// + /// A branch that has never run must fall back rather than failing. + /// + [TestMethod] + public void AnUnknownBranchFallsBackToTheBuildEstimate() + { + // Arrange + Build build = BuildWithDurations("main", 10, 10, 10, 10); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build, "never-built".As()); + + // Assert + Assert.AreEqual(TimeSpan.FromMinutes(10), estimate); + } + + /// + /// Estimation must be deterministic: the same history must always produce the same number, or + /// the Estimate column would flicker between frames. + /// + [TestMethod] + public void EstimationIsDeterministic() + { + // Arrange + Build build = BuildWithDurations(12, 9, 14, 11, 40, 10, 13); + + // Act + TimeSpan first = DurationEstimator.EstimateDuration(build); + TimeSpan second = DurationEstimator.EstimateDuration(build); + TimeSpan third = DurationEstimator.EstimateDuration(build); + + // Assert + Assert.AreEqual(first, second); + Assert.AreEqual(second, third); + } + + /// + /// The estimate must stay inside the range of the samples it was drawn from -- a weighted + /// average that escaped its own inputs would be a bug. + /// + [TestMethod] + public void TheEstimateStaysWithinTheSampleRange() + { + // Arrange + double[] durations = [8, 12, 10, 14, 9, 11, 13]; + Build build = BuildWithDurations(durations); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build); + + // Assert + Assert.IsGreaterThanOrEqualTo(TimeSpan.FromMinutes(durations.Min()), estimate); + Assert.IsLessThanOrEqualTo(TimeSpan.FromMinutes(durations.Max()), estimate); + } + + /// + /// An ongoing run has no final duration and must never be sampled, or the estimate would be + /// dragged toward however long the run happens to have been going. + /// + [TestMethod] + public void OngoingRunsAreNotSampled() + { + // Arrange + Build build = BuildWithDurations(10, 10, 10); + AddRun(build, "running", RunStatus.Running, 999); + + // Act + TimeSpan estimate = DurationEstimator.EstimateDuration(build); + + // Assert + Assert.AreEqual(TimeSpan.FromMinutes(10), estimate); + } +} diff --git a/BuildMonitor.sln b/BuildMonitor.sln index 3cb3321..c1ce98e 100644 --- a/BuildMonitor.sln +++ b/BuildMonitor.sln @@ -5,6 +5,8 @@ VisualStudioVersion = 17.10.35013.160 MinimumVisualStudioVersion = 10.0.40219.1 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BuildMonitor", "BuildMonitor\BuildMonitor.csproj", "{37FACB08-B293-478F-8EC9-4D52618FD4E7}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BuildMonitor.Test", "BuildMonitor.Test\BuildMonitor.Test.csproj", "{6E5F1A2C-9B34-4D71-8E62-5A0C7D3B9F41}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -15,6 +17,10 @@ Global {37FACB08-B293-478F-8EC9-4D52618FD4E7}.Debug|Any CPU.Build.0 = Debug|Any CPU {37FACB08-B293-478F-8EC9-4D52618FD4E7}.Release|Any CPU.ActiveCfg = Release|Any CPU {37FACB08-B293-478F-8EC9-4D52618FD4E7}.Release|Any CPU.Build.0 = Release|Any CPU + {6E5F1A2C-9B34-4D71-8E62-5A0C7D3B9F41}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {6E5F1A2C-9B34-4D71-8E62-5A0C7D3B9F41}.Debug|Any CPU.Build.0 = Debug|Any CPU + {6E5F1A2C-9B34-4D71-8E62-5A0C7D3B9F41}.Release|Any CPU.ActiveCfg = Release|Any CPU + {6E5F1A2C-9B34-4D71-8E62-5A0C7D3B9F41}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/BuildMonitor/AssemblyInfo.cs b/BuildMonitor/AssemblyInfo.cs new file mode 100644 index 0000000..45267e9 --- /dev/null +++ b/BuildMonitor/AssemblyInfo.cs @@ -0,0 +1,3 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("ktsu.BuildMonitor.Test")] diff --git a/BuildMonitor/BuildMonitor.cs b/BuildMonitor/BuildMonitor.cs index 960120f..3672999 100644 --- a/BuildMonitor/BuildMonitor.cs +++ b/BuildMonitor/BuildMonitor.cs @@ -182,18 +182,12 @@ private static float GetColumnWidth(string columnName) private static unsafe int? ProbeNativeImGuiTableColumnSize() { int csharpSize = sizeof(ImGuiTableColumn); + int widthGivenOffset; int narrowFieldBytes = 0; try { - if (Marshal.OffsetOf(nameof(ImGuiTableColumn.WidthGiven)).ToInt32() != WidthGivenOffset) - { - Log.Warning( - $"ImGuiTableColumn.WidthGiven is not at the expected offset {WidthGivenOffset}, so the " + - "struct layout is not one this workaround understands. Column width persistence is " + - "disabled. See BuildMonitor#258."); - return null; - } + widthGivenOffset = Marshal.OffsetOf(nameof(ImGuiTableColumn.WidthGiven)).ToInt32(); foreach (string fieldName in NarrowlyBoundColumnFields) { @@ -221,28 +215,67 @@ private static float GetColumnWidth(string columnName) return null; } - // One byte each: the binding is still narrow, so native is this many bytes wider. - if (narrowFieldBytes == NarrowlyBoundColumnFields.Length) + int? stride = ResolveNativeColumnStride(csharpSize, widthGivenOffset, narrowFieldBytes); + + if (stride is null) { - return csharpSize + ImGuiTableColumnSizeDifference; + Log.Warning( + $"The ImGuiTableColumn layout was not recognised (size {csharpSize}, WidthGiven at " + + $"{widthGivenOffset}, index fields occupying {narrowFieldBytes} bytes). Column width " + + "persistence is disabled rather than guessing at the stride. See BuildMonitor#258."); } - - // Two bytes each: the binding has been fixed and now matches native, so sizeof is correct - // and the workaround should be removed along with this whole probe. - if (narrowFieldBytes == NarrowlyBoundColumnFields.Length * 2) + else if (stride == csharpSize) { Log.Info( "Hexa.NET.ImGui now binds the ImGuiTableColumn index fields at their native width. " + "The struct size workaround is no longer needed and can be removed. See BuildMonitor#258."); - return csharpSize; } - Log.Warning( - $"The ImGuiTableColumn index fields occupy {narrowFieldBytes} bytes, which matches neither the " + - $"known-narrow binding ({NarrowlyBoundColumnFields.Length}) nor a corrected one " + - $"({NarrowlyBoundColumnFields.Length * 2}). Column width persistence is disabled rather than " + - "guessing at the stride. See BuildMonitor#258."); - return null; + return stride; + } + + /// + /// Decides the native stride of ImGuiTableColumn from a measured layout. + /// + /// The value of sizeof(ImGuiTableColumn). + /// The measured byte offset of the WidthGiven field. + /// + /// The total bytes the fields in occupy. + /// + /// + /// The native stride, or when the layout is not one this workaround + /// understands and reading it would be unsafe. + /// + /// + /// This is deliberately separate from and takes + /// its measurements as plain integers. The probe needs unsafe, reflection and a real + /// Hexa.NET.ImGui type, none of which a test can vary; the decision it makes is the part worth + /// testing, and expressed this way every branch -- including the ones that cannot be reached + /// with the binding currently referenced -- is reachable from a unit test. + /// + /// Measuring the affected fields is also deliberate rather than comparing + /// against a constant. A size threshold only ever infers the bug: + /// it cannot tell a fixed binding from one whose struct grew for an unrelated reason, and the + /// "native is about 112 bytes" figure this code used to carry does not even agree with the + /// measured 108 + 8. Counting the bytes those eight fields actually occupy tests the bug + /// itself, so a fixed binding is recognised as fixed and anything else is refused. + /// + internal static int? ResolveNativeColumnStride(int csharpSize, int widthGivenOffset, int narrowFieldBytes) + { + if (widthGivenOffset != WidthGivenOffset) + { + return null; + } + + // One byte each: the binding is still narrow, so native is this many bytes wider. + if (narrowFieldBytes == NarrowlyBoundColumnFields.Length) + { + return csharpSize + ImGuiTableColumnSizeDifference; + } + + // Two bytes each: the binding has been fixed and now matches native, so sizeof is correct + // and the workaround should be removed along with this whole probe. + return narrowFieldBytes == NarrowlyBoundColumnFields.Length * 2 ? csharpSize : null; } private static unsafe void SaveColumnWidth(string columnName, int columnIndex) diff --git a/CLAUDE.md b/CLAUDE.md index afe3c15..ab17a78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,6 +8,12 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co # Build the solution dotnet build +# Run all tests +dotnet test + +# Run a single test by name filter +dotnet test --filter "FullyQualifiedName~EstimationIsDeterministic" + # Run the application dotnet run --project BuildMonitor/BuildMonitor.csproj @@ -268,10 +274,16 @@ is 116. `ProbeNativeImGuiTableColumnSize` runs once from a static initializer and measures **the bug itself** — how many bytes those eight fields actually occupy — rather than inferring it from a size -threshold. Eight bytes means the binding is still narrow and the stride is `sizeof + 8`; sixteen -means it has been fixed, so `sizeof` is correct and the whole workaround can go; anything else, a -missing field, a moved `WidthGiven`, or a `Marshal` failure disables column-width persistence and -logs a warning. +threshold. The measuring and the deciding are deliberately separate: the probe needs `unsafe`, +reflection and a real Hexa.NET.ImGui type, none of which a test can vary, so it hands three plain +integers to `ResolveNativeColumnStride`, which is `internal` and covered by `ColumnStrideTests`. +That is what makes the corrected-binding and unrecognised-layout branches reachable from a test at +all — neither can be produced with the binding currently referenced. + +The rule it applies: eight bytes means the binding is still narrow and the stride is `sizeof + 8`; +sixteen means it has been fixed, so `sizeof` is correct and the whole workaround can go; anything +else, a missing field, a moved `WidthGiven`, or a `Marshal` failure disables column-width +persistence and logs a warning. This deliberately replaced two `Debug.Assert` calls. Those are compiled out of a Release build, so a fixed binding or a reordered struct would have left Release silently applying an offset that no @@ -490,6 +502,29 @@ BuildId buildId = workflowId.ToString().As(); ### Estimation - **DurationEstimator.cs**: Statistical duration estimation with IQR outlier removal and exponential weighting +## Testing + +`BuildMonitor.Test` uses **MSTest.Sdk** with the Microsoft Testing Platform, targeting `net10.0`. +The application project exposes its internals to it via `InternalsVisibleTo` in +`BuildMonitor/AssemblyInfo.cs`. + +Most of this application cannot be unit tested — the UI needs a live ImGui context and the +providers need credentials and network. What *is* covered is the provider-independent logic that +decides what the user sees and how hard the APIs get hit: + +- **`DurationEstimatorTests`** — the sample floor, the IQR outlier filter, exponential weighting + toward recent runs, that only completed successful runs are sampled, branch-specific estimation + and its fallback, determinism, and that the estimate stays inside its own sample range. +- **`ColumnStrideTests`** — the ImGuiTableColumn layout decision described above. + +When adding a test that needs a `Build`, note that `DurationEstimator` orders samples by `Started` +descending, so a fixture must set distinct `Started` values for "most recent" to mean anything; +`DurationEstimatorTests.BuildWithDurations` does this and is the pattern to copy. + +Anything genuinely worth testing that is currently tangled up with ImGui or a provider is usually +worth extracting into a plain method first, the way `ResolveNativeColumnStride` was — the +extraction is what makes it testable, and the pure function is easier to reason about besides. + ## Provider Implementation Details ### GitHub Provider