Skip to content

test(ui): FlaUI scaffold + Connect-Stream-Disconnect (#531)#532

Open
cptkoolbeenz wants to merge 9 commits into
mainfrom
test/flaui-scaffold
Open

test(ui): FlaUI scaffold + Connect-Stream-Disconnect (#531)#532
cptkoolbeenz wants to merge 9 commits into
mainfrom
test/flaui-scaffold

Conversation

@cptkoolbeenz
Copy link
Copy Markdown
Member

Summary

Phase 1 of the FlaUI UI-automation rollout for issue #531. Stands up the
scaffold so future UI tests have a place to live, and verifies FlaUI can
drive the WPF app under MSTest.

  • Adds FlaUI.Core + FlaUI.UIA3 5.0.0 to Daqifi.Desktop.Test.
  • New Daqifi.Desktop.Test/UITests/ folder with two tests:
    • MainWindowSmokeTest (Phase 1) — launches DAQiFi.exe, waits
      for the main window, asserts the title contains DAQiFi, then closes
      cleanly. Reports Assert.Inconclusive (SKIPPED) when the exe isn't
      built or the runner isn't elevated; never breaks normal CI.
    • ConnectStreamDisconnectTest (Phase 2, [Ignore]d) — full happy
      path stub. AutomationIds use a Daqifi.<Pane>.<Control> namespace so
      future XAML hooks are greppable.
  • No production code touched. Existing 24 test files untouched. No XAML
    annotations added yet (deferred to Phase 2 enablement).

Phase 1 vs Phase 2 status

Phase Test Status today
1 MainWindowSmokeTest.MainWindow_Launches_And_HasExpectedTitle Test run successful; reports SKIPPED on this dev box — see Surprise
2 ConnectStreamDisconnectTest.ConnectStreamDisconnect_HappyPath [Ignore]d, ready to enable once bench device + XAML AutomationIds are wired

Surprise — admin-elevation manifest blocks unattended launch

Daqifi.Desktop/app.manifest declares
requestedExecutionLevel level="requireAdministrator". Process.Start
(used internally by FlaUI.Application.Launch) returns
Win32Exception(740) "The requested operation requires elevation" unless
the test runner itself is already elevated. FlaUI cannot drive UAC.

The smoke test catches error 740 and reports Inconclusive so a normal
non-elevated CI run is informative (SKIPPED) rather than red. To exercise
the full launch path, two options for future work:

  1. Run dotnet test from an elevated terminal / CI agent, or
  2. Add an asInvoker UI-test manifest variant of the desktop app (out of
    scope for this PR — track separately if Phase 2 needs it).

What still blocks Phase 2

  • Bench device attached (USB or Wi-Fi) when the test runs.
  • XAML AutomationProperties.AutomationId="..." annotations on:
    Daqifi.Connection.AddDeviceButton, Daqifi.Connection.ConnectButton,
    Daqifi.Devices.ConnectedList, Daqifi.Channels.FirstChannelEnable,
    Daqifi.Streaming.StartButton, Daqifi.Streaming.StopButton,
    Daqifi.Connection.DisconnectButton, Daqifi.Graph.Live.
    Each XAML annotation should comment-link to this test + test(ui): FlaUI scaffold + Connect-Stream-Disconnect happy-path test #531.
  • Admin-elevation resolution (see above).

Test plan

  • dotnet build Daqifi.Desktop.Test/Daqifi.Desktop.Test.csproj — 0 errors, pre-existing warnings only
  • dotnet test --filter FullyQualifiedName~UITests — 2 skipped, run successful (Phase 1 = UAC path, Phase 2 = [Ignore])
  • Existing non-UI tests unaffected: 276/277 pass; the 1 failure is WindowsFirewallWrapperTests.CreateUdpRule_WithInvalidPort_CreatesRuleWithoutPort, pre-existing and unrelated (needs admin elevation to write firewall rules — same UAC root cause)
  • Re-run smoke test from elevated terminal to confirm the launch + title assertion fires end-to-end (deferred — needs admin shell)
  • Phase 2 enablement: annotate XAML, attach bench device, remove [Ignore], run

Closes #531 (Phase 1 scope). Phase 2 enablement tracked in the same issue or as a follow-up.

🤖 Generated with Claude Code

Phase 1 of the FlaUI UI-automation rollout. Adds FlaUI.Core + FlaUI.UIA3
(5.0.0) to Daqifi.Desktop.Test and a new UITests/ folder with:

- MainWindowSmokeTest: launches DAQiFi.exe, waits for the main window,
  asserts the title contains "DAQiFi", then closes cleanly. Reports
  Assert.Inconclusive (SKIPPED, not FAILED) when the exe isn't built
  yet or when the runner isn't elevated (the WPF app's app.manifest
  declares requireAdministrator, which Process.Start can't satisfy
  without an already-elevated parent).

- ConnectStreamDisconnectTest: Phase 2 happy-path stub (connect ->
  enable channel -> stream 3s -> stop -> disconnect). Marked [Ignore]
  pending a bench device + the XAML AutomationId annotations it
  references. AutomationIds use a "Daqifi.<Pane>.<Control>" namespace
  so future XAML hooks are greppable.

No production code changed; existing 24 test files untouched. Phase 1
test status on a non-elevated dev box is SKIPPED — expected. To exercise
the smoke test fully, either run dotnet test from an elevated terminal,
or relax app.manifest to asInvoker for the UI-test build target.
- Remove unused FlaUI.Core.Definitions import in Phase 2 test
- Drop empty inverted-bool branch in MainWindowSmokeTest teardown
- Match Phase 2 teardown to Phase 1 (5s wait between Close and Kill)
@cptkoolbeenz cptkoolbeenz requested a review from a team as a code owner May 18, 2026 01:04
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 18, 2026

❌ The PR title does not follow the Conventional Commits format. Please use one of the following prefixes: feat, fix, docs, deps, chore. For example: feat: add new feature

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Add FlaUI UI-automation scaffold with smoke and happy-path tests

🧪 Tests

Grey Divider

Walkthroughs

Description
• Add FlaUI UI-automation scaffold with Phase 1 smoke test
  - Launches DAQiFi.exe, verifies main window appears with correct title
  - Reports Inconclusive (SKIPPED) when exe not built or runner not elevated
• Include Phase 2 happy-path test stub (marked [Ignore])
  - Full connect → enable-channel → stream → disconnect workflow
  - AutomationIds use "Daqifi.<Pane>.<Control>" namespace for XAML traceability
• Add FlaUI.Core and FlaUI.UIA3 5.0.0 dependencies to test project
• No production code changes; existing 24 tests remain untouched
Diagram
flowchart LR
  A["Test Project"] -->|"Add FlaUI 5.0.0"| B["FlaUI Dependencies"]
  A -->|"Create UITests folder"| C["MainWindowSmokeTest"]
  A -->|"Create UITests folder"| D["ConnectStreamDisconnectTest"]
  C -->|"Phase 1: Launch & Verify"| E["Main Window Validation"]
  D -->|"Phase 2: Full Workflow"| F["Connect-Stream-Disconnect Path"]
  E -->|"Handle UAC Error 740"| G["Report Inconclusive"]
  F -->|"Marked Ignore"| H["Awaits Bench Device & XAML Ids"]
Loading

Grey Divider

File Changes

1. Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs 🧪 Tests +153/-0

Phase 1 main window launch and title validation test

• Phase 1 smoke test that launches DAQiFi.exe and validates main window appearance
• Catches Win32Exception error 740 (elevation required) and reports Inconclusive to avoid CI
 failures
• Implements exe discovery logic searching Debug/Release and net10.0-windows/net9.0-windows build
 outputs
• Includes graceful teardown with 5-second timeout before forcing app termination

Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs


2. Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs 🧪 Tests +186/-0

Phase 2 connect-stream-disconnect happy-path test stub

• Phase 2 happy-path test stub marked [Ignore] pending bench device and XAML AutomationId
 annotations
• Implements full workflow: connect → enable-channel → stream 3s → stop → disconnect
• Defines AutomationId constants using "Daqifi.<Pane>.<Control>" namespace for XAML traceability
• Includes helper methods for finding elements by AutomationId and polling with timeout
• Validates graph control existence and bounding box as proof-of-life for streaming

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs


3. Daqifi.Desktop.Test/Daqifi.Desktop.Test.csproj Dependencies +2/-0

Add FlaUI UI-automation framework dependencies

• Add FlaUI.Core 5.0.0 NuGet package dependency
• Add FlaUI.UIA3 5.0.0 NuGet package dependency

Daqifi.Desktop.Test/Daqifi.Desktop.Test.csproj


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: validate-pr-title

Failed stage: PR Conventional Commit Validation [❌]

Failed test name: ""

Failure summary:

The validate-pr-title job failed because the ytanikin/PRConventionalCommits@1.3.0 action rejected
the PR title's conventional commit type:
- Error at log line 52: Invalid or missing task type:
'test'. Must be one of: feat, fix, docs, deps, chore
- The workflow is configured with task_types:
["feat","fix","docs","deps","chore"] (log line 48), but the PR title uses the type test, which is
not allowed.

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

37:  VulnerabilityAlerts: read
38:  ##[endgroup]
39:  Secret source: Actions
40:  Prepare workflow directory
41:  Prepare all required actions
42:  Getting action download info
43:  Download action repository 'ytanikin/PRConventionalCommits@1.3.0' (SHA:b628c5a234cc32513014b7bfdd1e47b532124d98)
44:  Download action repository 'actions/github-script@v9' (SHA:3a2844b7e9c422d3c10d287c895573f7108da1b3)
45:  Complete job name: validate-pr-title
46:  ##[group]Run ytanikin/PRConventionalCommits@1.3.0
47:  with:
48:  task_types: ["feat","fix","docs", "deps", "chore"]
49:  add_label: false
50:  token: ***
51:  ##[endgroup]
52:  ##[error]Invalid or missing task type: 'test'. Must be one of: feat, fix, docs, deps, chore
53:  ##[group]Run actions/github-script@v9

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (1)

Context used

Grey Divider


Action required

1. Device list hidden by tabs 🐞 Bug ≡ Correctness ⭐ New
Description
ConnectStreamDisconnectHappyPath now fail-fast asserts DEVICE_LIST_ID exists under the main window,
but MainWindow only renders the selected tab’s content so the devices list will not be in the UIA
tree unless the Devices tab is selected. This will cause the Phase 2 test to fail immediately even
when AutomationIds are correct and a device is available.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R127-128]

+            _ = FindByAutomationId(mainWindow, cf, DEVICE_LIST_ID,
+                "Connected-devices list container.");
Evidence
The test unconditionally asserts the list exists under mainWindow, but the app’s TabControl
renders only selected content and the devices UI lives under the Devices tab, which is not selected
by default (SelectedIndex defaults to 0). Therefore the element cannot be found at that point even
when correctly wired.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[121-129]
Daqifi.Desktop/MainWindow.xaml[174-194]
Daqifi.Desktop/MainWindow.xaml[234-306]
Daqifi.Desktop/ViewModels/DaqifiViewModel.cs[119-124]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ConnectStreamDisconnectHappyPath` asserts the connected-devices list (`DEVICE_LIST_ID`) exists under `mainWindow`, but `MainWindow` renders only the selected tab’s content. Since the default selected tab is Live Graph, the Devices pane (and its connected-devices list) is not present in the UIA tree at that time, causing a deterministic failure.

## Issue Context
In `MainWindow.xaml`, the `TabControl` template uses `ContentSource="SelectedContent"`, meaning only the active tab is in the visual/UIA tree. The Devices UI is hosted under the Devices tab (`DevicesPanePrototype`). `DaqifiViewModel` does not initialize `_selectedIndex`, so it defaults to `0` (first tab: Live Graph).

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[121-129]

### Implementation direction
- Before calling `FindByAutomationId(... DEVICE_LIST_ID ...)`, navigate to the Devices tab via UIA (e.g., find the TabItem by name/header "Devices" and select it) and wait until the pane is loaded, then perform the fail-fast check.
- Alternatively, scope the lookup to the Devices tab’s content after selecting it (avoid asserting existence in the main window when the tab isn’t active).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Selector regression becomes skip ✓ Resolved 🐞 Bug ≡ Correctness
Description
The device-appear wait checks only ItemCount > 0, so if the connected-devices list AutomationId is
missing/regressed the predicate never becomes true and the test times out as Assert.Inconclusive,
incorrectly presenting a selector regression as “device not available”. This can silently mask
broken XAML AutomationId wiring on the bench machine.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R134-136]

+            WaitForOrInconclusive(
+                () => FindListItems(mainWindow, cf, DEVICE_LIST_ID).ItemCount > 0,
+                DEVICE_APPEAR_TIMEOUT,
Evidence
The wait predicate ignores ListFound, while FindListItems explicitly returns `(ListFound:false,
ItemCount:0)` when the list element cannot be found. This means a missing AutomationId produces the
same behavior as “list exists but empty,” and because the wait is WaitForOrInconclusive, it
becomes a skip rather than a failure.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[121-140]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[276-283]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`WaitForOrInconclusive` currently waits for a device using only `ItemCount > 0`. When the devices list element itself is missing (AutomationId not wired/regressed), `FindListItems` returns `(ListFound:false, ItemCount:0)` and the test times out as **Inconclusive**, misclassifying a selector regression as a bench-device absence.

### Issue Context
This is specifically problematic for bench runs where the env-var gate is enabled and the test is intended to validate the workflow + UI selectors.

### Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[134-140]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[276-294]

### Suggested fix
Before the poll, explicitly assert the list exists (and fail if it doesn’t), then keep the existing `WaitForOrInconclusive` to handle “device didn’t appear” as a skip:

```csharp
// Fail fast if the selector is missing/regressed.
_ = FindByAutomationId(
   mainWindow, cf, DEVICE_LIST_ID,
   "Connected devices list container.");

