test(discovery): the code that decides which Linux serial ports are DAQiFi had no tests

[tylerkron]

What was wrong

On Linux, before Core opens a serial port to see whether a DAQiFi board is on it, it asks the kernel what USB device that port belongs to — that's how a machine full of Bluetooth radios, GPS receivers and other vendors' adapters doesn't cost a discovery sweep a probe timeout each. That lookup had no tests at all.

It is not incidental code. CI runs on ubuntu-latest, so this is the provider that actually executes there, and it is the one every Linux user's discovery goes through. When it gets an answer wrong nothing crashes — a board just quietly stops being discovered, or every unrelated port gets probed again and discovery slows to a crawl. Neither shows up as a failure anywhere.

How it was fixed

The lookup is a walk up the sysfs device tree, and it was testable in principle — it reads real files — but it hardcoded /sys/class/tty, so there was no way to point it at anything but the live machine. The walk is now split from the "am I on Linux?" gate and takes the tty class directory as a parameter, which is the same seam MacOsUsbPortDescriptorProvider.Parse already has for the same reason. Behaviour is unchanged: for a bare port name, Path.Combine builds exactly the path the old string interpolation did.

On top of that seam, 35 tests drive it against a fixture tty tree with real symlinks, covering the parts that are easy to get wrong and impossible to notice: that the device entry is a symlink which has to be resolved before walking (walking the logical path climbs back into the class directory and finds nothing), that a node needs both idVendor and idProduct before it counts, that unparseable values mean "unknown" rather than "keep climbing and report the hub's IDs", and the exact depth the walk stops at. The two platform provider factories, which also had no tests, get four more.

One thing a reviewer may want to push back on: the fixtures create real directory symlinks. That works on Linux and macOS; on Windows, where an unprivileged process cannot create one, those specific tests return early rather than fail. Windows is not in CI, and the non-symlink tests still cover the walk itself.

Verification

• Full suite green on net9.0 (3320 Core + 192 Mcp) and net10.0 (3320 Core), release build 0 warnings, two consecutive runs.
• Mutation-tested: 10 of 12 mutants killed. Ignoring the resolved symlink, moving the depth bound either way, accepting one id file instead of both, walking past an unparseable node, pointing at the tty entry instead of its device node, parsing the ids as decimal, dropping the empty-name guard, using the whole port path as the name, and the macOS factory branch all fail the suite. The two survivors are equivalent mutants — NumberStyles.HexNumber already implies AllowLeadingWhite | AllowTrailingWhite, so the .Trim() on each value is redundant for anything sysfs emits.
• No bench run. This path is Linux-only and the bench Mac could not exercise it with a board attached; nothing that reaches the device changed.

Part of #464 (the discovery-provider slice; the Windows half of it is PR #515, which is why this stays clear of those files).

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add fixture-based tests for Linux sysfs USB port descriptor resolution

🧪 Tests ✨ Enhancement 🕐 40+ Minutes

AI Description

• Refactor Linux USB descriptor lookup to accept a tty class root for testability.
• Add comprehensive fixture-driven tests for sysfs walking, parsing, and depth bounds.
• Add platform factory tests to prevent silent provider fallback regressions.

Diagram

