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 ba7b20f..3672999 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,173 @@ 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 widthGivenOffset; + int narrowFieldBytes = 0; + + try + { + widthGivenOffset = Marshal.OffsetOf(nameof(ImGuiTableColumn.WidthGiven)).ToInt32(); + + 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; + } - // 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."); + int? stride = ResolveNativeColumnStride(csharpSize, widthGivenOffset, narrowFieldBytes); - return nativeSize; + if (stride is null) + { + 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."); + } + 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 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) { - // 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 +292,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..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 @@ -260,6 +266,32 @@ 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. 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 +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: @@ -470,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