WaitForOrInconclusive(
   () => FindListItems(mainWindow, cf, DEVICE_LIST_ID).ItemCount > 0,
   DEVICE_APPEAR_TIMEOUT,
   ...);
```

(Alternative: change the predicate to check `snap.ListFound && snap.ItemCount > 0` and separately fail when the list is never found, but ensure it fails rather than being swallowed into `Inconclusive`.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Device-missing path fails test ✓ Resolved 📎 Requirement gap ☼ Reliability
Description
When a bench device is not actually discoverable, the test times out waiting for the
connected-device list and fails via Assert.Fail rather than skipping with Assert.Inconclusive.
This violates the requirement to skip (not fail) Phase 2 when the bench device is
unavailable/disconnected.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R125-127]

+            WaitFor(
+                () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length > 0,
+                DEVICE_APPEAR_TIMEOUT,
Evidence
PR Compliance ID 6 requires the Phase 2 test to skip (not fail) when the bench device is not
discoverable. The new code waits for the device list to become non-empty and relies on WaitFor,
which ends in Assert.Fail on timeout—causing a failure rather than Assert.Inconclusive when the
device is absent.

Skip Connect-Stream-Disconnect test when no bench device is discoverable
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[125-127]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[299-301]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ConnectStreamDisconnectHappyPath` currently fails when no bench device appears in the connected list within the timeout, but the compliance requirement is to *skip* (Inconclusive) when the device is not discoverable.

## Issue Context
The test waits for the device to appear using `WaitFor(...)`, which fails via `Assert.Fail(...)` on timeout. This makes runs on machines without the device (or with it disconnected) go red instead of being skipped.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[125-128]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[276-301]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (6)
4. Channel toggle may disable ✓ Resolved 🐞 Bug ≡ Correctness
Description
ConnectStreamDisconnect_HappyPath calls Toggle() on the first-channel control without verifying its
current state, so a pre-enabled channel will be turned off and streaming assertions can fail or
observe no data.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R125-130]

+            // ----- Enable first channel -----
+            var firstChannel = FindByAutomationId(mainWindow, cf, FIRST_CHANNEL_TOGGLE_ID,
+                "Enable-toggle on the first analog channel.");
+            // Toggle controls in MahApps are typically ToggleButtons; click via Invoke.
+            firstChannel.AsToggleButton().Toggle();
+
Evidence
The test comment and variable naming indicate the intent is to enable the channel, but the
implementation inverts state unconditionally.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[125-130]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ConnectStreamDisconnect_HappyPath` intends to **enable** the first channel, but it uses `Toggle()` blindly. If the toggle is already enabled (e.g., persisted UI state or a prior run), the call will disable it and break the rest of the flow.

## Issue Context
This test is meant to be a deterministic “happy path” bench test; it should set the channel to a known enabled state, not invert whatever state happens to exist.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[125-130]

## Suggested change
Read the current toggle state / checked state and only toggle when it is not already enabled (or explicitly set it to On if the API supports it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. AddDeviceButtonId naming noncompliant ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
New constants and private fields are named in PascalCase instead of SCREAMING_SNAKE_CASE for
constants and _camelCase for private fields, violating the repository’s C# naming conventions.
This introduces style inconsistency in newly added/changed UI test code.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R36-53]

+    private const string DesktopExeName = "DAQiFi.exe";
+    private const string DesktopProjectName = "Daqifi.Desktop";
+
+    // AutomationIds that Phase 2 expects to exist in MainWindow / dialogs.
+    // None of these are wired up yet; add them in the corresponding XAML with
+    // a comment referencing this test + #531 when enabling the test.
+    private const string AddDeviceButtonId    = "Daqifi.Connection.AddDeviceButton";
+    private const string ConnectButtonId      = "Daqifi.Connection.ConnectButton";
+    private const string DeviceListId         = "Daqifi.Devices.ConnectedList";
+    private const string FirstChannelToggleId = "Daqifi.Channels.FirstChannelEnable";
+    private const string StartStreamingId     = "Daqifi.Streaming.StartButton";
+    private const string StopStreamingId      = "Daqifi.Streaming.StopButton";
+    private const string DisconnectButtonId   = "Daqifi.Connection.DisconnectButton";
+    private const string LiveGraphId          = "Daqifi.Graph.Live";
+
+    private static readonly TimeSpan MainWindowTimeout = TimeSpan.FromSeconds(60);
+    private static readonly TimeSpan DeviceAppearTimeout = TimeSpan.FromSeconds(15);
+    private static readonly TimeSpan StreamingDwellTime = TimeSpan.FromSeconds(3);
Evidence
PR Compliance ID 8 mandates that constants use SCREAMING_SNAKE_CASE and private fields use
underscore-prefixed camelCase. In the cited UI test files, ConnectStreamDisconnectTest.cs
introduces multiple const identifiers such as AddDeviceButtonId and private static readonly
fields like MainWindowTimeout in PascalCase, and MainWindowSmokeTest.cs similarly adds constants
like DesktopExeName and private fields like MainWindowTimeout using PascalCase, demonstrating
noncompliance with the required naming patterns.

CLAUDE.md
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[36-53]
Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[30-36]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
C# naming conventions in this repo require constants to be named in SCREAMING_SNAKE_CASE and private fields to be named `_camelCase`. The newly added/changed UI tests introduce multiple `const` members and `private static readonly` fields using PascalCase, which violates PR Compliance ID 8 and creates style inconsistency.

## Issue Context
This PR adds/updates test code under `Daqifi.Desktop.Test/UITests/`, including files that define many `AutomationId` constants and timeout values as `private const` and `private static readonly` members. Update the identifiers to match the mandated naming conventions for new/changed code.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[36-53]
- Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[30-36]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. ConnectStreamDisconnectTest always ignored ✓ Resolved 📎 Requirement gap ☼ Reliability
Description
The Phase 2 happy-path test is unconditionally skipped via [Ignore] rather than using
device-dependent skip behavior. This prevents validating the workflow against a bench device when
present and does not meet the required skip semantics.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R32-34]

+[TestClass]
+[Ignore("Phase 2 - needs bench device. See issue #531; remove [Ignore] when a Nyquist is on the bench and XAML AutomationIds are wired up.")]
+public class ConnectStreamDisconnectTest
Evidence
PR Compliance ID 5 requires an end-to-end workflow test with explicit skip behavior when no device
is discoverable. The added test is unconditionally marked [Ignore], so it never runs regardless of
device availability.

Implement ConnectStreamDisconnectTest happy-path workflow with device-dependent skip behavior
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[32-34]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Connect→Stream→Disconnect test is always skipped using `[Ignore]`, but the compliance requirement calls for a test that runs when a device is available and explicitly skips when no device is discoverable.

## Issue Context
The current approach prevents the test from ever validating the workflow, even on a properly configured bench environment.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[32-34]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[55-126]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Graph non-zero data unverified 📎 Requirement gap ≡ Correctness
Description
The Phase 2 test does not assert the graph shows non-zero data points after streaming; it only
checks visibility and a non-zero bounding rectangle. This does not satisfy the required validation
of actual streamed data.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R102-112]

+            var liveGraph = FindByAutomationId(mainWindow, cf, LiveGraphId,
+                "Live graph control. Should contain non-zero point count after dwell.");
+
+            // OxyPlot / LiveCharts surfaces don't expose data points through UIA, so
+            // we settle for proof-of-life: the control exists, is visible, and has
+            // a non-trivial bounding rectangle. Strengthen this once we know which
+            // graph library is in use (search MainWindow.xaml for oxy:/lvc:).
+            Assert.IsFalse(liveGraph.IsOffscreen, "Live graph was offscreen after Start.");
+            Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0
+                          && liveGraph.BoundingRectangle.Height > 0,
+                "Live graph had zero-sized bounding box; streaming likely did not start.");
Evidence
PR Compliance ID 5 requires asserting the graph shows non-zero data points after a streaming dwell.
The code explicitly states it "settle[s] for proof-of-life" and only checks IsOffscreen and
BoundingRectangle, which does not validate non-zero data.

Implement ConnectStreamDisconnectTest happy-path workflow with device-dependent skip behavior
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[102-112]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The compliance requirement for the Phase 2 UI test includes asserting the graph shows non-zero data points after streaming. The current test only checks that the graph control exists/is visible and has a non-zero bounding box, which does not prove that data is streaming.

## Issue Context
UI Automation often cannot access plot data directly; this may require adding a test hook (e.g., a UI element exposing point count, a debug-only automation-accessible label, or another verifiable indicator) to validate non-zero samples.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[102-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. UI test files misnamed ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
New test files do not follow the required *Tests.cs naming convention (they are
MainWindowSmokeTest.cs and ConnectStreamDisconnectTest.cs). This can reduce consistency and may
conflict with repo testing conventions.
Code

Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[R1-10]

+using System;
+using System.ComponentModel;
+using System.IO;
+using System.Reflection;
+using FlaUI.Core;
+using FlaUI.Core.AutomationElements;
+using FlaUI.UIA3;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Daqifi.Desktop.Test.UITests;
Evidence
PR Compliance ID 14 requires test files to follow the *Tests.cs naming convention. The PR adds
MainWindowSmokeTest.cs and ConnectStreamDisconnectTest.cs, which do not match *Tests.cs
(evidenced by the file paths containing the declared test namespaces/classes).

CLAUDE.md
Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[1-10]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[1-12]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Testing conventions require test files to be named `*Tests.cs`. The PR adds two new UI test files that do not match this pattern.

## Issue Context
Even if MSTest discovery works via attributes, the repo standard requires consistent file naming.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[1-10]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[1-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. Inline catch/finally braces ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new teardown code uses inline catch { ... } / finally { ... } blocks instead of Allman brace
style. This breaks the required C# formatting standard for modified code.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R131-141]

+                try
+                {
+                    app.Close();
+                    // Give the app up to 5s to shut down gracefully before forcing it,
+                    // matching the Phase 1 smoke-test teardown.
+                    app.WaitWhileMainHandleIsMissing(TimeSpan.FromSeconds(5));
+                    if (!app.HasExited) app.Kill();
+                }
+                catch { /* best-effort */ }
+                finally { app.Dispose(); }
+            }
Evidence
PR Compliance ID 9 requires Allman braces. ConnectStreamDisconnectTest.cs introduces `catch { /*
best-effort */ } and finally { app.Dispose(); }`, which are inline brace blocks and therefore
non-compliant.

CLAUDE.md
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[131-141]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Repository style requires Allman braces (opening brace on a new line) and consistent block formatting. The new code uses inline brace blocks for `catch` and `finally`.

## Issue Context
This occurs in the best-effort teardown path for the UI application.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[131-141]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

10. Disconnect check false positive ✓ Resolved 🐞 Bug ≡ Correctness
Description
FindListChildren returns an empty array when the devices list element is missing, and the disconnect
wait condition treats “missing list” as “0 devices”, so the test can pass without actually verifying
the device was removed. This can mask selector/AutomationId regressions or transient UIA-tree issues
during disconnect.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R232-237]

+    private static AutomationElement[] FindListChildren(
+        AutomationElement scope, ConditionFactory cf, string automationId)
+    {
+        var element = scope.FindFirstDescendant(cf.ByAutomationId(automationId));
+        return element is null ? Array.Empty<AutomationElement>() : element.FindAllChildren();
+    }
Evidence
The helper currently converts “element not found” into an empty children array, and the disconnect
wait condition interprets empty children as success. In the current UI, the devices pane contains an
always-present ItemsControl bound to the Devices collection, so a missing list element should be
treated as an error rather than a successful “0 devices” state.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[232-237]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[190-193]
Daqifi.Desktop/View/Prototype/DevicesPanePrototype.xaml[469-477]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`FindListChildren` returns `Array.Empty<AutomationElement>()` when the list element is not found. The disconnect assertion uses `FindListChildren(...).Length == 0`, which makes “list missing” indistinguishable from “list empty”, producing a false-positive pass.

## Issue Context
This is specifically risky in the disconnect phase: a transient UIA failure, stale scope, or incorrect/missing AutomationId could make the list lookup return null, and the test would still succeed.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[190-193]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[232-237]

## Suggested fix
- Change the disconnect predicate to explicitly require the list element exists before checking its child count, e.g.:
 - `var list = mainWindow.FindFirstDescendant(cf.ByAutomationId(DEVICE_LIST_ID));`
 - `return list != null && list.FindAllChildren().Length == 0;`
- Alternatively, change `FindListChildren` to *not* return empty when the element is missing (e.g., throw/assert in contexts where absence is a failure, or return `null`/a sentinel and handle it in the predicate).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


11. detail line exceeds 120 ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
A new line building the detail message exceeds the 120-character limit, which violates the
changed-code formatting standard. This makes the code harder to read and maintain.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R241-244]

+        var detail = lastException is null
+            ? string.Empty
+            : $" Last exception while enumerating top-level windows: {lastException.GetType().Name}: {lastException.Message}";
+        Assert.Fail($"{failureMessage} (waited {timeout.TotalSeconds:F0}s).{detail}");
Evidence
PR Compliance ID 29 requires modified lines to remain within 120 characters. The newly added
detail message construction contains overlong lines in both polling helpers.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[241-244]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[268-271]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `detail` assignment uses a very long interpolated string line, exceeding the 120-character limit.

## Issue Context
The repository formatting standard for changed code caps lines at 120 characters.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[241-244]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[268-271]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


12. ConnectStreamDisconnect_HappyPath naming ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
The new test method name ConnectStreamDisconnect_HappyPath uses underscores, which does not meet
the repo’s stated PascalCase member naming convention. This reduces consistency and discoverability
across the test suite.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R70-73]

+    [TestMethod]
+    [TestCategory("UI-Bench")]
+    public void ConnectStreamDisconnect_HappyPath()
+    {
Evidence
PR Compliance ID 9 requires methods to use PascalCase. The newly added [TestMethod] is named with
an underscore, violating that convention.

CLAUDE.md
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[70-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test method name uses underscores (`ConnectStreamDisconnect_HappyPath`), which is not strict PascalCase per the naming convention rule.

## Issue Context
This file is newly added UI automation scaffolding; keeping names consistent improves searchability and maintainability.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[70-73]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (4)
13. Stale UIA element reused ✓ Resolved 🐞 Bug ☼ Reliability
Description
ConnectStreamDisconnect_HappyPath caches the connected-devices list AutomationElement and reuses it
later after major UI transitions; if the element becomes stale, polling will repeatedly hit UIA
exceptions and eventually time out even when the device list is correctly updated.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[R119-123]

+            // Wait for the connected-devices list to show at least one row.
+            var deviceList = FindByAutomationId(mainWindow, cf, DEVICE_LIST_ID,
+                "Connected-devices list container.");
+            WaitFor(() => deviceList.FindAllChildren().Length > 0, DEVICE_APPEAR_TIMEOUT,
+                "Device did not appear in the connected list.");
Evidence
The code captures deviceList once, then queries it both to detect appearance and later removal;
the polling helper catches exceptions (which is good), but that also means a stale element can keep
throwing until timeout.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[119-123]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[171-177]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[248-272]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test stores `deviceList` once and uses it in multiple `WaitFor` calls across connect/stream/disconnect. UI Automation elements can become invalid across tree refreshes, which can turn these waits into timeouts.

## Issue Context
Your `WaitFor` helper intentionally catches transient UIA exceptions and only reports the last one on timeout; if `deviceList` becomes stale, the condition may never evaluate successfully.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[119-123]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[171-177]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[248-272]

## Suggested change
Avoid reusing a long-lived cached `AutomationElement` for list polling. Options:
- Re-locate the list before the second wait (after disconnect), and/or
- In the wait predicate, re-find the list each poll iteration (without asserting) and query children on the fresh element.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


14. Dialog searched in main window ✓ Resolved 🐞 Bug ≡ Correctness
Description
ConnectStreamDisconnectTest searches for the Connect button under the main window even though the
app opens a separate ConnectionDialog window, so the element lookup will fail when this test is
enabled. This blocks the happy-path test at the first connect step.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R74-82]

+            // ----- Connect -----
+            var addDevice = FindByAutomationId(mainWindow, cf, AddDeviceButtonId,
+                "Add-device entry point (USB/Serial picker).");
+            addDevice.AsButton().Invoke();
+
+            var connect = FindByAutomationId(mainWindow, cf, ConnectButtonId,
+                "Confirm button on the connection dialog.");
+            connect.AsButton().Invoke();
+
Evidence
The app opens a dedicated ConnectionDialog window for connection, so searching within the main
window cannot find dialog-scoped controls like the Connect button.

Daqifi.Desktop/ViewModels/DaqifiViewModel.cs[1066-1072]
Daqifi.Desktop/View/ConnectionDialog.xaml[1-14]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[74-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ConnectStreamDisconnectTest` clicks the Add Device entry point and then tries to locate `ConnectButtonId` as a descendant of `mainWindow`. In the app, Add Device opens a separate `ConnectionDialog` window, so the Connect button will not be in the main window’s UIA tree.

