Add a Codacy badge to README.md#12
Closed
codacy-badger wants to merge 1 commit into
Closed
Conversation
cptkoolbeenz
added a commit
that referenced
this pull request
May 18, 2026
Pass 2 new findings (post-pass-1): Bug fixes: - Channel toggle may disable (#1, importance 9): Toggle() blindly inverts the channel's current state. If the channel was already enabled (e.g., persisted UI state from a prior bench run), the call would disable it and the rest of the flow (Start streaming, graph proof-of-life, Disconnect) would fail or observe no data. Read the current ToggleState and only Toggle() when it is not already On, so the channel is set deterministically. - Stale UIA element reused (#9): caching the connected-devices list AutomationElement once and reusing it for both the appearance wait (post-connect) and the removal wait (post-disconnect) can deadlock the polling loop if UI Automation invalidates the element across a tree refresh (the WaitFor catch keeps swallowing the stale-element exception until timeout). Add a FindListChildren helper that re-locates the list element on every poll iteration; use it in both WaitFor predicates so a fresh element is queried each tick. Rule violations: - ConnectStreamDisconnect_HappyPath naming (#7): the test method name contained an underscore, which doesn't match the repo's PascalCase member naming convention. Renamed to ConnectStreamDisconnectHappyPath. - detail line exceeds 120 (#8): the interpolated "Last exception" detail string was being built on a single line >120 chars in both polling helpers (WaitForTopLevelWindow and WaitFor). Extracted a shared BuildTimeoutMessage helper that constructs the suffix across two lines, both of which stay within the 120-column limit. - Inline if-return / control-flow (from /improve): the WaitFor helper had `if (condition()) return;` on a single line, which violates the Allman-brace convention. Expanded to a properly braced block. SKIPPED (with rationale): - Graph non-zero data unverified (review #3, persistent finding): asserting non-zero data on a live OxyPlot/LiveCharts surface is structurally not solvable from the FlaUI side - UI Automation does not expose plot series data. Closing the gap requires an app-side test hook (e.g., AutomationProperties.HelpText bound to the live point count, or a debug-only automation label), which is outside the scope of the test-scaffold PR. Tracked under #531 follow-up. The current proxy assertions (IsEnabled, IsFalse IsOffscreen, non-zero BoundingRectangle, pre-stream geometry captured) catch the "graph never rendered" failure mode and are the best UIA-only proxy available without app changes. - "Poll for streaming start state" (suggestions, importance 3): the proposed change replaces Thread.Sleep with WaitFor and adds Stop-button-enabled to the predicate. The current code intentionally uses a dwell time (STREAMING_DWELL_TIME = 3s) to allow data to accumulate on the live graph, not to wait for an unrelated "started" signal. Replacing it with WaitFor on Stop-button-enabled would exit immediately on Start (the Stop button enables synchronously), which defeats the purpose of letting samples arrive. Keeping the dwell. Will revisit if Qodo re-raises this with stronger justification. Persistent-comment-only findings from pass 1 (#2, #5, #6, #11, #12): Qodo's persistent comment still lists these against the OLD file paths (ConnectStreamDisconnectTest.cs without 's'); the actual fixes landed in pass 1. They should age out as Qodo re-scans on the next pass.
5 tasks
cptkoolbeenz
added a commit
that referenced
this pull request
May 18, 2026
Pass 4 new findings: Bug fixes: - Disconnect check false positive (#8): FindListChildren returned Array.Empty when the list element was missing, and the disconnect WaitFor predicate just checked Length == 0 - so a missing list (e.g. an AutomationId regression or a stale UIA tree) was indistinguishable from an empty list, and the test would pass for the wrong reason. Replaced with a ListItemSnapshot record that carries both ListFound and ItemCount, and made the disconnect predicate require ListFound && ItemCount == 0. Requirement gap: - Device-missing path fails test (#1): the env-var gate handles the no-bench-rig case, but if DAQIFI_BENCH_DEVICE_AVAILABLE=1 is set and the device fails to actually appear in the connected- devices list (powered off, cable disconnected, USB stack glitch), the test was failing (Assert.Fail) instead of skipping (Assert.Inconclusive). #531 compliance requires Phase 2 to skip on no-discoverable-device. Added a WaitForOrInconclusive helper that has the same poll semantics as WaitFor but reports timeout via Assert.Inconclusive; used it for the post-connect device- appearance wait. Refactored WaitFor + WaitForOrInconclusive to share a private TryPoll loop so the two paths can't diverge. Suggestions (/improve): - Filter list rows correctly (importance 8) + Count real list items only (importance 7): renamed FindListChildren -> FindListItems and switched from FindAllChildren() (which returns every visual child, including scrollbars and item-container headers) to FindAllDescendants(ControlType.ListItem) with a DataItem fallback for DataGrid-style rows. This is the same fix that closes both improve suggestions in one change. SKIPPED: - Wait for streaming readiness (importance 3): same as pass-2's "Poll for streaming start state" - the dwell is intentional to let unobservable graph points accumulate, not a poor-man's wait for an unrelated UI signal. Replacing it with Stop-button-enabled polling would return immediately on Start (the Stop button enables synchronously) and defeat the purpose. Persistent comment carry-over (review #4 AddDeviceButtonId naming, #5 Inline catch/finally braces, plus the #12-#15 carry-overs): all reference the OLD file path `ConnectStreamDisconnectTest.cs` (no trailing s). The actual constants in the new ConnectStreamDisconnectTests.cs are already SCREAMING_SNAKE_CASE, and the teardown uses Allman braces via UIAppLifecycle. CloseAppGracefully. These are stale Qodo-snapshot carry-overs from earlier passes that the persistent comment hasn't retired.
cptkoolbeenz
added a commit
that referenced
this pull request
May 18, 2026
…fe counts
Pass 5 new findings (both /improve, high-value):
Importance 9 - Assert visible update during streaming:
Replaced the fixed Thread.Sleep(STREAMING_DWELL_TIME) with a
WaitFor that polls until the live graph's BoundingRectangle
changes from its pre-stream baseline (while remaining onscreen
and enabled). This gives us:
- A concrete proof-of-life signal ("the plot actually updated")
instead of just "we waited 3s and the control is still there"
- An early exit when the change is visible quickly (typically
< 1s on the bench) instead of always burning the full dwell
- A specific failure message ("did not visibly update") that
points at the real problem instead of just timing out
The dwell still bounds the wait at STREAMING_DWELL_TIME (3s) so
we don't extend the worst-case test runtime. Bounded poll, same
budget, better signal.
Importance 7 - Avoid virtualization false item counts:
The pass-4 FindListItems counted realized ListItem / DataItem
descendants only. WPF UI virtualization can leave items
unrealized in the UIA tree until they scroll into view, so a
ListBox with 10 items but only 5 currently realized would report
ItemCount=5 (or 0 if all are offscreen). For the device list this
mostly doesn't matter (typically a couple of devices, all
realized), but the predicate is now used for both appearance and
disappearance checks - a false 0 would be a false-positive pass.
Restructured FindListItems to try, in order:
1. GridPattern.RowCount - logical row count, virtualization-safe
2. AsListBox().Items.Length - same, for ListBox-derived controls
3. ListItem descendants - last-resort fallback (current behavior)
4. DataItem descendants - DataGrid fallback
Convergence trajectory: pass 5 marked 11 of the prior 13 review
items as Resolved. Only remaining "active" findings in the review
rollup are #6 (Graph non-zero data unverified - structurally
unfixable, documented skip) and the #12-#17 carry-overs against
the OLD ConnectStreamDisconnectTest.cs path (no trailing s) that
Qodo's persistent comment hasn't retired. Real review-surface
state is now effectively converged - any actual code-quality
issues found post-pass-1 have been addressed.
cptkoolbeenz
added a commit
that referenced
this pull request
May 18, 2026
Pass 6 new findings (all /improve):
Importance 9 - Record baseline before starting stream:
The pass-5 change kept the original ordering: invoke Start, then
capture preStreamRect. WPF data-binding can push the first frame
during the Invoke() call, so the rectangle we captured "before
the dwell" could already reflect post-stream geometry - the diff
then compares a moved baseline against itself, producing a
false-fail ("did not visibly update") even when streaming
worked. Moved the FindByAutomationId + preStreamRect capture
BEFORE start.Invoke() to close the race.
Importance 7 - Avoid counting non-data rows:
WPF DataGrids include the column-header band as a row in
GridPattern.RowCount. Without correction, an empty grid reports
RowCount=1 (just the header), the disconnect predicate keeps
seeing ItemCount > 0, and the WaitFor times out. Detect a Header
descendant and subtract one from RowCount; clamp at 0 so a non-
DataGrid with stray Header noise can't go negative. Also fixes
a build error from the previous pass: GridPattern.RowCount is
AutomationProperty<int>, so .Value is needed to get the int.
Importance 5 - Ensure skip cannot fall through:
MainWindowSmokeTests was missing the "return; // unreachable"
guard after Assert.Inconclusive in the exePath-null branch
(though Inconclusive throws, the compiler can't prove that, so
the null-suppression operator `!` was needed downstream). Added
the explicit return and dropped the suppression operator on
both MainWindowSmokeTests.exePath and ConnectStreamDisconnect
Tests.exePath. ConnectStreamDisconnectTests already had the
return on the env-var path; this aligns the exePath path with
the same pattern.
Status: pass 5 marked 11 of 13 prior review items as Resolved.
Pass 6 introduces no new bug-level findings - all 3 items
above are /improve suggestions and address subtle but real
issues (race condition, header miscounting, control-flow
ergonomics). The persistent comment still carries pass-1
items #12-#17 against the OLD ConnectStreamDisconnectTest.cs
path; these are Qodo-rollup artifacts, not active findings
against HEAD.
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.
No description provided.