Fix PR #180's main-side CI regressions (Octave + MATLAB E-I)#181
Merged
Conversation
PR #180 introduced MATLAB-only classdef syntax that breaks the Octave test suite: the class fails to load, taking test_notification_center_pane down with it. - Property type-constraint `IsAttached logical = false` (Octave classdef has no inline type validation) -> plain default + comment. - `Event.empty` property defaults and method assignments (no ClassName.empty in Octave) -> `[]`, which behaves identically under isempty / numel / indexing / concatenation in every use here. - Comma-list expansion over classdef object arrays ([events.Severity], [events.StartTime]) and arrayfun over them (Octave supports neither) -> explicit index loops, matching the pane's existing text-filter loop. Also drops Event.empty from the test double (StubEventStore) and the test's empty-case assertion. Verified 18/18 pass in both Octave 7+ (octave-cli) and MATLAB R2020b+; mlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR #180 (Phase 1040: Companion Notification Center) added a notification bell to the FastSenseCompanion toolbar, growing it from a 1x9 to a 1x10 grid: the bell was inserted at col 8, pushing the flex spacer 8->9 and the gear 9->10 (all documented in FastSenseCompanion.m). #180 updated TestFastSenseCompanion and TestCompanionEventViewer but missed TestFastSenseCompanionPlantLogToolbar, whose findToolbarGrid_ still matched a 9-column grid and whose assertions still expected the gear at col 9. The grid was never found (verifyNotEmpty failed) and the gear-column check failed, turning MATLAB Tests (E-I) red on main and on every PR that merges with main. Updates the test to the shipped 1x10 layout {110,110,110,130,70,90,70,70,'1x',36}: col 8 = bell (70), col 9 = flex spacer, col 10 = gear (36). Test-only change matching the intentional, self-consistent implementation. Verified 11/11 pass in MATLAB R2020b+ (the two previously-failing methods testToolbarGridIs1x5 + testSettingsButtonMovedToCol5 now green); mlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
HanSur94
changed the title
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
1 participant
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.
Problem
PR #180 (Phase 1040: Companion Notification Center) merged to
mainand turned two CI checks red — onmainitself and on every open PR that merges withmain(e.g. #178). Confirmed it's amainregression, not PR-specific:main@c02e34d6(just before Phase 1040: Companion Notification Center — acknowledgeable inbox in the Event Viewer #180): green.main@2a191c0b(Phase 1040: Companion Notification Center — acknowledgeable inbox in the Event Viewer #180 merge): Octave Tests + MATLAB Tests (E-I) both red.This PR fixes both. (Originally split as #181 + #182, but each half alone left the other check red — neither was independently green — so they're consolidated here into one mergeable PR.)
Fix 1 — Octave Tests (
NotificationCenterPane.m)#180's new class uses MATLAB-only
classdefsyntax; Octave can't load it, sotest_notification_center_paneerrors and takes the suite down.IsAttached logical = falseEvent.empty(defaults + assignments)ClassName.empty[](identical underisempty/numel/indexing/concat)[events.Severity],[events.StartTime]arrayfun(@(e) e.Id, events, …)arrayfunover classdef object arraysLoops mirror the pane's existing text-filter loop. Also drops
Event.emptyfromStubEventStoreand the test's empty-case assertion.Fix 2 — MATLAB Tests (E-I) (
TestFastSenseCompanionPlantLogToolbar.m)#180 added the notification bell to the companion toolbar, growing it 1×9 → 1×10 (bell @ col 8; spacer 8→9; gear 9→10, documented in
FastSenseCompanion.m). #180 updatedTestFastSenseCompanion/TestCompanionEventViewerbut missedTestFastSenseCompanionPlantLogToolbar, whosefindToolbarGrid_matched only 9 columns (grid never found →verifyNotEmptyfailed) and whose gear-column assertion expected col 9.Updates the test to the shipped 1×10 layout
{110,110,110,130,70,90,70,70,'1x',36}. Test-only; matches the intentional implementation.Verification (local)
test_notification_center_pane→ 18/18 (was: class wouldn't load). CI Octave Tests already confirmed PASS on this branch.test_notification_center_pane18/18;TestFastSenseCompanionPlantLogToolbar11/11 (the two previously-failing methods now green).mlint/ Code Analyzer: clean on both files.Not addressed (pre-existing flake, unrelated)
MATLAB Tests (Q-Z)can showTestTagPerfRegression/testConsumerMigrationTickGateerroring — a wall-clock perf gate (bench_consumer_migration_tick: 17.5% > 10%, 648.9ms vs 762.2ms). It's timing-sensitive CI-load flake, touches nothing in this PR, and passes on re-run. Left alone.🤖 Generated with Claude Code