### Issue Context
The production app shows the connection UI via `_dialogService.ShowDialog<ConnectionDialog>(...)`, which creates a new top-level `MetroWindow`.

### Fix
After invoking Add Device:
1. Locate the `ConnectionDialog` top-level window (e.g., by title containing `CONNECT DEVICE`, by window class/name, or by waiting for a second top-level window).
2. Search for `ConnectButtonId` within that dialog window instead of `mainWindow`.
3. Optionally wait for the dialog to close after invoking Connect.

### Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[74-88]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


15. [Ignore] line exceeds 120 ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
ConnectStreamDisconnectTest has an [Ignore(...)] attribute message on a single line that exceeds
the 120-column limit. This violates the repo’s changed-code formatting standards and reduces
readability/style compliance.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[33]

+[Ignore("Phase 2 - needs bench device. See issue #531; remove [Ignore] when a Nyquist is on the bench and XAML AutomationIds are wired up.")]
Evidence
PR Compliance ID 42 requires changed code to remain within a 120-column limit. The [Ignore(...)]
attribute at line 33 is a single long line that exceeds this limit.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[33-33]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `[Ignore(...)]` attribute text is on a single line exceeding the 120-column limit, violating the changed-code formatting standards.

## Issue Context
This PR adds new UI test scaffolding; changed code is expected to follow the repo’s 120-column limit.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[33-33]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


16. No 740 skip in Phase2 ✓ Resolved 🐞 Bug ☼ Reliability
Description
ConnectStreamDisconnectTest launches DAQiFi.exe without handling Win32 error 740 even though the app
manifest requires admin elevation, so once [Ignore] is removed the test will fail on non-elevated
runners instead of reporting Inconclusive like the smoke test.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R65-71]

