fix(octave): guard isvalid calls so Octave CI passes#146
Merged
Conversation
Octave 7+ has no isvalid() for classdef handles, so the three unguarded isvalid(w) / isvalid(w.FastSenseObj) calls in DashboardEngine line 2022 and 2045/2048 broke Octave CI (test_engine_preview_nbuckets_reset). Inline an isOctave flag and short-circuit the check on Octave; behaviour unchanged on MATLAB.
Octave 7+ has no isvalid() for classdef handles, so the unguarded isvalid(obj.FastSenseObj) check in autoScaleY_ broke every test that renders a FastSenseWidget or calls setYLimitMode (testDetectStaleWithFrozenSensor, testRefreshTriggers*, test_set_y_limit_mode_*, test_dashboard_preview_envelope). Inline an isOctave flag and short-circuit on Octave.
Octave 7+ has no isvalid() for classdef handles. Once autoScaleY_ is fixed, dashboard tests proceed to addYLimitButtons_ which calls syncYLimitButtonsState_ — the unguarded isvalid(w) on the cached widget handle would then throw. Inline isOctave and short-circuit on Octave.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'FastSense Performance'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: e19b113 | Previous: 17a9ff3 | Ratio |
|---|---|---|---|
Downsample mean std(1M) |
0.022 ms |
0.006 ms |
3.67 |
Instantiation mean std(1M) |
1.339 ms |
1.207 ms |
1.11 |
Render mean std(1M) |
1.546 ms |
1.153 ms |
1.34 |
Downsample mean std(5M) |
0.086 ms |
0.026 ms |
3.31 |
Instantiation mean std(5M) |
0.455 ms |
0.319 ms |
1.43 |
Zoom cycle mean std(5M) |
0.552 ms |
0.463 ms |
1.19 |
Zoom cycle mean ( std00M) |
0.783 ms |
0.622 ms |
1.26 |
Render mean ( std00M) |
96.912 ms |
10.524 ms |
9.21 |
Zoom cycle mean ( std00M) |
0.807 ms |
0.622 ms |
1.30 |
Dashboard create+render mean |
1134.795 ms |
974.447 ms |
1.16 |
Dashboard live tick stdmean |
2.571 ms |
1.295 ms |
1.99 |
Dashboard page switch stdmean |
1.917 ms |
0.081 ms |
23.67 |
Dashboard broadcastTimeRange stdmean |
0.174 ms |
0.143 ms |
1.22 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @HanSur94
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.
Summary
Octave 7+ does not implement
isvalid()for classdef handles — every unguardedisvalid(...)call on a user-defined handle throws'isvalid' undefined. The project already has a self-handle polyfill (DashboardEngine.isObjValid_), but several call sites on non-self handles (widgets, FastSense objects, cached widget refs) bypass it. The latest Octave CI run (https://github.com/HanSur94/FastSense/actions/runs/25863120537) reports these as the dominant failure mode.Smallest-possible surgical patch: at each unguarded site, inline
isOctave = exist('OCTAVE_VERSION', 'builtin') ~= 0;and short-circuit theisvalid()check on Octave. Behaviour on MATLAB is unchanged.Sites touched (5 in 3 files)
libs/Dashboard/DashboardEngine.mrefreshActivePageWidgetsAfterResize_Pass 1~isvalid(w)->(~isOctave && ~isvalid(w))libs/Dashboard/DashboardEngine.mrefreshActivePageWidgetsAfterResize_Pass 2~isvalid(w)->(~isOctave && ~isvalid(w))libs/Dashboard/DashboardEngine.mrefreshActivePageWidgetsAfterResize_Pass 2~isvalid(w.FastSenseObj)->(~isOctave && ~isvalid(w.FastSenseObj))libs/Dashboard/FastSenseWidget.mautoScaleY_(Follow-mode guard)isvalid(obj.FastSenseObj)->(isOctave || isvalid(obj.FastSenseObj))libs/Dashboard/DashboardLayout.msyncYLimitButtonsState_(cached widget ref)isvalid(w)->(isOctave || isvalid(w))The two DashboardEngine sites in Pass 1/Pass 2 share one
isOctaveflag at the top of the function; the other two files declare the flag inline next to the guard.Octave tests this unblocks (from the failing CI log)
test_dashboard_perf_fixes::test_engine_preview_nbuckets_reset(line 2022)test_dashboard_preview_envelope(autoScaleY_at line 453, thensyncYLimitButtonsState_at 1038)test_fastsense_widget_ylimit_modes::test_set_y_limit_mode_*(5 cases, all hit line 453)test_fastsense_widget_ylimit_modes::test_follow_mode_still_short_circuits_autoscale(line 453)suite/TestFastSenseWidget::testPropertiesForwardToInnerFastSense,testRefreshTriggersRerenderOnAdded,testRefreshTriggersRerenderOnOpenToClosed(line 453)suite/TestSensorStaleDetection::testDetectStaleWithFrozenSensor(line 453)refreshActivePageWidgetsAfterResize_failures observed downstream in the same CI runWhat I deliberately did NOT touch
DashboardEngine.m:2086-2087(isWidgetLineWhite_) — already inside atry/catchblock; Octave swallows the error.DashboardEngine.m:1844, 521, 1861, 1871, 1892, 1934, 3095— all insidetry/catchor guard timer (UDD) handles that Octave handles differently.libs/FastSenseCompanion/**— companion is gated behind anotSupportedthrow on Octave (constructor line 103). The "warning: error caught while executing handle class delete method" intest_companion_shellis cosmetic (test still PASSES on Octave per CI log).libs/FastSense/HoverCrosshair.m— MATLAB-only, tests skip withSKIPPED (Octave: HoverCrosshair is MATLAB-only — uses isvalid).libs/Dashboard/DashboardToolbar.m:311/315— only reached via the Follow toolbar toggle, no current Octave test exercises it.isObjValid_self-handle polyfill — reused as-is at the entry guard ofrefreshActivePageWidgetsAfterResize_, only the per-widget probes needed changing.Test plan
test_dashboard_perf_fixesto completion (noisvalidfailure ontest_engine_preview_nbuckets_reset).test_dashboard_preview_envelopereaches the envelope assertions instead of erroring insideautoScaleY_.test_fastsense_widget_ylimit_modesruns alltest_set_y_limit_mode_*cases without thenear line 453error.isvalidcall on MATLAB).mh_style/mh_lintviolations in the three touched files (verified locally).