graph TD
  A(["SerialDeviceFinder"]) --> B(["UsbPortDescriptorProviderFactory"]) --> C(["LinuxUsbPortDescriptorProvider.GetDescriptor"]) --> D(["Resolve(port, ttyRoot)"]) --> E[("sysfs: /sys/class/tty -> /sys/devices")]
  T(["Core.Tests sysfs fixtures"]) --> D

  subgraph Legend
    direction LR
    _svc(["Runtime component"]) ~~~ _test(["Test code"]) ~~~ _fs[("Filesystem")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce an IFileSystem abstraction (e.g., System.IO.Abstractions)

• ➕ Eliminates need for real directory symlinks in tests; more portable across Windows runners
• ➕ Makes it easier to simulate IO failures and edge cases deterministically
• ➖ Adds dependency and wider refactor surface across discovery providers
• ➖ Can drift from real sysfs behavior (especially link resolution semantics) unless carefully modeled

2. Use an internal seam via injected tty-root provider (Func)

• ➕ Keeps API narrower: tests override the root provider, production uses default
• ➕ Avoids exposing a more general Resolve method shape beyond tests
• ➖ Still needs a filesystem fixture and symlink handling for realism
• ➖ Slightly more indirection than a direct Resolve(root) seam

3. Run Linux-only integration tests against real /sys/class/tty

• ➕ No fixture upkeep; validates actual kernel-provided layout end-to-end
• ➕ Catches environment-specific quirks that fixtures may miss
• ➖ Flaky and hardware-dependent in CI; requires attached USB devices
• ➖ Hard to cover negative/boundary cases (unparseable IDs, depth limit) reliably

Recommendation: The PR’s approach (split platform gate from a parameterized Resolve and test with a real fixture tree) is the best trade-off: it preserves real sysfs semantics (including symlink resolution) without requiring hardware in CI. The early-return behavior on Windows for symlink-dependent tests is a pragmatic compromise given CI is Ubuntu-based; if Windows CI becomes required, consider an IFileSystem abstraction or pre-created junctions as a follow-up.

Files changed (3) +572 / -2

Refactor (1) +32 / -2

LinuxUsbPortDescriptorProvider.csRefactor Linux sysfs lookup into testable Resolve(port, ttyRoot) +32/-2

Refactor Linux sysfs lookup into testable Resolve(port, ttyRoot)

• Extracts the sysfs walking logic into an internal static Resolve method that accepts the tty class root path, while GetDescriptor remains the Linux-gated entrypoint using /sys/class/tty. Introduces named constants for the default tty root and maximum device-tree levels, and switches sysfs path construction to Path.Combine without changing behavior for typical base port names.

src/Daqifi.Core/Device/Discovery/LinuxUsbPortDescriptorProvider.cs

Tests (2) +540 / -0

LinuxUsbPortDescriptorProviderTests.csAdd fixture-driven sysfs tests for Linux USB VID/PID resolution +468/-0

Add fixture-driven sysfs tests for Linux USB VID/PID resolution

• Adds a comprehensive suite of tests exercising Linux sysfs device-tree walking, including symlink resolution, parsing rules (hex/whitespace/unparseable), missing sysfs cases, and the maximum traversal depth. Builds a temporary tty/device fixture tree on disk and conditionally skips symlink-dependent assertions on Windows where unprivileged symlink creation is not available.

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs

UsbProviderFactoryTests.csAdd coverage for platform USB provider factory selection +72/-0

Add coverage for platform USB provider factory selection

• Adds tests ensuring the USB descriptor and location provider factories always return a provider and select the expected implementation for the current OS. Validates that non-supported platforms fall back to the null providers rather than failing or returning an unexpected type.

src/Daqifi.Core.Tests/Device/Discovery/UsbProviderFactoryTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Windows-invalid fixture directory ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The sysfs-shape fixture creates a directory named "1-1.2:1.0"; ':' is an invalid character in a
Windows path segment, so this test will throw during fixture setup on Windows. This makes the test
project non-portable and will fail for developers or CI runs on Windows even before any
platform-gated assertions execute.

Code

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[R258-261]

+        var deviceNode = CreateDeviceTree("usb1", "1-1", "1-1.2");
+        WriteIds(deviceNode, DaqifiVendorText, DaqifiProductText);
+        LinkTty("ttyACM0", CreateSubdirectory(deviceNode, "1-1.2:1.0"));
+

Relevance

●●● Strong

Likely accepted: fixes a real Windows-only crash in test setup; team considers Windows test behavior
(see #427).

PR-#427

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test explicitly creates a subdirectory named with ':' and CreateSubdirectory uses
Directory.CreateDirectory, which will fail on Windows due to invalid path characters.

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[258-263]
src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[397-402]

Agent prompt

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

### Issue description
`Resolve_RealSysfsShape_ReadsIdsFromTheUsbDeviceNodeAboveTheInterface` creates a fixture directory with the Linux sysfs interface name `1-1.2:1.0`. On Windows, `:` is not a valid directory name character, so `Directory.CreateDirectory(...)` throws and the test suite fails when run on Windows.

### Issue Context
This is a test-only portability failure introduced by the fixture layout. It is separate from symlink privilege concerns; even if symlink creation were handled, the directory name itself is invalid on Windows.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[258-261]

### How to fix
Do one of the following:
1. Add an early return/skip at the start of this test (and any other tests that create Linux-only sysfs node names) when running on Windows, before creating any directories.
2. Use a Windows-safe fixture name (e.g., replace `:` with `_`) while keeping the test intent, and document that the fixture uses a sanitized sysfs segment when the underlying filesystem forbids `:`.

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

---

2. Windows symlink tests brittle 🐞 Bug ☼ Reliability

Description

Many tests call LinkTty(), which asserts that creating a directory symlink succeeded; on Windows
without symlink privileges this will fail the test suite rather than skipping. TryLinkDeviceNode()
also only treats IOException/UnauthorizedAccessException as “no symlink support”, so other
symlink-related failures can still break tests on Windows.

Code

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[R440-443]

+    private void LinkTty(string ttyName, string target)
+    {
+        Assert.True(TryLinkTty(ttyName, target), "the fixture symlink could not be created");
+    }

Relevance

●● Moderate

Team often hardens tests, but has rejected Windows “skip when restricted” patterns; outcome
uncertain here.

PR-#403
PR-#427

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

LinkTty hard-asserts symlink creation success, while TryLinkDeviceNode explicitly anticipates
Windows symlink creation failures and returns false in that case—meaning LinkTty will fail rather
than skip on those Windows environments. Numerous tests depend on LinkTty for their fixture setup.

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[439-447]
src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[448-465]
src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[60-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
`LinuxUsbPortDescriptorProviderTests` intends to tolerate Windows environments where an unprivileged process can’t create directory symlinks. However, many tests use `LinkTty(...)`, which hard-asserts symlink creation success; this can fail the whole test suite on Windows. Additionally, the Windows exception filter in `TryLinkDeviceNode(...)` is narrow.

## Issue Context
The helper comment says Windows may be unable to create symlinks and callers should have “nothing left to assert”, but `LinkTty` still asserts success. This creates inconsistent behavior across tests and can break dev/CI runs that execute tests on Windows.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[439-465]

### Suggested change
- Make `LinkTty(...)` skip the test (not pass/fail) when symlink creation is unavailable on Windows. For xUnit, throwing `Xunit.Sdk.SkipException` is a common approach.
- Broaden the Windows catch filter in `TryLinkDeviceNode(...)` (or simplify it) so all Windows “symlink not possible here” failures return `false` consistently (e.g., include `NotSupportedException` / `PlatformNotSupportedException`).
- Optional: centralize a single `EnsureSymlinksSupported()` helper and call it from any test that requires real symlinks, so the intent is explicit.

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

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 26582e1

Results up to commit 807db58 ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Windows symlink tests brittle 🐞 Bug ☼ Reliability

Description

Many tests call LinkTty(), which asserts that creating a directory symlink succeeded; on Windows
without symlink privileges this will fail the test suite rather than skipping. TryLinkDeviceNode()
also only treats IOException/UnauthorizedAccessException as “no symlink support”, so other
symlink-related failures can still break tests on Windows.

Code

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[R440-443]

+    private void LinkTty(string ttyName, string target)
+    {
+        Assert.True(TryLinkTty(ttyName, target), "the fixture symlink could not be created");
+    }

Relevance

●● Moderate

Team often hardens tests, but has rejected Windows “skip when restricted” patterns; outcome
uncertain here.

PR-#403
PR-#427

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

LinkTty hard-asserts symlink creation success, while TryLinkDeviceNode explicitly anticipates
Windows symlink creation failures and returns false in that case—meaning LinkTty will fail rather
than skip on those Windows environments. Numerous tests depend on LinkTty for their fixture setup.

src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[439-447]
src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[448-465]
src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[60-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
`LinuxUsbPortDescriptorProviderTests` intends to tolerate Windows environments where an unprivileged process can’t create directory symlinks. However, many tests use `LinkTty(...)`, which hard-asserts symlink creation success; this can fail the whole test suite on Windows. Additionally, the Windows exception filter in `TryLinkDeviceNode(...)` is narrow.

## Issue Context
The helper comment says Windows may be unable to create symlinks and callers should have “nothing left to assert”, but `LinkTty` still asserts success. This creates inconsistent behavior across tests and can break dev/CI runs that execute tests on Windows.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/LinuxUsbPortDescriptorProviderTests.cs[439-465]

### Suggested change
- Make `LinkTty(...)` skip the test (not pass/fail) when symlink creation is unavailable on Windows. For xUnit, throwing `Xunit.Sdk.SkipException` is a common approach.
- Broaden the Windows catch filter in `TryLinkDeviceNode(...)` (or simplify it) so all Windows “symlink not possible here” failures return `false` consistently (e.g., include `NotSupportedException` / `PlatformNotSupportedException`).
- Optional: centralize a single `EnsureSymlinksSupported()` helper and call it from any test that requires real symlinks, so the intent is explicit.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0a7d290

[tylerkron]

Note for the next review round: the summary's remaining Bugs (1) entry is stale. It quotes TryLinkDeviceNode(), TryLinkTty(...) and Assert.True(...) at lines R440-443 — none of those symbols exist any more, and the file is 418 lines. All three were deleted in 0a7d290, which is the commit the review says it was updated up to. The inline thread for it is replied to and resolved; there are 0 unresolved threads on this head.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0a7d290

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 26582e1

[tylerkron]

Qodo-clean, CI green — ready for review.

4 rounds, ending on head 26582e1. 0 unresolved review threads, build SUCCESS, mergeable CLEAN; re-checked after a 4-minute settle with the review comment's updated_at unchanged.

• Round 1 (807db58) raised one Medium: the fixtures were inconsistent about Windows — the symlink helper documented degrading when the privilege is missing, while the caller every test used hard-asserted the link was created. Real, and fixed in 0a7d290, but at the root rather than at the assert: only four tests are actually about the link, and the rest are about the walk and the parsing, which the provider handles fine with a real device directory. That took the symlink dependency from 13 test methods to 4 and deleted the narrow exception filter along with the branch it guarded. Declined the suggested Xunit.Sdk.SkipException — this repo pins xUnit 2.9.3 and dynamic skip arrived in v3, and a silently-skipping test would report success without having run.
• Round 2 (0a7d290) re-posted the same finding verbatim, still quoting TryLinkDeviceNode() and Assert.True(...) at lines R440-443 — symbols deleted in the commit the review said it was updated up to, in a file that is 418 lines. Flagged as stale rather than acted on.
• Round 3 raised one genuinely new Medium, and a good catch: the sysfs-shape fixture copied the kernel's interface node name 1-1.2:1.0 verbatim, and : is not a legal path segment character on Windows, so Directory.CreateDirectory threw during setup before the test reached anything it was about. Fixed in 26582e1 — nothing parses a node's name, so the spelling was decoration.
• Round 4 struck that finding through as resolved. The summary's remaining Bugs (1) is round 1's entry, which Qodo has not struck through but whose inline thread is replied to and resolved and whose quoted code no longer exists.

Full suite green on net9.0 (3320 Core + 192 Mcp) and net10.0 (3320 Core) after every push, release build 0 warnings. Mutation coverage held at 10 of 12 across the restructure, the two survivors being equivalent mutants.

[qodo-code-review]

Qodo-clean, CI green — ready for review.

4 rounds, ending on head 26582e1. 0 unresolved review threads, build SUCCESS, mergeable CLEAN; re-checked after a 4-minute settle with the review comment's updated_at unchanged.

• Round 1 (807db58) raised one Medium: the fixtures were inconsistent about Windows — the symlink helper documented degrading when the privilege is missing, while the caller every test used hard-asserted the link was created. Real, and fixed in 0a7d290, but at the root rather than at the assert: only four tests are actually about the link, and the rest are about the walk and the parsing, which the provider handles fine with a real device directory. That took the symlink dependency from 13 test methods to 4 and deleted the narrow exception filter along with the branch it guarded. Declined the suggested Xunit.Sdk.SkipException — this repo pins xUnit 2.9.3 and dynamic skip arrived in v3, and a silently-skipping test would report success without having run.
• Round 2 (0a7d290) re-posted the same finding verbatim, still quoting TryLinkDeviceNode() and Assert.True(...) at lines R440-443 — symbols deleted in the commit the review said it was updated up to, in a file that is 418 lines. Flagged as stale rather than acted on.
• Round 3 raised one genuinely new Medium, and a good catch: the sysfs-shape fixture copied the kernel's interface node name 1-1.2:1.0 verbatim, and : is not a legal path segment character on Windows, so Directory.CreateDirectory threw during setup before the test reached anything it was about. Fixed in 26582e1 — nothing parses a node's name, so the spelling was decoration.
• Round 4 struck that finding through as resolved. The summary's remaining Bugs (1) is round 1's entry, which Qodo has not struck through but whose inline thread is replied to and resolved and whose quoted code no longer exists.

Full suite green on net9.0 (3320 Core + 192 Mcp) and net10.0 (3320 Core) after every push, release build 0 warnings. Mutation coverage held at 10 of 12 across the restructure, the two survivors being equivalent mutants.

The PR looks ready from the evidence provided: the reported Windows-invalid fixture issue was addressed, and the symlink dependency is now limited to the four tests explicitly exercising link behavior. The remaining active entry, finding 2, appears stale: it references deleted TryLinkDeviceNode/Assert.True code and obsolete line numbers. I can’t independently verify the CI or merge state here, but the supplied test and build results support a clean review.