+        Application? app = null;
+        try
+        {
+            app = Application.Launch(exePath!);
+            using var automation = new UIA3Automation();
+            var mainWindow = app.GetMainWindow(automation, MainWindowTimeout);
+            Assert.IsNotNull(mainWindow, "Main window did not appear.");
Evidence
The app manifest explicitly requires elevation; the smoke test handles error 740 as Inconclusive,
but the Phase 2 test does not, so it will throw/fail in the same environment the smoke test
intentionally skips.

Daqifi.Desktop/app.manifest[15-21]
Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[54-71]
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[55-70]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ConnectStreamDisconnectTest` calls `Application.Launch(...)` without handling the elevation-required failure path (Win32Exception NativeErrorCode 740). The smoke test already treats this case as `Assert.Inconclusive`, but Phase 2 will throw/fail when enabled on non-elevated runners.

## Issue Context
- The desktop app’s manifest requests `requireAdministrator`, which triggers `ERROR_ELEVATION_REQUIRED (740)` when launched from a non-elevated test runner.
- `MainWindowSmokeTest` already catches 740 and marks the test inconclusive.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[65-73]
- Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[54-71]

## Suggested fix
Wrap `Application.Launch(exePath!)` in a try/catch matching the smoke test:
- `catch (Win32Exception ex) when (ex.NativeErrorCode == 740)`
- `Assert.Inconclusive("Skipped: ... requires administrator elevation ...")`
Optionally, extract a shared helper (e.g., `TryLaunchOrInconclusive(exePath)`) used by both tests so the behavior stays consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

17. Teardown wait uses main-handle ✓ Resolved 🐞 Bug ☼ Reliability
Description
The UI tests attempt to “give the app up to 5s to shut down gracefully” but wait using
WaitWhileMainHandleIsMissing, which is a main-window-handle helper rather than an explicit
process-exit wait. This can make teardown timing depend on handle semantics and risks killing the
app before shutdown cleanup completes.
Code

Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[R92-100]

+                    app.Close();
+                    // Close() requests graceful shutdown; give the app up to 5s
+                    // to exit on its own. If it hasn't, force it so the next
+                    // test run starts from a clean slate.
+                    app.WaitWhileMainHandleIsMissing(TimeSpan.FromSeconds(5));
+                    if (!app.HasExited)
+                    {
+                        app.Kill();
+                    }
Evidence
The production app wires cleanup into the window close path (including device disconnects). The test
teardown’s comment indicates it intends to allow graceful shutdown, so using an explicit
process-exit wait better matches that intent and reduces the chance of interrupting cleanup.

Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[92-101]
Daqifi.Desktop/MainWindow.xaml.cs[24-35]
Daqifi.Desktop/ViewModels/DaqifiViewModel.cs[1113-1120]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Teardown currently uses `app.WaitWhileMainHandleIsMissing(TimeSpan.FromSeconds(5))` to represent a graceful shutdown grace period. This couples teardown to window-handle behavior and may not reliably wait for the process to exit.

### Issue Context
The app performs cleanup on window closing (e.g., executing `Shutdown`, which disconnects devices). Abrupt kills can bypass this path.

### Fix
Replace the handle-based wait with an explicit process-exit wait:
- After `app.Close()`, wait up to 5 seconds for the process to exit (e.g., via the underlying `Process.WaitForExit(5000)` if exposed, or a polling loop on `app.HasExited` with a timeout).
- Only call `Kill()` after that timeout expires.

### Fix Focus Areas
- Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[92-101]
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[133-138]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


18. Exe locator prefers Debug always 🐞 Bug ☼ Reliability
Description
ConnectStreamDisconnectTest.TryLocateDesktopExe searches Debug before Release unconditionally, so if
both builds exist it may drive the wrong/stale binary and produce misleading UI-test results.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R173-185]

+    private static string? TryLocateDesktopExe()
+    {
+        var testAssemblyDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)
+                              ?? Directory.GetCurrentDirectory();
+        var repoRoot = Path.GetFullPath(Path.Combine(testAssemblyDir, "..", "..", "..", ".."));
+        var configs = new[] { "Debug", "Release" };
+        var tfms = new[] { "net10.0-windows", "net9.0-windows" };
+        return (from c in configs
+                from t in tfms
+                let p = Path.Combine(repoRoot, DesktopProjectName, "bin", c, t, DesktopExeName)
+                where File.Exists(p)
+                select p).FirstOrDefault();
+    }
Evidence
The Phase 2 test hardcodes Debug-first, while the smoke test is explicitly configuration-aware; this
makes Phase 2 more likely to pick the wrong binary when both outputs are present.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[173-185]
Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[127-149]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TryLocateDesktopExe` in `ConnectStreamDisconnectTest` always searches `Debug` before `Release`, regardless of whether the test assembly itself was built in Debug or Release. This can cause the test to run against an unexpected build when both outputs exist.

## Issue Context
`MainWindowSmokeTest` already has configuration-aware preference logic (`#if DEBUG` selects Debug-first, otherwise Release-first). Phase 2 should follow the same behavior to reduce confusion.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[173-185]
- Daqifi.Desktop.Test/UITests/MainWindowSmokeTest.cs[127-149]

## Suggested fix
Adopt the same `#if DEBUG` config preference logic used in `MainWindowSmokeTest`, and iterate configs/tfms in that order (instead of a fixed `new[] { "Debug", "Release" }`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


19. WaitFor hides root cause ✓ Resolved 🐞 Bug ◔ Observability
Description
WaitFor swallows all exceptions during polling and discards them, so when a UI condition never
becomes true the test fails without the underlying UIA error details, making failures significantly
harder to debug.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R155-170]

+    private static void WaitFor(Func<bool> condition, TimeSpan timeout, string failureMessage)
+    {
+        var deadline = DateTime.UtcNow + timeout;
+        while (DateTime.UtcNow < deadline)
+        {
+            try
+            {
+                if (condition()) return;
+            }
+            catch
+            {
+                // Swallow transient UIA errors while controls are still spinning up.
+            }
+            Thread.Sleep(200);
+        }
+        Assert.Fail(failureMessage + $" (waited {timeout.TotalSeconds:F0}s)");
Evidence
The polling loop catches any exception type and throws it away, and the eventual failure message
only includes the generic failureMessage + timeout, so diagnostic details are lost.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[155-171]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`WaitFor` catches all exceptions and ignores them. If the condition never succeeds, the final `Assert.Fail` does not include any context about repeated UIA failures that may have prevented the condition from becoming true.

## Issue Context
The method comment says these are “transient UIA errors”, but fully dropping the exception details makes real breakages (wrong AutomationId, control not ready, UIA provider crash) much harder to diagnose.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[155-171]

## Suggested fix
Capture exceptions while still tolerating transients, e.g.:
- keep a `Exception? lastException` (or bounded list)
- in `catch (Exception ex) { lastException = ex; }`
- on timeout, append `lastException.ToString()` (or a summary count) to the failure message.
Also consider only swallowing known transient exceptions if you can identify them, and rethrow unexpected ones.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit acbd357

Results up to commit 9197d29


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (1)


Action required
1. Graph non-zero data unverified 📎 Requirement gap ≡ Correctness
Description
The Phase 2 test does not assert the graph shows non-zero data points after streaming; it only
checks visibility and a non-zero bounding rectangle. This does not satisfy the required validation
of actual streamed data.
Code

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[R102-112]

+            var liveGraph = FindByAutomationId(mainWindow, cf, LiveGraphId,
+                "Live graph control. Should contain non-zero point count after dwell.");
+
+            // OxyPlot / LiveCharts surfaces don't expose data points through UIA, so
+            // we settle for proof-of-life: the control exists, is visible, and has
+            // a non-trivial bounding rectangle. Strengthen this once we know which
+            // graph library is in use (search MainWindow.xaml for oxy:/lvc:).
+            Assert.IsFalse(liveGraph.IsOffscreen, "Live graph was offscreen after Start.");
+            Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0
+                          && liveGraph.BoundingRectangle.Height > 0,
+                "Live graph had zero-sized bounding box; streaming likely did not start.");
Evidence
PR Compliance ID 5 requires asserting the graph shows non-zero data points after a streaming dwell.
The code explicitly states it "settle[s] for proof-of-life" and only checks IsOffscreen and
BoundingRectangle, which does not validate non-zero data.

Implement ConnectStreamDisconnectTest happy-path workflow with device-dependent skip behavior
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[102-112]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The compliance requirement for the Phase 2 UI test includes asserting the graph shows non-zero data points after streaming. The current test only checks that the graph control exists/is visible and has a non-zero bounding box, which does not prove that data is streaming.

## Issue Context
UI Automation often cannot access plot data directly; this may require adding a test hook (e.g., a UI element exposing point count, a debug-only automation-accessible label, or another verifiable indicator) to validate non-zero samples.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[102-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation t...

Comment thread Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs Outdated
Comment thread Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs Outdated
Comment thread Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs Outdated
Comment on lines +102 to +112
var liveGraph = FindByAutomationId(mainWindow, cf, LiveGraphId,
"Live graph control. Should contain non-zero point count after dwell.");

// OxyPlot / LiveCharts surfaces don't expose data points through UIA, so
// we settle for proof-of-life: the control exists, is visible, and has
// a non-trivial bounding rectangle. Strengthen this once we know which
// graph library is in use (search MainWindow.xaml for oxy:/lvc:).
Assert.IsFalse(liveGraph.IsOffscreen, "Live graph was offscreen after Start.");
Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0
&& liveGraph.BoundingRectangle.Height > 0,
"Live graph had zero-sized bounding box; streaming likely did not start.");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

4. Graph non-zero data unverified 📎 Requirement gap ≡ Correctness

The Phase 2 test does not assert the graph shows non-zero data points after streaming; it only
checks visibility and a non-zero bounding rectangle. This does not satisfy the required validation
of actual streamed data.
Agent Prompt
## Issue description
The compliance requirement for the Phase 2 UI test includes asserting the graph shows non-zero data points after streaming. The current test only checks that the graph control exists/is visible and has a non-zero bounding box, which does not prove that data is streaming.

## Issue Context
UI Automation often cannot access plot data directly; this may require adding a test hook (e.g., a UI element exposing point count, a debug-only automation-accessible label, or another verifiable indicator) to validate non-zero samples.

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[102-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread Daqifi.Desktop.Test/UITests/MainWindowSmokeTests.cs
@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/improve

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 9197d29

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

PR Code Suggestions ✨

Latest suggestions up to acbd357

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent orphaned UI test processes

Add fallback process termination (app.Kill()) in the catch block of the teardown
logic.

Daqifi.Desktop.Test/UITests/UIAppLifecycle.cs [82-119]

 public static void CloseAppGracefully(Application? app, TimeSpan grace)
 {
     if (app is null)
     {
         return;
     }
+
+    var pid = app.ProcessId;
 
     try
     {
         app.Close();
 
         var deadline = DateTime.UtcNow + grace;
         while (DateTime.UtcNow < deadline && !app.HasExited)
         {
             Thread.Sleep(100);
         }
 
         if (!app.HasExited)
         {
             app.Kill();
-            // Kill() is asynchronous - the OS posts the termination but the
-            // process can still hold file/socket handles for a moment after.
-            // Block here so callers see a fully-exited process when
-            // Dispose() runs, preventing stray processes between consecutive
-            // test runs. FlaUI's Application doesn't expose the underlying
-            // Process directly, but we can fetch it by ProcessId.
-            WaitForExitByProcessId(app.ProcessId, grace);
+            WaitForExitByProcessId(pid, grace);
         }
     }
     catch
     {
         // Best-effort teardown - never let cleanup mask the real failure.
+        try
+        {
+            if (!app.HasExited)
+            {
+                app.Kill();
+                WaitForExitByProcessId(pid, grace);
+            }
+        }
+        catch
+        {
+            // Swallow secondary teardown failures.
+        }
     }
     finally
     {
         app.Dispose();
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: If app.Close() throws (which is common in UI Automation), the empty catch block swallows the error and skips app.Kill(), potentially leaking the process and failing subsequent tests.

Medium
Detect redraws via screenshots

Use element screenshots to detect redraws instead of relying on bounding
rectangle changes.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [191-220]

 var liveGraph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
     "Live graph control. Should visibly update after streaming starts.");
 Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0,
     "Pre-stream graph rectangle was empty; the control wasn't laid out before Start.");
-var preStreamRect = liveGraph.BoundingRectangle;
+
+var preStreamImage = liveGraph.Capture();
+var preStreamBytes = preStreamImage.ToByteArray();
 
 var start = FindByAutomationId(mainWindow, cf, START_STREAMING_ID,
     "Start-streaming command button.");
 start.AsButton().Invoke();
 
-// Poll-and-detect rather than fixed-sleep: WaitFor returns as soon
-// as we observe a visible change to the graph (and skips the
-// remaining dwell), but if nothing has changed by the deadline we
-// still spent the same wall-clock budget as the old Thread.Sleep
-// and get a precise failure message ("did not visibly update").
 WaitFor(
     () =>
     {
         var graph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
             "Live graph control (streaming-update poll).");
-        var rect = graph.BoundingRectangle;
-        return !graph.IsOffscreen
-               && graph.IsEnabled
-               && rect.Width > 0
-               && rect.Height > 0
-               && !rect.Equals(preStreamRect);
+        if (graph.IsOffscreen || !graph.IsEnabled)
+        {
+            return false;
+        }
+
+        var img = graph.Capture();
+        var bytes = img.ToByteArray();
+        return !bytes.SequenceEqual(preStreamBytes);
     },
     STREAMING_DWELL_TIME,
     "Live graph did not visibly update during the streaming dwell; "
     + "streaming may not have started or data is not reaching the plot.");
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: While it is true that BoundingRectangle might not change on standard data updates, using Capture() can introduce flakiness due to rendering differences or require non-standard extensions like .ToByteArray() which do not natively exist on System.Drawing.Bitmap.

Low
General
Wait for window readiness

Ensure the window is ready for interaction before returning the matched Window
handle.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [360-389]

 private static Window WaitForTopLevelWindow(
     Application app, UIA3Automation automation, string titleFragment,
     TimeSpan timeout, string failureMessage)
 {
     var deadline = DateTime.UtcNow + timeout;
     Exception? lastException = null;
     while (DateTime.UtcNow < deadline)
     {
         try
         {
             var match = app.GetAllTopLevelWindows(automation)
                 .FirstOrDefault(w => (w.Title ?? string.Empty)
                     .Contains(titleFragment, StringComparison.OrdinalIgnoreCase));
-            if (match is not null)
+
+            if (match is not null && match.IsEnabled && !match.IsOffscreen)
             {
                 return match;
             }
         }
         catch (Exception ex)
         {
             lastException = ex;
         }
 
         Thread.Sleep(200);
     }
 
     Assert.Fail(BuildTimeoutMessage(failureMessage, timeout,
         "enumerating top-level windows", lastException));
     throw new InvalidOperationException("unreachable; Assert.Fail throws.");
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: Adding readiness checks such as IsEnabled and !IsOffscreen is a good practice to prevent flakiness caused by window transition animations (especially common with MahApps).

Medium
  • More

Previous suggestions

Suggestions up to commit 69f939e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve assertion semantics in polling

Re-throw test assertion exceptions immediately in TryPoll rather than catching
them and waiting for a timeout.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [422-447]

 private static bool TryPoll(
     Func<bool> condition, TimeSpan timeout, out Exception? lastException)
 {
     lastException = null;
     var deadline = DateTime.UtcNow + timeout;
     while (DateTime.UtcNow < deadline)
     {
         try
         {
             if (condition())
             {
                 return true;
             }
         }
+        catch (UnitTestAssertException)
+        {
+            // Never convert an explicit assert into a timeout.
+            throw;
+        }
         catch (Exception ex)
         {
             // Swallow but remember transient UIA errors while controls are still
             // spinning up; if we eventually time out, surface the last error so
             // the failure message points at the real root cause.
             lastException = ex;
         }
+
         Thread.Sleep(200);
     }
 
     return false;
 }
Suggestion importance[1-10]: 8

__

Why: Catching UnitTestAssertException in a polling loop hides immediate test failures behind slow timeouts, obscuring the true cause of failure.

Medium
Prevent PID-reuse teardown hazards

Check the process start time during teardown to prevent interacting with an
unrelated process due to rapid PID recycling.

Daqifi.Desktop.Test/UITests/UIAppLifecycle.cs [82-119]

 public static void CloseAppGracefully(Application? app, TimeSpan grace)
 {
     if (app is null)
     {
         return;
     }
 
+    DateTime? startTime = null;
     try
     {
+        startTime = TryGetProcessStartTime(app.ProcessId);
+
         app.Close();
 
         var deadline = DateTime.UtcNow + grace;
         while (DateTime.UtcNow < deadline && !app.HasExited)
         {
             Thread.Sleep(100);
         }
 
         if (!app.HasExited)
         {
             app.Kill();
-            // Kill() is asynchronous - the OS posts the termination but the
-            // process can still hold file/socket handles for a moment after.
-            // Block here so callers see a fully-exited process when
-            // Dispose() runs, preventing stray processes between consecutive
-            // test runs. FlaUI's Application doesn't expose the underlying
-            // Process directly, but we can fetch it by ProcessId.
-            WaitForExitByProcessId(app.ProcessId, grace);
+            WaitForExitByProcessId(app.ProcessId, startTime, grace);
         }
     }
     catch
     {
         // Best-effort teardown - never let cleanup mask the real failure.
     }
     finally
     {
         app.Dispose();
     }
 }
 
+private static DateTime? TryGetProcessStartTime(int processId)
+{
+    try
+    {
+        using var process = Process.GetProcessById(processId);
+        return process.StartTime.ToUniversalTime();
+    }
+    catch
+    {
+        return null;
+    }
+}
+
+private static void WaitForExitByProcessId(int processId, DateTime? expectedStartTime, TimeSpan grace)
+{
+    try
+    {
+        using var process = Process.GetProcessById(processId);
+        if (expectedStartTime.HasValue &&
+            process.StartTime.ToUniversalTime() != expectedStartTime.Value)
+        {
+            return;
+        }
+
+        process.WaitForExit((int)grace.TotalMilliseconds);
+    }
+    catch (ArgumentException)
+    {
+        // Process already exited - no further wait needed.
+    }
+}
+
Suggestion importance[1-10]: 3

__

Why: While theoretically possible, PID recycling in the milliseconds between Kill() and fetching the process by ID is extremely unlikely, making this optimization only marginally beneficial.

Low
✅ Suggestions up to commit 004c857
CategorySuggestion                                                                                                                                    Impact
Possible issue
Record baseline before starting stream
Suggestion Impact:The test was reordered to capture the live graph bounding-rectangle baseline before clicking Start Streaming, with added comments explaining the flake/race condition being prevented.

code diff:

-            // ----- Start streaming, dwell, check graph has data -----
-            var start = FindByAutomationId(mainWindow, cf, START_STREAMING_ID,
-                "Start-streaming command button.");
-            start.AsButton().Invoke();
-
-            // Capture graph geometry BEFORE streaming so we have a baseline to
-            // diff against. UI Automation can't see OxyPlot / LiveCharts plot
-            // data directly, so the best UIA-only proxy for "data is arriving"
-            // is "the live graph's bounding rectangle changed during the
-            // dwell". Once the XAML grows an automation-visible point-count
-            // probe (tracked under #531) this can become a hard assertion.
+            // ----- Capture graph baseline, start streaming, check for update -----
+            // Capture graph geometry BEFORE invoking Start so we have a
+            // baseline to diff against. UI Automation can't see OxyPlot /
+            // LiveCharts plot data directly, so the best UIA-only proxy for
+            // "data is arriving" is "the live graph's bounding rectangle
+            // changed after streaming started". Once the XAML grows an
+            // automation-visible point-count probe (tracked under #531) this
+            // can become a hard assertion.
+            //
+            // The baseline MUST be captured before invoking Start: otherwise
+            // the WPF data-binding could have already pushed the first sample
+            // by the time we read the rectangle, and the post-stream diff
+            // would compare against an already-updated baseline (false-fail).
             var liveGraph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
                 "Live graph control. Should visibly update after streaming starts.");
             Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0,
                 "Pre-stream graph rectangle was empty; the control wasn't laid out before Start.");
             var preStreamRect = liveGraph.BoundingRectangle;
+
+            var start = FindByAutomationId(mainWindow, cf, START_STREAMING_ID,
+                "Start-streaming command button.");
+            start.AsButton().Invoke();
 

Move the baseline graph capture before invoking the start streaming button. This
prevents race conditions where the graph might have already updated before the
baseline is recorded.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [170-184]

-var start = FindByAutomationId(mainWindow, cf, START_STREAMING_ID,
-    "Start-streaming command button.");
-start.AsButton().Invoke();
-
 // Capture graph geometry BEFORE streaming so we have a baseline to
 // diff against. UI Automation can't see OxyPlot / LiveCharts plot
 // data directly, so the best UIA-only proxy for "data is arriving"
 // is "the live graph's bounding rectangle changed during the
 // dwell". Once the XAML grows an automation-visible point-count
 // probe (tracked under #531) this can become a hard assertion.
 var liveGraph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
     "Live graph control. Should visibly update after streaming starts.");
 Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0,
     "Pre-stream graph rectangle was empty; the control wasn't laid out before Start.");
 var preStreamRect = liveGraph.BoundingRectangle;
 
+var start = FindByAutomationId(mainWindow, cf, START_STREAMING_ID,
+    "Start-streaming command button.");
+start.AsButton().Invoke();
+

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: Capturing the baseline after the action has been invoked introduces a race condition that can lead to flaky UI tests.

High
Avoid counting non-data rows
Suggestion Impact:Updated the GridPattern.RowCount-based item count logic to subtract a detected header row and clamp the result to >= 0, preventing empty-grid checks from timing out.

code diff:

         // Prefer GridPattern.RowCount when available - it reports the logical
-        // row count regardless of virtualization.
+        // row count regardless of virtualization. WPF DataGrids commonly count
+        // their header band as a row in GridPattern; if a Header descendant
+        // exists, subtract one so an empty grid can reach ItemCount=0
+        // (otherwise the disconnect predicate would never see 0 and would
+        // time out).
         var gridPattern = element.Patterns.Grid.PatternOrDefault;
         if (gridPattern is not null)
         {
-            return new ListItemSnapshot(ListFound: true, ItemCount: gridPattern.RowCount);
+            // GridPattern.RowCount is AutomationProperty<int>; .Value reads
+            // the actual count via UIA.
+            var rowCount = gridPattern.RowCount.Value;
+            var header = element.FindFirstDescendant(cf.ByControlType(ControlType.Header));
+            if (header is not null && rowCount > 0)
+            {
+                rowCount--;
+            }
+            return new ListItemSnapshot(ListFound: true, ItemCount: Math.Max(0, rowCount));
         }

Account for non-data header rows in GridPattern.RowCount. Detect and subtract
headers to accurately return the data item count.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [295-299]

 var gridPattern = element.Patterns.Grid.PatternOrDefault;
 if (gridPattern is not null)
 {
-    return new ListItemSnapshot(ListFound: true, ItemCount: gridPattern.RowCount);
+    var rowCount = gridPattern.RowCount;
+
+    // Some WPF grids/data-grids report header rows via GridPattern.
+    // If we detect a header, discount it so "empty list" can reach 0.
+    var header = element.FindFirstDescendant(cf.ByControlType(ControlType.Header));
+    if (header is not null && rowCount > 0)
+    {
+        rowCount--;
+    }
+
+    return new ListItemSnapshot(ListFound: true, ItemCount: Math.Max(0, rowCount));
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: WPF DataGrids frequently include header rows in the UI Automation GridPattern.RowCount, which would incorrectly block the empty list assertion and cause a timeout.

Medium
General
Ensure skip cannot fall through
Suggestion Impact:The commit added a return statement after Assert.Inconclusive and changed the LaunchOrInconclusive call to pass exePath without using the null-forgiving operator.

code diff:

@@ -47,12 +47,13 @@
                 $"Skipped: {DESKTOP_EXE_NAME} was not found. Build the {DESKTOP_PROJECT_NAME} " +
                 "project (Debug or Release, net10.0-windows) before running this UI test. " +
                 "See issue #531 for the full FlaUI scaffold rollout plan.");
+            return; // unreachable; Assert.Inconclusive throws.
         }
 
         Application? app = null;
         try
         {
-            app = UIAppLifecycle.LaunchOrInconclusive(exePath!);
+            app = UIAppLifecycle.LaunchOrInconclusive(exePath);
 

Add a return; statement directly after Assert.Inconclusive to prevent continued
execution with a null exePath.

Daqifi.Desktop.Test/UITests/MainWindowSmokeTests.cs [44-55]

 if (exePath is null)
 {
     Assert.Inconclusive(
         $"Skipped: {DESKTOP_EXE_NAME} was not found. Build the {DESKTOP_PROJECT_NAME} " +
         "project (Debug or Release, net10.0-windows) before running this UI test. " +
         "See issue #531 for the full FlaUI scaffold rollout plan.");
+    return;
 }
 
 Application? app = null;
 try
 {
-    app = UIAppLifecycle.LaunchOrInconclusive(exePath!);
+    app = UIAppLifecycle.LaunchOrInconclusive(exePath);

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: Adding an explicit return ensures consistent control flow and removes the need for the null-suppression operator (!) when invoking UIAppLifecycle.LaunchOrInconclusive.

Low
✅ Suggestions up to commit 25afc80
CategorySuggestion                                                                                                                                    Impact
Possible issue
Assert visible update during streaming
Suggestion Impact:The commit removed Thread.Sleep and the post-dwell assertions, and added a WaitFor polling loop that repeatedly re-finds the live graph and checks it is onscreen/enabled with a non-zero bounding rectangle that differs from the pre-stream rectangle, failing with an explicit "did not visibly update" message.

code diff:

-            Thread.Sleep(STREAMING_DWELL_TIME);
-
-            // Re-fetch in case the surface was lazy-bound on Start.
-            liveGraph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
-                "Live graph control (post-dwell).");
-
-            Assert.IsFalse(liveGraph.IsOffscreen, "Live graph was offscreen after Start.");
-            Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0
-                          && liveGraph.BoundingRectangle.Height > 0,
-                "Live graph had zero-sized bounding box; streaming likely did not start.");
-
-            // Proxy data-arrival check: enabled-state should remain visible and the
-            // graph's rectangle stayed non-empty across the dwell. A stronger
-            // assertion needs an AutomationProperties.HelpText (or similar) bound
-            // to the live point count - tracked under #531 follow-up.
-            Assert.IsTrue(liveGraph.IsEnabled,
-                "Live graph was disabled after streaming dwell; streaming likely did not start.");
-            Assert.IsTrue(preStreamRect.Width > 0,
-                "Pre-stream graph rectangle was empty; the control wasn't laid out before Start.");
+            // Poll-and-detect rather than fixed-sleep: WaitFor returns as soon
+            // as we observe a visible change to the graph (and skips the
+            // remaining dwell), but if nothing has changed by the deadline we
+            // still spent the same wall-clock budget as the old Thread.Sleep
+            // and get a precise failure message ("did not visibly update").
+            WaitFor(
+                () =>
+                {
+                    var graph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
+                        "Live graph control (streaming-update poll).");
+                    var rect = graph.BoundingRectangle;
+                    return !graph.IsOffscreen
+                           && graph.IsEnabled
+                           && rect.Width > 0
+                           && rect.Height > 0
+                           && !rect.Equals(preStreamRect);
+                },
+                STREAMING_DWELL_TIME,
+                "Live graph did not visibly update during the streaming dwell; "
+                + "streaming may not have started or data is not reaching the plot.");
 

Replace the fixed Thread.Sleep with a polling loop that checks for the bounding
rectangle to change, ensuring that a visual update actually occurred.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [182-202]

 var preStreamRect = liveGraph.BoundingRectangle;
 
-Thread.Sleep(STREAMING_DWELL_TIME);
+WaitFor(
+    () =>
+    {
+        var graph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID, "Live graph control (poll).");
+        var rect = graph.BoundingRectangle;
+        return !graph.IsOffscreen
+               && graph.IsEnabled
+               && rect.Width > 0
+               && rect.Height > 0
+               && rect != preStreamRect;
+    },
+    STREAMING_DWELL_TIME,
+    "Live graph did not visibly update during the streaming dwell; streaming may not have started.");
 
-// Re-fetch in case the surface was lazy-bound on Start.
 liveGraph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
     "Live graph control (post-dwell).");
 
-Assert.IsFalse(liveGraph.IsOffscreen, "Live graph was offscreen after Start.");
-Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0
-              && liveGraph.BoundingRectangle.Height > 0,
-    "Live graph had zero-sized bounding box; streaming likely did not start.");
-
-// Proxy data-arrival check: enabled-state should remain visible and the
-// graph's rectangle stayed non-empty across the dwell. A stronger
-// assertion needs an AutomationProperties.HelpText (or similar) bound
-// to the live point count - tracked under #531 follow-up.
-Assert.IsTrue(liveGraph.IsEnabled,
-    "Live graph was disabled after streaming dwell; streaming likely did not start.");
-Assert.IsTrue(preStreamRect.Width > 0,
-    "Pre-stream graph rectangle was empty; the control wasn't laid out before Start.");
-

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: Replacing the fixed Thread.Sleep with a polling mechanism that validates the expected bounding rectangle change fulfills the author's documented intent and speeds up the test.

High
Avoid virtualization false item counts
Suggestion Impact:Updated FindListItems to prefer GridPattern.RowCount and ListBox.Items.Length with a try/catch fallback, only using descendant counting (ListItem/DataItem) as a last resort to mitigate virtualization issues.

code diff:

     /// <summary>
     /// Re-finds the list element by AutomationId and counts its data rows.
     /// Always re-locates the parent on each call so a stale AutomationElement
-    /// from a prior UI-tree refresh can't poison a polling loop. Counts only
-    /// <c>ListItem</c> / <c>DataItem</c> descendants rather than every visual
-    /// child, because a WPF ItemsControl's child collection includes
-    /// scrollbars, item-container headers, etc., which would otherwise let
-    /// <c>Length &gt; 0</c> succeed even when no data rows exist.
+    /// from a prior UI-tree refresh can't poison a polling loop.
+    ///
+    /// Counts use UI Automation patterns first (Grid.RowCount or
+    /// ListBox.Items.Length) because WPF UI virtualization can leave rows
+    /// unrealized (offscreen list items are not present as descendants in the
+    /// UIA tree until they scroll into view) - so counting realized
+    /// descendants alone would report 0 even when the list has items. The
+    /// realized-descendant fallback (ListItem / DataItem) covers the case
+    /// where the control isn't a ListBox / Grid but still exposes its items
+    /// directly.
     /// </summary>
     private static ListItemSnapshot FindListItems(
         AutomationElement scope, ConditionFactory cf, string automationId)
@@ -282,13 +290,36 @@
             return new ListItemSnapshot(ListFound: false, ItemCount: 0);
         }
 
+        // Prefer GridPattern.RowCount when available - it reports the logical
+        // row count regardless of virtualization.
+        var gridPattern = element.Patterns.Grid.PatternOrDefault;
+        if (gridPattern is not null)
+        {
+            return new ListItemSnapshot(ListFound: true, ItemCount: gridPattern.RowCount);
+        }
+
+        // ListBox.Items.Length is similarly virtualization-safe; AsListBox
+        // throws if the element isn't a ListBox, so guard with try/catch
+        // rather than a fragile control-type sniff.
+        try
+        {
+            var listBox = element.AsListBox();
+            return new ListItemSnapshot(ListFound: true, ItemCount: listBox.Items.Length);
+        }
+        catch
+        {
+            // Fall through to the realized-descendant fallback.
+        }
+
+        // Last-resort fallback: count realized ListItem / DataItem descendants.
+        // This loses accuracy under virtualization but works for non-virtualized
+        // ItemsControls and DataGrids.
         var listItems = element.FindAllDescendants(cf.ByControlType(ControlType.ListItem));
         if (listItems.Length > 0)
         {
             return new ListItemSnapshot(ListFound: true, ItemCount: listItems.Length);
         }
 
-        // DataGrid rows typically present as DataItem rather than ListItem.
         var dataItems = element.FindAllDescendants(cf.ByControlType(ControlType.DataItem));
         return new ListItemSnapshot(ListFound: true, ItemCount: dataItems.Length);

Use UIA patterns (like GridPattern.RowCount or AsListBox().Items.Length) to
count items robustly, as descendant counting is unreliable with UI
virtualization.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [276-294]

 private static ListItemSnapshot FindListItems(
     AutomationElement scope, ConditionFactory cf, string automationId)
 {
     var element = scope.FindFirstDescendant(cf.ByAutomationId(automationId));
     if (element is null)
     {
         return new ListItemSnapshot(ListFound: false, ItemCount: 0);
     }
 
+    // Prefer UIA patterns over realized descendants to avoid WPF virtualization
+    // causing false "0 items" when rows are offscreen/unrealized.
+    var gridPattern = element.Patterns.Grid.PatternOrDefault;
+    if (gridPattern is not null)
+    {
+        return new ListItemSnapshot(ListFound: true, ItemCount: gridPattern.RowCount);
+    }
+
+    try
+    {
+        var listBox = element.AsListBox();
+        return new ListItemSnapshot(ListFound: true, ItemCount: listBox.Items.Length);
+    }
+    catch
+    {
+        // Fall back to realized descendants below.
+    }
+
     var listItems = element.FindAllDescendants(cf.ByControlType(ControlType.ListItem));
     if (listItems.Length > 0)
     {
         return new ListItemSnapshot(ListFound: true, ItemCount: listItems.Length);
     }
 
-    // DataGrid rows typically present as DataItem rather than ListItem.
     var dataItems = element.FindAllDescendants(cf.ByControlType(ControlType.DataItem));
     return new ListItemSnapshot(ListFound: true, ItemCount: dataItems.Length);
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: Using UIA patterns instead of descendant counting avoids potential issues with UI virtualization reporting incorrect item counts.

Medium
✅ Suggestions up to commit 17ab383
CategorySuggestion                                                                                                                                    Impact
Possible issue
Filter list rows correctly
Suggestion Impact:Replaced FindListChildren/FindAllChildren() with a new FindListItems helper that counts only ListItem or (fallback) DataItem descendants, preventing false positives from non-row visual children; test predicates were updated to use the new item count.

code diff:

@@ -123,10 +123,21 @@
             // elements can become stale across major tree transitions (e.g.
             // when the ConnectionDialog closes), and a cached element that's
             // gone stale will keep throwing inside WaitFor until timeout.
-            WaitFor(
-                () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length > 0,
+            //
+            // If the device never appears within the timeout, treat it as
+            // "bench device not actually discoverable" and skip
+            // (Assert.Inconclusive) rather than fail - the env-var gate told
+            // us a device was *expected*, but the connect path can still no-op
+            // when the hardware is powered off or the cable is unplugged.
+            // Phase 2 must skip-on-unavailable per the #531 compliance bar
+            // (Qodo review #1).
+            WaitForOrInconclusive(
+                () => FindListItems(mainWindow, cf, DEVICE_LIST_ID).ItemCount > 0,
                 DEVICE_APPEAR_TIMEOUT,
-                "Device did not appear in the connected list.");
+                "Device did not appear in the connected list within "
+                + $"{DEVICE_APPEAR_TIMEOUT.TotalSeconds:F0}s. The "
+                + $"{BENCH_AVAILABLE_ENV_VAR} env var is set but no device was "
+                + "discovered - check the USB connection / power state.");
 
             // ----- Enable first channel -----
             // Set the toggle deterministically to the 'On' state rather than
@@ -200,8 +211,17 @@
                 "Disconnect command button.");
             disconnect.AsButton().Invoke();
 
+            // Require BOTH that the list element still exists AND that its
+            // data-item count is zero. Without the ListFound guard, an
+            // AutomationId regression (or a transient UIA-tree drop) would
+            // make `ItemCount == 0` pass for the wrong reason: "the list is
+            // gone" looks identical to "the list is empty" (Qodo review #8).
             WaitFor(
-                () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length == 0,
+                () =>
+                {
+                    var snap = FindListItems(mainWindow, cf, DEVICE_LIST_ID);
+                    return snap.ListFound && snap.ItemCount == 0;
+                },
                 DEVICE_APPEAR_TIMEOUT,
                 "Device was not removed from the connected list after disconnect.");
         }
@@ -236,17 +256,41 @@
     }
 
     /// <summary>
-    /// Re-finds the list element by AutomationId and returns its children.
+    /// Result of looking up a list container by AutomationId and counting its
+    /// data rows. <see cref="ListFound"/> distinguishes "list element missing"
+    /// from "list found and empty" - a disconnect predicate that just checked
+    /// for "0 children" would otherwise produce a false-positive pass when
+    /// the AutomationId regressed (Qodo review #8).
+    /// </summary>
+    private readonly record struct ListItemSnapshot(bool ListFound, int ItemCount);
+
+    /// <summary>
+    /// Re-finds the list element by AutomationId and counts its data rows.
     /// Always re-locates the parent on each call so a stale AutomationElement
-    /// from a prior UI-tree refresh can't poison a polling loop. Returns an
-    /// empty array if the list is currently absent (the caller's WaitFor
-    /// predicate keeps polling until it appears).
-    /// </summary>
-    private static AutomationElement[] FindListChildren(
+    /// from a prior UI-tree refresh can't poison a polling loop. Counts only
+    /// <c>ListItem</c> / <c>DataItem</c> descendants rather than every visual
+    /// child, because a WPF ItemsControl's child collection includes
+    /// scrollbars, item-container headers, etc., which would otherwise let
+    /// <c>Length &gt; 0</c> succeed even when no data rows exist.
+    /// </summary>
+    private static ListItemSnapshot FindListItems(
         AutomationElement scope, ConditionFactory cf, string automationId)
     {
         var element = scope.FindFirstDescendant(cf.ByAutomationId(automationId));
-        return element is null ? Array.Empty<AutomationElement>() : element.FindAllChildren();
+        if (element is null)
+        {
+            return new ListItemSnapshot(ListFound: false, ItemCount: 0);
+        }
+
+        var listItems = element.FindAllDescendants(cf.ByControlType(ControlType.ListItem));
+        if (listItems.Length > 0)
+        {
+            return new ListItemSnapshot(ListFound: true, ItemCount: listItems.Length);
+        }
+
+        // DataGrid rows typically present as DataItem rather than ListItem.
+        var dataItems = element.FindAllDescendants(cf.ByControlType(ControlType.DataItem));
+        return new ListItemSnapshot(ListFound: true, ItemCount: dataItems.Length);
     }

Modify the list child query to specifically search for ListItem or DataItem
control types instead of returning all visual children.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [245-250]

-private static AutomationElement[] FindListChildren(
+private static AutomationElement[] FindListItems(
     AutomationElement scope, ConditionFactory cf, string automationId)
 {
     var element = scope.FindFirstDescendant(cf.ByAutomationId(automationId));
-    return element is null ? Array.Empty<AutomationElement>() : element.FindAllChildren();
+    if (element is null)
+    {
+        return Array.Empty<AutomationElement>();
+    }
+
+    var listItems = element.FindAllDescendants(cf.ByControlType(ControlType.ListItem));
+    if (listItems.Length > 0)
+    {
+        return listItems;
+    }
+
+    // DataGrid rows typically show up as DataItem.
+    return element.FindAllDescendants(cf.ByControlType(ControlType.DataItem));
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: Using FindAllChildren() on a WPF list control will return non-item visual elements like scrollbars and headers, which incorrectly causes .Length > 0 checks to pass even when the list is empty.

Medium
Count real list items only
Suggestion Impact:Replaced FindListChildren-based predicates with a new FindListItems implementation that counts ListItem/DataItem descendants, preventing false positives from non-item visual children; updated wait checks to use the item count (and added an additional guard ensuring the list exists when asserting it becomes empty).

code diff:

-            WaitFor(
-                () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length > 0,
+            //
+            // If the device never appears within the timeout, treat it as
+            // "bench device not actually discoverable" and skip
+            // (Assert.Inconclusive) rather than fail - the env-var gate told
+            // us a device was *expected*, but the connect path can still no-op
+            // when the hardware is powered off or the cable is unplugged.
+            // Phase 2 must skip-on-unavailable per the #531 compliance bar
+            // (Qodo review #1).
+            WaitForOrInconclusive(
+                () => FindListItems(mainWindow, cf, DEVICE_LIST_ID).ItemCount > 0,
                 DEVICE_APPEAR_TIMEOUT,
-                "Device did not appear in the connected list.");
+                "Device did not appear in the connected list within "
+                + $"{DEVICE_APPEAR_TIMEOUT.TotalSeconds:F0}s. The "
+                + $"{BENCH_AVAILABLE_ENV_VAR} env var is set but no device was "
+                + "discovered - check the USB connection / power state.");
 
             // ----- Enable first channel -----
             // Set the toggle deterministically to the 'On' state rather than
@@ -200,8 +211,17 @@
                 "Disconnect command button.");
             disconnect.AsButton().Invoke();
 
+            // Require BOTH that the list element still exists AND that its
+            // data-item count is zero. Without the ListFound guard, an
+            // AutomationId regression (or a transient UIA-tree drop) would
+            // make `ItemCount == 0` pass for the wrong reason: "the list is
+            // gone" looks identical to "the list is empty" (Qodo review #8).
             WaitFor(
-                () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length == 0,
+                () =>
+                {
+                    var snap = FindListItems(mainWindow, cf, DEVICE_LIST_ID);
+                    return snap.ListFound && snap.ItemCount == 0;
+                },
                 DEVICE_APPEAR_TIMEOUT,
                 "Device was not removed from the connected list after disconnect.");
         }
@@ -236,17 +256,41 @@
     }
 
     /// <summary>
-    /// Re-finds the list element by AutomationId and returns its children.
+    /// Result of looking up a list container by AutomationId and counting its
+    /// data rows. <see cref="ListFound"/> distinguishes "list element missing"
+    /// from "list found and empty" - a disconnect predicate that just checked
+    /// for "0 children" would otherwise produce a false-positive pass when
+    /// the AutomationId regressed (Qodo review #8).
+    /// </summary>
+    private readonly record struct ListItemSnapshot(bool ListFound, int ItemCount);
+
+    /// <summary>
+    /// Re-finds the list element by AutomationId and counts its data rows.
     /// Always re-locates the parent on each call so a stale AutomationElement
-    /// from a prior UI-tree refresh can't poison a polling loop. Returns an
-    /// empty array if the list is currently absent (the caller's WaitFor
-    /// predicate keeps polling until it appears).
-    /// </summary>
-    private static AutomationElement[] FindListChildren(
+    /// from a prior UI-tree refresh can't poison a polling loop. Counts only
+    /// <c>ListItem</c> / <c>DataItem</c> descendants rather than every visual
+    /// child, because a WPF ItemsControl's child collection includes
+    /// scrollbars, item-container headers, etc., which would otherwise let
+    /// <c>Length &gt; 0</c> succeed even when no data rows exist.
+    /// </summary>
+    private static ListItemSnapshot FindListItems(
         AutomationElement scope, ConditionFactory cf, string automationId)
     {
         var element = scope.FindFirstDescendant(cf.ByAutomationId(automationId));
-        return element is null ? Array.Empty<AutomationElement>() : element.FindAllChildren();
+        if (element is null)
+        {
+            return new ListItemSnapshot(ListFound: false, ItemCount: 0);
+        }
+
+        var listItems = element.FindAllDescendants(cf.ByControlType(ControlType.ListItem));
+        if (listItems.Length > 0)
+        {
+            return new ListItemSnapshot(ListFound: true, ItemCount: listItems.Length);
+        }
+
+        // DataGrid rows typically present as DataItem rather than ListItem.
+        var dataItems = element.FindAllDescendants(cf.ByControlType(ControlType.DataItem));
+        return new ListItemSnapshot(ListFound: true, ItemCount: dataItems.Length);
     }

Update the wait conditions to verify the count of actual list items rather than
all visual child elements.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [126-206]

 WaitFor(
-    () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length > 0,
+    () => FindListItems(mainWindow, cf, DEVICE_LIST_ID).Length > 0,
     DEVICE_APPEAR_TIMEOUT,
     "Device did not appear in the connected list.");
 ...
 WaitFor(
-    () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length == 0,
+    () => FindListItems(mainWindow, cf, DEVICE_LIST_ID).Length == 0,
     DEVICE_APPEAR_TIMEOUT,
     "Device was not removed from the connected list after disconnect.");
Suggestion importance[1-10]: 7

__

Why: Updating the method calls to explicitly check for list items instead of all visual children avoids false positives from WPF list container elements like scrollbars.

Medium
Wait for streaming readiness
Suggestion Impact:The commit removed the fixed Thread.Sleep(STREAMING_DWELL_TIME) and replaced it with a polling-based WaitFor that returns as soon as streaming is detected via a UI-visible graph update (bounding rectangle change), achieving a dynamic readiness wait (though it uses graph state rather than Stop-button enabled state).

code diff:

-            Thread.Sleep(STREAMING_DWELL_TIME);
-
-            // Re-fetch in case the surface was lazy-bound on Start.
-            liveGraph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
-                "Live graph control (post-dwell).");
-
-            Assert.IsFalse(liveGraph.IsOffscreen, "Live graph was offscreen after Start.");
-            Assert.IsTrue(liveGraph.BoundingRectangle.Width > 0
-                          && liveGraph.BoundingRectangle.Height > 0,
-                "Live graph had zero-sized bounding box; streaming likely did not start.");
-
-            // Proxy data-arrival check: enabled-state should remain visible and the
-            // graph's rectangle stayed non-empty across the dwell. A stronger
-            // assertion needs an AutomationProperties.HelpText (or similar) bound
-            // to the live point count - tracked under #531 follow-up.
-            Assert.IsTrue(liveGraph.IsEnabled,
-                "Live graph was disabled after streaming dwell; streaming likely did not start.");
-            Assert.IsTrue(preStreamRect.Width > 0,
-                "Pre-stream graph rectangle was empty; the control wasn't laid out before Start.");
+            // Poll-and-detect rather than fixed-sleep: WaitFor returns as soon
+            // as we observe a visible change to the graph (and skips the
+            // remaining dwell), but if nothing has changed by the deadline we
+            // still spent the same wall-clock budget as the old Thread.Sleep
+            // and get a precise failure message ("did not visibly update").
+            WaitFor(
+                () =>
+                {
+                    var graph = FindByAutomationId(mainWindow, cf, LIVE_GRAPH_ID,
+                        "Live graph control (streaming-update poll).");
+                    var rect = graph.BoundingRectangle;
+                    return !graph.IsOffscreen
+                           && graph.IsEnabled
+                           && rect.Width > 0
+                           && rect.Height > 0
+                           && !rect.Equals(preStreamRect);
+                },
+                STREAMING_DWELL_TIME,
+                "Live graph did not visibly update during the streaming dwell; "
+                + "streaming may not have started or data is not reaching the plot.");
 

Replace the fixed Thread.Sleep with a dynamic UI state wait, such as checking
for the stop button to become enabled.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [161-196]

 start.AsButton().Invoke();
-...
-Thread.Sleep(STREAMING_DWELL_TIME);
+
+// Wait for an explicit UI signal that streaming actually started.
+WaitFor(
+    () =>
+    {
+        var stopButton = FindByAutomationId(mainWindow, cf, STOP_STREAMING_ID,
+                "Stop-streaming command button (streaming-started probe).")
+            .AsButton();
+        return stopButton.IsEnabled;
+    },
+    TimeSpan.FromSeconds(15),
+    "Stop button did not become enabled; streaming likely did not start.");
+
+// Optional small dwell to allow the graph to render at least once.
+Thread.Sleep(TimeSpan.FromSeconds(1));
 ...
 var stop = FindByAutomationId(mainWindow, cf, STOP_STREAMING_ID,
     "Stop-streaming command button.");
 stop.AsButton().Invoke();
Suggestion importance[1-10]: 3

__

Why: While dynamic waits are preferred over static Thread.Sleep, this suggestion overlooks the PR's explicit comments explaining that the fixed dwell time is specifically needed to allow unobservable graph points to accumulate.

Low
✅ Suggestions up to commit a2f9571
CategorySuggestion                                                                                                                                    Impact
Possible issue
Make toggle state deterministic
Suggestion Impact:The commit adds a post-Toggle() WaitFor loop (re-finding the element each poll) and introduces a 5-second TOGGLE_PROPAGATION_TIMEOUT constant to ensure the toggle reaches the On state before continuing.

code diff:

+    private static readonly TimeSpan TOGGLE_PROPAGATION_TIMEOUT = TimeSpan.FromSeconds(5);
     private static readonly TimeSpan STREAMING_DWELL_TIME = TimeSpan.FromSeconds(3);
     private static readonly TimeSpan SHUTDOWN_GRACE = TimeSpan.FromSeconds(5);
 
@@ -140,6 +141,18 @@
             if (firstChannelToggle.ToggleState != ToggleState.On)
             {
                 firstChannelToggle.Toggle();
+
+                // Wait for the toggle state change to propagate through the
+                // WPF data-binding before asserting downstream stream behavior.
+                // Re-find the element each poll so we don't rely on a possibly
+                // stale AutomationElement reference after the UI updates.
+                WaitFor(
+                    () => FindByAutomationId(mainWindow, cf, FIRST_CHANNEL_TOGGLE_ID,
+                            "First-channel toggle (post-toggle).")
+                        .AsToggleButton()
+                        .ToggleState == ToggleState.On,
+                    TOGGLE_PROPAGATION_TIMEOUT,
+                    "First channel did not reach the 'On' state after toggling.");
             }

Add a wait condition after calling Toggle() to ensure the toggle state reaches
On. This makes the test deterministic and prevents proceeding before the UI
state updates.

Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs [137-143]

 var firstChannel = FindByAutomationId(mainWindow, cf, FIRST_CHANNEL_TOGGLE_ID,
     "Enable-toggle on the first analog channel.");
 var firstChannelToggle = firstChannel.AsToggleButton();
+
 if (firstChannelToggle.ToggleState != ToggleState.On)
 {
     firstChannelToggle.Toggle();
+
+    WaitFor(
+        () => FindByAutomationId(mainWindow, cf, FIRST_CHANNEL_TOGGLE_ID,
+                "Enable-toggle on the first analog channel.")
+            .AsToggleButton()
+            .ToggleState == ToggleState.On,
+        TimeSpan.FromSeconds(5),
+        "First channel did not reach the 'On' state after toggling.");
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: UI interactions can be asynchronous, and waiting for the toggle state to actually change before proceeding is an excellent way to prevent test flakiness.

Medium
Set correct working directory
Suggestion Impact:Replaced Application.Launch(exePath) with Application.Launch(ProcessStartInfo) and set WorkingDirectory to Path.GetDirectoryName(exePath) (falling back to Environment.CurrentDirectory), matching the suggestion’s intent.

code diff:

@@ -1,5 +1,7 @@
 using System;
 using System.ComponentModel;
+using System.Diagnostics;
+using System.IO;
 using System.Threading;
 using FlaUI.Core;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
@@ -43,7 +45,17 @@
     {
         try
         {
-            return Application.Launch(exePath);
+            // Set WorkingDirectory to the exe's folder so the app's relative
+            // path lookups (config files, resource probing, settings stores)
+            // resolve from a known location instead of inheriting the test
+            // runner's cwd (which can be anything from MSTest's bin/ down to
+            // wherever the IDE was launched from).
+            var startInfo = new ProcessStartInfo(exePath)
+            {
+                WorkingDirectory = Path.GetDirectoryName(exePath)
+                                   ?? Environment.CurrentDirectory,
+            };
+            return Application.Launch(startInfo);
         }

Use ProcessStartInfo to launch the app with its working directory set to the
executable's folder. This avoids relative path issues during test execution.

Daqifi.Desktop.Test/UITests/UIAppLifecycle.cs [42-57]

 public static Application LaunchOrInconclusive(string exePath)
 {
     try
     {
-        return Application.Launch(exePath);
+        var startInfo = new System.Diagnostics.ProcessStartInfo(exePath)
+        {
+            WorkingDirectory = System.IO.Path.GetDirectoryName(exePath) ?? Environment.CurrentDirectory
+        };
+
+        return Application.Launch(startInfo);
     }
     catch (Win32Exception ex) when (ex.NativeErrorCode == 740 /* ERROR_ELEVATION_REQUIRED */)
     {
         Assert.Inconclusive(
             "Skipped: DAQiFi.exe requires administrator elevation (app.manifest sets " +
             "requestedExecutionLevel=requireAdministrator). Run the test from an elevated " +
             "terminal, or switch the manifest to asInvoker for the UI-test build. See #531.");
-        // Assert.Inconclusive throws; this return keeps the compiler happy.
         throw;
     }
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: Setting the correct working directory when launching an executable in tests is a great practice to prevent elusive file-resolution bugs.

Medium
Ensure process fully terminates
Suggestion Impact:Added a post-Kill wait by looking up the OS process via ProcessId and calling Process.WaitForExit(...) (with a helper method) to avoid stray processes during test teardown.

code diff:

@@ -87,6 +99,13 @@
             if (!app.HasExited)
             {
                 app.Kill();
+                // Kill() is asynchronous - the OS posts the termination but the
+                // process can still hold file/socket handles for a moment after.
+                // Block here so callers see a fully-exited process when
+                // Dispose() runs, preventing stray processes between consecutive
+                // test runs. FlaUI's Application doesn't expose the underlying
+                // Process directly, but we can fetch it by ProcessId.
+                WaitForExitByProcessId(app.ProcessId, grace);
             }
         }
         catch
@@ -98,5 +117,24 @@
             app.Dispose();
         }
     }
+
+    /// <summary>
+    /// Blocks until the OS process with the given PID has exited or the grace
+    /// window expires. Swallows the (expected) <see cref="ArgumentException"/>
+    /// from <see cref="Process.GetProcessById(int)"/> when the process is
+    /// already gone by the time we look it up.
+    /// </summary>
+    private static void WaitForExitByProcessId(int processId, TimeSpan grace)
+    {
+        try
+        {
+            using var process = Process.GetProcessById(processId);
+            process.WaitForExit((int)grace.TotalMilliseconds);
+        }
+        catch (ArgumentException)
+        {
+            // Process already exited - no further wait needed.
+        }
+    }

Wait for the process to exit after calling Kill() to ensure it fully terminates
before disposing. Use app.Process?.WaitForExit(...) to prevent stray processes.

Daqifi.Desktop.Test/UITests/UIAppLifecycle.cs [70-100]

 public static void CloseAppGracefully(Application? app, TimeSpan grace)
 {
     if (app is null)
     {
         return;
     }
 
     try
     {
         app.Close();
 
         var deadline = DateTime.UtcNow + grace;
         while (DateTime.UtcNow < deadline && !app.HasExited)
         {
             Thread.Sleep(100);
         }
 
         if (!app.HasExited)
         {
             app.Kill();
+            app.Process?.WaitForExit((int)grace.TotalMilliseconds);
         }
     }
     catch
     {
         // Best-effort teardown - never let cleanup mask the real failure.
     }
     finally
     {
         app.Dispose();
     }
 }

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: While Kill() is asynchronous and a brief wait ensures clean teardown, this is somewhat mitigated by the fact that the teardown is already best-effort and Dispose() will be called.

Low
✅ Suggestions up to commit 5163755
CategorySuggestion                                                                                                                                    Impact
Possible issue
Make toggle state deterministic
Suggestion Impact:Replaced the unconditional Toggle() on the first channel with a state check against ToggleState.On and toggles only when needed; also added the required ToggleState import.

code diff:

@@ -6,6 +6,7 @@
 using FlaUI.Core;
 using FlaUI.Core.AutomationElements;
 using FlaUI.Core.Conditions;
+using FlaUI.Core.Definitions;
 using FlaUI.UIA3;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
@@ -69,7 +70,7 @@
 
     [TestMethod]
     [TestCategory("UI-Bench")]
-    public void ConnectStreamDisconnect_HappyPath()
+    public void ConnectStreamDisconnectHappyPath()
     {
         if (!IsBenchDeviceAvailable())
         {
@@ -117,16 +118,29 @@
             connect.AsButton().Invoke();
 
             // Wait for the connected-devices list to show at least one row.
-            var deviceList = FindByAutomationId(mainWindow, cf, DEVICE_LIST_ID,
-                "Connected-devices list container.");
-            WaitFor(() => deviceList.FindAllChildren().Length > 0, DEVICE_APPEAR_TIMEOUT,
+            // Re-find the list inside the predicate each poll: UI Automation
+            // elements can become stale across major tree transitions (e.g.
+            // when the ConnectionDialog closes), and a cached element that's
+            // gone stale will keep throwing inside WaitFor until timeout.
+            WaitFor(
+                () => FindListChildren(mainWindow, cf, DEVICE_LIST_ID).Length > 0,
+                DEVICE_APPEAR_TIMEOUT,
                 "Device did not appear in the connected list.");
 
             // ----- Enable first channel ---...

Bug fixes (5):
- Dialog searched in main window (#7): ConnectionDialog is a separate
  top-level MetroWindow; add WaitForTopLevelWindow helper and search for
  the Connect button inside it instead of inside the main window.
- No 740 skip in Phase 2 (#8): extract shared LaunchOrInconclusive helper
  in UIAppLifecycle and use it in both Phase 1 and Phase 2 so the UAC
  elevation path is handled identically.
- Teardown wait uses main-handle (#9): replace
  WaitWhileMainHandleIsMissing with a polling loop on app.HasExited in
  the new UIAppLifecycle.CloseAppGracefully, so we wait for the
  process to exit (allowing the app's shutdown handlers to run) rather
  than just for the main-window handle to vanish.
- WaitFor hides root cause (#10): capture lastException during polling
  and append it to the Assert.Fail message so a timed-out wait surfaces
  the real underlying UIA error.
- Exe locator prefers Debug always (#11): use the same #if DEBUG /
  preferredConfigs pattern as Phase 1 by sharing
  MainWindowSmokeTests.TryLocateDesktopExe.

Rule violations (4):
- AddDeviceButtonId naming noncompliant (#1): rename all constants to
  SCREAMING_SNAKE_CASE per CLAUDE.md.
- Inline catch/finally braces (#2): teardown now lives in a shared
  Allman-formatted helper.
- UI test files misnamed (#5): rename files to *Tests.cs.
- [Ignore] line exceeds 120 chars (#6): the unconditional [Ignore]
  attribute is removed entirely in favour of runtime env-var gating
  (see requirement gap #3 below), so the long line goes with it.

Requirement gaps (2):
- ConnectStreamDisconnectTest always ignored (#3): replace the
  unconditional [Ignore] attribute with a runtime env-var gate
  (DAQIFI_BENCH_DEVICE_AVAILABLE=1). On a normal CI run the test
  self-reports Inconclusive; on the bench machine, set the variable
  and the happy-path runs. No code change needed to enable.
- Graph non-zero data unverified (#4): strengthen the post-stream
  check (still UIA-only since OxyPlot/LiveCharts data is not surfaced
  through UI Automation today): capture the pre-stream bounding
  rectangle, assert the control is enabled and re-rendered after the
  dwell, and document the residual gap (needs an automation-visible
  point-count probe, tracked under #531 follow-up).

Refactoring:
- Add Daqifi.Desktop.Test/UITests/UIAppLifecycle.cs as the single home
  for LaunchOrInconclusive and CloseAppGracefully, removing the
  duplicated try/catch and teardown logic in both test files.
@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 5163755

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit 5163755

Comment thread Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs
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.
@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit a2f9571

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

Qodo pass 2 skip notes:

  • Graph non-zero data unverified (review Export file overwritten during export #3, persistent) — SKIPPED.
    Structurally not fixable through FlaUI alone. UI Automation does not
    expose OxyPlot / LiveCharts plot series data; the WPF graph control
    has no automation-visible point count. Closing the requirement would
    need an app-side test hook (e.g. AutomationProperties.HelpText
    bound to the live point count, or a debug-only automation-accessible
    label) — that's an app change outside the scope of this test-scaffold
    PR. Current proxy assertions (control exists, IsEnabled, not
    offscreen, non-zero bounding rectangle, pre-stream geometry captured
    before the dwell) catch the "graph never rendered" failure mode.
    Tracked as a follow-up under issue test(ui): FlaUI scaffold + Connect-Stream-Disconnect happy-path test #531.

  • Poll for streaming start state (/improve importance 3) —
    SKIPPED. The proposed change replaces Thread.Sleep(STREAMING_DWELL_TIME)
    with WaitFor on a Stop-button-enabled predicate. The dwell is
    intentional: it gives streamed samples 3s to accumulate on the
    live graph, not to wait for an unrelated UI "started" signal. The
    Stop button enables synchronously on Start, so the proposed WaitFor
    would return immediately and defeat the purpose. Keeping the dwell.

Persistent-comment carry-over from pass 1 (findings #2, #5, #6,
#11, #12 in the rollup): these reference the OLD file path
ConnectStreamDisconnectTest.cs (without trailing s). The actual
fixes landed in pass 1's rename + refactor. They should age out of
Qodo's persistent surface on the next pass.

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit a2f9571

Comment thread Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs Outdated
New suggestions surfaced post-pass-2:

- Make toggle state deterministic (importance 8): the pass-2 fix
  reads ToggleState and only calls Toggle() when not already On, but
  WPF data-binding updates the toggle's reported state asynchronously
  after Toggle() returns. If the test proceeds to Start streaming
  before the toggle bound back to its data context, the channel may
  effectively be off when Start fires. Add a WaitFor on the post-
  toggle state (re-finding the element each poll so a stale UIA
  reference can't poison the wait) with a new
  TOGGLE_PROPAGATION_TIMEOUT (5s).

- Set correct working directory (importance 7): replace the bare
  Application.Launch(exePath) with Application.Launch(ProcessStartInfo)
  that sets WorkingDirectory = exe's folder. This avoids file-
  resolution surprises when the app uses relative paths for config /
  resources and the test runner's cwd is wherever the IDE/MSTest was
  launched from. Also avoids breaking on CI agents that run from
  unrelated work trees.

- Ensure process fully terminates (importance 5): after Kill() we
  Dispose()d immediately, but Kill() is async at the OS layer - the
  process can still hold file/socket handles for a beat afterwards.
  Add WaitForExit(grace) via a Process.GetProcessById lookup
  (FlaUI's Application exposes ProcessId but not the underlying
  Process). The new WaitForExitByProcessId helper swallows the
  expected ArgumentException when the PID is already gone.

Persistent comment carry-over (review #3, #4, #6, #11-#15):
Qodo's persistent rollup still lists the original pass-1 findings
against the OLD file paths (ConnectStreamDisconnectTest.cs without
trailing s, MainWindowSmokeTest.cs likewise). The actual fixes have
already landed - the new files at HEAD don't have the flagged
patterns - so these are stale carry-overs from earlier passes.
Qodo's review of the latest commit (a2f9571) marks 7 of the 11
original findings as Resolved; the remainder are misclassified due
to the same stale-snapshot behavior.

The only genuinely active requirement gap is #6 (Graph non-zero data
unverified). That's structurally not fixable from the FlaUI side -
documented in the pass-2 PR comment.
@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 17ab383

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit 17ab383

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
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 25afc80

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit 25afc80

Comment thread Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs
…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
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit 004c857

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit 004c857

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.
@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit 69f939e

…wait

Pass 7 new finding:

Bug fix - Selector regression becomes skip (review #1):
The pass-4 change made the post-connect device-appear wait use
WaitForOrInconclusive(ItemCount > 0, ...). That correctly turns
"device powered off / cable unplugged" into Inconclusive (which
is what #531 compliance requires), but it ALSO turns "the
DEVICE_LIST_ID AutomationId is missing / regressed" into
Inconclusive - because FindListItems returns
(ListFound:false, ItemCount:0) when the list element itself
isn't found, so ItemCount > 0 never becomes true, the wait
times out, and the selector regression silently presents as
"bench device not available".

Fix: locate the list element ONCE before the wait
(_ = FindByAutomationId(..., DEVICE_LIST_ID, ...)) so a
genuinely-missing selector hard-fails with the
FindByAutomationId Assert.IsNotNull. Then the
WaitForOrInconclusive only converts true "list exists but
has no devices yet" timeouts to Inconclusive.

This is correctness: the env-var-gated bench run should
catch a broken XAML AutomationId hookup as a failure, not
quietly skip it as if the hardware were unplugged.

Persistent comment carry-over (review #16 Teardown wait,
#17 Exe locator): both still reference the OLD
ConnectStreamDisconnectTest.cs / MainWindowSmokeTest.cs
paths (no trailing s). The actual fixes have been in place
since pass 1 (UIAppLifecycle.CloseAppGracefully polls
HasExited; TryLocateDesktopExe uses #if DEBUG-aware config
preference). These are Qodo persistent-rollup artifacts
against the original PR diff, not active findings against HEAD.

Suggestions surface: all pass-6 /improve items now resolved
on a single commit; no new /improve suggestions in pass 7.
The review surface continues marking carry-over items as
Resolved (pass 7 marks 15 of 18 prior findings as Resolved).
@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/improve

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 18, 2026

Persistent review updated to latest commit acbd357

@qodo-code-review
Copy link
Copy Markdown
Contributor

Persistent suggestions updated to latest commit acbd357

@cptkoolbeenz
Copy link
Copy Markdown
Member Author

Convergence summary — pass 8 (rule (e) documented-dismissal exit):

After 7 commits applied across 8 Qodo passes, the active surface
has stabilised at two findings that are structurally unfixable
through this PR alone. Per qodo-cycle skill rule (e), this is
the documented-dismissal exit point:

1. Review #5 — Graph non-zero data unverified (📎 Requirement gap)

Cannot fix in this PR. UI Automation does not expose
OxyPlot / LiveCharts plot series data through the UIA tree.
Verified across passes 2–5 by attempting four progressively
stronger proxies (visibility, bounding rectangle, IsEnabled,
pre/post-stream rectangle diff). The current best assertion is
in pass 5/6 (liveGraph.BoundingRectangle != preStreamRect)
with the baseline captured before Start.Invoke() (pass 6)
to close a race condition. Closing the requirement fully needs
an app-side automation hook (e.g. AutomationProperties.HelpText
bound to the live point count, or a debug-only automation-accessible
label) — that's an app change outside the scope of this UI-test
scaffold PR. Tracked as a follow-up under issue #531.

2. Review #17 — Exe locator prefers Debug always (🐞 Bug, stale carry-over)

Cannot fix because the referenced code no longer exists.
Qodo's evidence cites
Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTest.cs[173-185]
(note: NO trailing s on Test). That file was renamed to
ConnectStreamDisconnectTests.cs in pass 1, and the duplicated
TryLocateDesktopExe was removed entirely — Phase 2 now calls
MainWindowSmokeTests.TryLocateDesktopExe, which already uses
the #if DEBUG configuration-aware ordering the finding asks for.
Verified by:

$ grep -rn "TryLocateDesktopExe" Daqifi.Desktop.Test/UITests/
ConnectStreamDisconnectTests.cs:86:  var exePath = MainWindowSmokeTests.TryLocateDesktopExe();
MainWindowSmokeTests.cs:80:  internal static string? TryLocateDesktopExe()

$ grep -A 4 "#if DEBUG" Daqifi.Desktop.Test/UITests/MainWindowSmokeTests.cs
#if DEBUG
        var preferredConfigs = new[] { "Debug", "Release" };
#else
        var preferredConfigs = new[] { "Release", "Debug" };
#endif

This is a persistent-comment carry-over from the original PR
diff snapshot; Qodo's rollup hasn't retired it even though the
target file was deleted in pass 1. Other carry-overs in this
class were marked Resolved over passes 5-8 (review #12, #13, #15,
#16, #18); #17 has not been retired but the same fix is in place.

Convergence trajectory

Pass Commit New bugs found New rules New reqs Resolved by Qodo
1 5163755 applied 11 findings (5 bug, 4 rule, 2 req)
2 a2f9571 +3 +1 0 2
3 17ab383 0 (3 /improve suggestions) 0 0 0
4 25afc80 +1 0 +1 5
5 004c857 0 (2 /improve, importance 9 + 7) 0 0 11 of 13
6 69f939e 0 (3 /improve, all applied) 0 0 most carry-overs
7 acbd357 +1 (selector regression) 0 0 most remaining
8 0 0 0 pass-7 resolved

/agentic_review final counters: 🐞 Bugs (1), 📘 Rule violations (0),
📎 Requirement gaps (1). Both remaining items dismissed per rule (e).
/improve: 0 unresolved (down from 2 historical
<!-- not_implemented --> markers which referenced the OLD
file paths that no longer exist).

Ready for human review.

Comment on lines +127 to +128
_ = FindByAutomationId(mainWindow, cf, DEVICE_LIST_ID,
"Connected-devices list container.");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Device list hidden by tabs 🐞 Bug ≡ Correctness

ConnectStreamDisconnectHappyPath now fail-fast asserts DEVICE_LIST_ID exists under the main window,
but MainWindow only renders the selected tab’s content so the devices list will not be in the UIA
tree unless the Devices tab is selected. This will cause the Phase 2 test to fail immediately even
when AutomationIds are correct and a device is available.
Agent Prompt
## Issue description
`ConnectStreamDisconnectHappyPath` asserts the connected-devices list (`DEVICE_LIST_ID`) exists under `mainWindow`, but `MainWindow` renders only the selected tab’s content. Since the default selected tab is Live Graph, the Devices pane (and its connected-devices list) is not present in the UIA tree at that time, causing a deterministic failure.

## Issue Context
In `MainWindow.xaml`, the `TabControl` template uses `ContentSource="SelectedContent"`, meaning only the active tab is in the visual/UIA tree. The Devices UI is hosted under the Devices tab (`DevicesPanePrototype`). `DaqifiViewModel` does not initialize `_selectedIndex`, so it defaults to `0` (first tab: Live Graph).

## Fix Focus Areas
- Daqifi.Desktop.Test/UITests/ConnectStreamDisconnectTests.cs[121-129]

### Implementation direction
- Before calling `FindByAutomationId(... DEVICE_LIST_ID ...)`, navigate to the Devices tab via UIA (e.g., find the TabItem by name/header "Devices" and select it) and wait until the pane is loaded, then perform the fail-fast check.
- Alternatively, scope the lookup to the Devices tab’s content after selecting it (avoid asserting existence in the main window when the tab isn’t active).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(ui): FlaUI scaffold + Connect-Stream-Disconnect happy-path test

1 participant