test: add direct unit tests for WindowsUsbPortDescriptorProvider by tylerkron · Pull Request #572 · daqifi/daqifi-core

tylerkron · 2026-08-22T16:33:34Z

What was wrong

WindowsUsbPortDescriptorProvider — the collaborator that resolves a serial port's USB vendor/product ID on Windows via the shared PnP entity map — had no tests, unlike its Linux and macOS siblings. It read directly from the WindowsPnpPortMap.Shared singleton with no seam to inject a fake, so its lookup logic (which entity wins when a port is claimed by more than one, what happens on a WMI failure) was only reachable through the platform factory or real WMI.

How it was fixed

Pulled the lookup out into an internal static Resolve(string, WindowsPnpPortMap), the same shape LinuxUsbPortDescriptorProvider.Resolve and MacOsUsbPortDescriptorProvider.Parse already use. WindowsPnpPortMap already takes an injected query function, so Resolve can be driven against a fake map on any OS. GetDescriptor is now just the Windows platform gate plus the real shared map.

Added WindowsUsbPortDescriptorProviderTests covering: a port with a USB entity, a port the map doesn't list, a port claimed only by a non-USB entity, a port claimed by both a non-USB and a USB entity (picks the USB one), and a WMI failure surfacing from the map (returns null rather than propagating).

Verification

dotnet test full suite green on net9.0 and net10.0 (3713 passed, 2 skipped, 0 failed both times). Test-only change plus an internal refactor (no public API surface touched) — no bench validation needed.

Part of #464 (slice 3: discovery descriptor providers). Not merging — for review.

…ovider WindowsUsbPortDescriptorProvider had no direct tests — unlike its Linux and macOS siblings, it read straight from the shared WindowsPnpPortMap.Shared singleton with no injectable seam, so its lookup logic was only reachable through the platform factory or WMI itself. Extracted an internal static Resolve(string, WindowsPnpPortMap) method, mirroring the Resolve/Parse seam already used by LinuxUsbPortDescriptorProvider and MacOsUsbPortDescriptorProvider, so the lookup can be driven against a fake map (WindowsPnpPortMap already takes an injected query function) on any OS. GetDescriptor now just adds the Windows platform gate and supplies the real shared map. Added WindowsUsbPortDescriptorProviderTests covering: a port with a USB entity, a port the map doesn't list, a port claimed only by a non-USB entity, a port claimed by both (skips to the USB one), and a WMI failure surfacing from the map (returns null instead of propagating). Part of #464 (slice 3: discovery descriptor providers). dotnet test full suite green on net9.0 and net10.0 (3713 passed, 2 skipped, 0 failed both times). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tylerkron · 2026-08-22T16:33:40Z

/agentic_review

qodo-code-review · 2026-08-22T16:35:11Z

PR Summary by Qodo

Add unit tests for WindowsUsbPortDescriptorProvider via injectable Resolve seam

🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Extract Windows USB VID/PID lookup into an internal Resolve(port, map) seam.
• Add unit tests covering misses, non-USB entities, conflicts, and map/WMI failures.
• Keep GetDescriptor Windows-gated and backed by the shared cached PnP map.

Diagram

graph TD
  T["WindowsUsbPortDescriptorProviderTests"] --> R["Resolve(port, map)"] --> M["WindowsPnpPortMap.GetDeviceIds"] --> P["PnpDeviceIdParser.SelectUsbDescriptor"] --> D["UsbPortDescriptor?"]
  G["GetDescriptor(port)"] --> R --> M
  G --> S["WindowsPnpPortMap.Shared"] --> M

High-Level Assessment

The following are alternative approaches to this PR:

1. Constructor-inject IWindowsPnpPortMap (or Func>)

➕ Makes the dependency explicit and mockable without static methods
➕ Avoids exposing additional internal surface area on the provider
➖ More invasive change (constructor signature, factory updates, potential DI wiring)
➖ Adds abstraction surface for a single call site; likely overkill for this scenario

2. Make WindowsPnpPortMap.Shared replaceable in tests (settable or internal setter)

➕ No new Resolve method; tests can redirect Shared
➖ Introduces global mutable state and test ordering risk
➖ Harder to reason about correctness in concurrent discovery scenarios

Recommendation: Keep the current approach (internal static Resolve(port, map)) because it mirrors the existing Linux/macOS testing seams, avoids global mutable state, and limits production changes to a single delegation from GetDescriptor. The alternatives either increase architectural surface area (new interface/DI wiring) or add risk via a mutable singleton.

Files changed (2) +109 / -1

Refactor (1) +14 / -1

WindowsUsbPortDescriptorProvider.csExtract internal Resolve(port, map) seam for testable VID/PID lookup +14/-1

Extract internal Resolve(port, map) seam for testable VID/PID lookup
• Refactors GetDescriptor() to delegate to a new internal static Resolve(portName, WindowsPnpPortMap) method. Resolve performs the map lookup and VID/PID selection and swallows map/query failures to return null, preserving probe-fallback behavior while enabling direct unit testing with an injected map.
src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs

Tests (1) +95 / -0

WindowsUsbPortDescriptorProviderTests.csAdd cross-platform unit coverage for Windows USB port descriptor resolution +95/-0

Add cross-platform unit coverage for Windows USB port descriptor resolution
• Introduces direct tests for WindowsUsbPortDescriptorProvider.Resolve() using a fake WindowsPnpPortMap query. Covers USB and non-USB entities, missing ports, multi-claim selection behavior, and exception-to-null fallback. Adds an off-Windows GetDescriptor() assertion to ensure the platform gate short-circuits before any WMI-backed access.
src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs

qodo-code-review · 2026-08-22T16:36:11Z

Code Review by Qodo

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

1. ~~Unit test triggers WMI~~ ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

GetDescriptor_PortNoEntityClaims_ReturnsNull calls WindowsUsbPortDescriptorProvider.GetDescriptor on
Windows, which delegates to WindowsPnpPortMap.Shared; the first lookup can rebuild the shared map
and execute a real WMI Win32_PnPEntity query, making this test slow and potentially flaky.

Code

src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[R95-97]

+#pragma warning disable CA1416 // GetDescriptor is Windows-gated at runtime, not by this call site.
+        Assert.Null(provider.GetDescriptor("COM_DAQIFI_TEST_" + Guid.NewGuid().ToString("N")));
+#pragma warning restore CA1416

Evidence
The test directly invokes GetDescriptor; GetDescriptor delegates to Resolve with
WindowsPnpPortMap.Shared. WindowsPnpPortMap.Shared is constructed with a WMI-backed query, and
GetDeviceIds rebuilds the map (calling that query) when no live cache exists, which is the typical
first-call path in a fresh test process on Windows.
src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[87-97]
src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[13-22]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[59-64]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[121-147]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[153-160]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[239-257]

Agent prompt

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

### Issue description
`GetDescriptor_PortNoEntityClaims_ReturnsNull` currently calls `WindowsUsbPortDescriptorProvider.GetDescriptor(...)` with a random COM-like name. On Windows this can cause `WindowsPnpPortMap.Shared` to rebuild and run a real WMI query, which makes the test suite slower and introduces platform/service flakiness.

### Issue Context
This PR’s intent is “direct unit tests” using the new `Resolve(..., WindowsPnpPortMap)` seam. The only remaining `GetDescriptor` coverage should ideally avoid any dependency on the real singleton map.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[87-98]

### What to change
Pick one of these (ordered by preference):
1) Remove/replace this test with a pure unit assertion using the seam:
  - Test `Resolve` with a fake map for all logic, and keep `GetDescriptor_OffWindows_ReturnsNullWithoutTouchingTheMap` as the platform-gate test.
2) If you want to keep a `GetDescriptor` test, make it *guaranteed* not to rebuild the shared map:
  - Call `GetDescriptor` with `""` or whitespace so `WindowsPnpPortMap.GetDeviceIds` short-circuits before any rebuild/WMI.
  - Update the test name/comment accordingly (it would be validating "invalid/blank port name returns null" rather than "no entity claims").
3) Mark it as Windows-only integration (skip by default / category) so it doesn’t affect normal unit runs.

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

2. ~~Null map silently ignored~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

WindowsUsbPortDescriptorProvider.Resolve accepts a WindowsPnpPortMap parameter but does not validate
it and then catches all exceptions, so a null/invalid map will be silently converted into a null
descriptor, masking a programmer error as “port unclassified”. This failure mode is introduced by
the new injection seam and makes future callers much harder to debug.

Code

src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[R32-39]

+    internal static UsbPortDescriptor? Resolve(string portName, WindowsPnpPortMap map)
+    {
        try
        {
            // A port the map doesn't list is left unclassified rather than refreshed: the caller
            // probes anything it can't classify, so a miss costs one probe, while refreshing for
            // every miss would put a WMI query back on the per-port path this exists to remove.
-            return PnpDeviceIdParser.SelectUsbDescriptor(WindowsPnpPortMap.Shared.GetDeviceIds(portName));
+            return PnpDeviceIdParser.SelectUsbDescriptor(map.GetDeviceIds(portName));

Relevance

●● Moderate
Validation concerns are plausible, but no close precedent addresses null injection combined with
intentional catch-all fallback.
PR-#201
PR-#515

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Resolve is newly added and wraps map.GetDeviceIds(...) in a catch-all without validating map,
meaning a null map would throw NullReferenceException and be swallowed into a null return value,
indistinguishable from “no USB entity”.

src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[21-47]

Agent prompt

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

## Issue description
`WindowsUsbPortDescriptorProvider.Resolve(string, WindowsPnpPortMap)` introduces a new parameter (`map`) but does not guard it against null and then uses a broad `catch { return null; }`. This can turn programmer errors (null map, unexpected exceptions) into a silent “no descriptor”, hiding real defects.

## Issue Context
The intent of the catch is to treat WMI/map rebuild failures as “unclassified” so the caller can fall back to probing. That behavior should remain, but argument validation should happen outside the try/catch (or the catch should be narrowed) so invalid inputs still fail loudly.

## Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[32-47]

## Suggested change
- Add `ArgumentNullException.ThrowIfNull(map);` before the `try`.
- Consider narrowing the `catch` to expected exception types thrown by `WindowsPnpPortMap.GetDeviceIds/Rebuild` (or at least avoid catching `ArgumentNullException`).

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

3. ~~Misleading WMI comment~~ ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

WindowsUsbPortDescriptorProvider.Resolve’s XML comment claims it “never touches WMI”, but Resolve
calls map.GetDeviceIds and can therefore trigger a WMI-backed rebuild when passed
WindowsPnpPortMap.Shared, making the comment inaccurate and misleading for future maintainers.

Code

src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[R30-31]
+    /// exposed. Unlike <see cref="GetDescriptor"/>, this never touches WMI, so it carries no
+    /// platform attribute of its own.

Evidence
The comment asserts no WMI access; however Resolve delegates to WindowsPnpPortMap.GetDeviceIds, and
the Shared map’s rebuild path invokes a WMI query (QueryPortEntities uses ManagementObjectSearcher)
when the cache is not live.
src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[24-43]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[121-147]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[153-160]
src/Daqifi.Core/Device/Discovery/WindowsPnpPortMap.cs[239-257]

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 doc comment on `WindowsUsbPortDescriptorProvider.Resolve` says it “never touches WMI”, but `Resolve` calls `map.GetDeviceIds(...)`. When `map` is `WindowsPnpPortMap.Shared`, `GetDeviceIds` may rebuild and run a WMI query via `QueryPortEntities`.

### Issue Context
Even if `Resolve` does not directly reference `System.Management`, it can still cause WMI access depending on the provided map. The comment should reflect that nuance to avoid incorrect assumptions.

### Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[24-31]

### What to change
Update the wording to something like:
- “Unlike `GetDescriptor`, this method is not platform-annotated because it does not directly call Windows-only APIs; whether a WMI query occurs depends on the provided `WindowsPnpPortMap` implementation.”

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

4. ~~Test asserts nothing on Windows~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

GetDescriptor_OffWindows_ReturnsNullWithoutTouchingTheMap only asserts inside an `if
(!IsOSPlatform(Windows))` block, so on Windows it will pass without executing any assertion. This
makes the test misleading and can hide regressions in how the Windows platform gate behaves under
Windows CI.

Code

src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[R88-92]

+        if (!System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(
+                System.Runtime.InteropServices.OSPlatform.Windows))
+        {
+            Assert.Null(provider.GetDescriptor("COM9"));
+        }

Relevance

●●● Strong
Recent test-review precedents accept strengthening assertions and preventing platform-specific tests
from passing vacuously.
PR-#509
PR-#454

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The only assertion in the test is guarded by a runtime OS check; when the condition is false (i.e.,
on Windows) the test body completes without any assertion.
src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[80-93]

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 `GetDescriptor_OffWindows_ReturnsNullWithoutTouchingTheMap` performs assertions only when not on Windows. When running on Windows, it executes no assertions and still reports as passing.

## Issue Context
The test is explicitly about the off-Windows short-circuit behavior; on Windows it should be marked skipped or should return early with a clear reason.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[80-93]

## Suggested change
- Add an early `if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) return;` at the top of the test, or use a skipping mechanism used elsewhere in this repo (e.g., a custom `SkippableFact`/`Skip.If`) so the intent is explicit.

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

5. ~~Overbroad CA1416 suppression~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new tests blanket-disable CA1416 for the entire file, which can hide real platform-guard
violations in future edits to this test class. This is avoidable by making only the Windows-only
entrypoint platform-annotated (or by scoping the suppression more narrowly).

Code

src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[R20-22]
+#pragma warning disable CA1416
+public class WindowsUsbPortDescriptorProviderTests
+{

Relevance

●●● Strong
Recent repository precedent accepts maintainability fixes that narrow or clarify cross-platform test
behavior.
PR-#529
PR-#509

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test file disables CA1416 globally, and the provider type is annotated as Windows-only, forcing
suppressions even for calling the pure Resolve method.
src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[12-22]
src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[11-33]

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 tests use `#pragma warning disable CA1416` for the whole file to allow construction/use of `WindowsUsbPortDescriptorProvider`, even though the logic under test (`Resolve`) is OS-agnostic.

## Issue Context
Broad suppression can mask genuine accidental calls to Windows-only APIs in future test edits.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[20-22]
- src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[11-33]

## Suggested change
- Prefer moving `[SupportedOSPlatform("windows")]` from the class to only `GetDescriptor` (or to a small Windows-only wrapper) so `Resolve` can be called without CA1416 suppression.
- If you keep the attribute on the class, scope the pragma to the minimal lines that need it (constructor call) rather than the whole file.

ⓘ 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 commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 30ee80b

Results up to commit 3afa133 ⚖️ Balanced

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

1. ~~Null map silently ignored~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

WindowsUsbPortDescriptorProvider.Resolve accepts a WindowsPnpPortMap parameter but does not validate
it and then catches all exceptions, so a null/invalid map will be silently converted into a null
descriptor, masking a programmer error as “port unclassified”. This failure mode is introduced by
the new injection seam and makes future callers much harder to debug.

Code

src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[R32-39]

+    internal static UsbPortDescriptor? Resolve(string portName, WindowsPnpPortMap map)
+    {
        try
        {
            // A port the map doesn't list is left unclassified rather than refreshed: the caller
            // probes anything it can't classify, so a miss costs one probe, while refreshing for
            // every miss would put a WMI query back on the per-port path this exists to remove.
-            return PnpDeviceIdParser.SelectUsbDescriptor(WindowsPnpPortMap.Shared.GetDeviceIds(portName));
+            return PnpDeviceIdParser.SelectUsbDescriptor(map.GetDeviceIds(portName));

Relevance

●● Moderate
Validation concerns are plausible, but no close precedent addresses null injection combined with
intentional catch-all fallback.
PR-#201
PR-#515

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Resolve is newly added and wraps map.GetDeviceIds(...) in a catch-all without validating map,
meaning a null map would throw NullReferenceException and be swallowed into a null return value,
indistinguishable from “no USB entity”.

src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[21-47]

Agent prompt

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

## Issue description
`WindowsUsbPortDescriptorProvider.Resolve(string, WindowsPnpPortMap)` introduces a new parameter (`map`) but does not guard it against null and then uses a broad `catch { return null; }`. This can turn programmer errors (null map, unexpected exceptions) into a silent “no descriptor”, hiding real defects.

## Issue Context
The intent of the catch is to treat WMI/map rebuild failures as “unclassified” so the caller can fall back to probing. That behavior should remain, but argument validation should happen outside the try/catch (or the catch should be narrowed) so invalid inputs still fail loudly.

## Fix Focus Areas
- src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[32-47]

## Suggested change
- Add `ArgumentNullException.ThrowIfNull(map);` before the `try`.
- Consider narrowing the `catch` to expected exception types thrown by `WindowsPnpPortMap.GetDeviceIds/Rebuild` (or at least avoid catching `ArgumentNullException`).

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

2. ~~Test asserts nothing on Windows~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

GetDescriptor_OffWindows_ReturnsNullWithoutTouchingTheMap only asserts inside an `if
(!IsOSPlatform(Windows))` block, so on Windows it will pass without executing any assertion. This
makes the test misleading and can hide regressions in how the Windows platform gate behaves under
Windows CI.

Code

src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[R88-92]

+        if (!System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(
+                System.Runtime.InteropServices.OSPlatform.Windows))
+        {
+            Assert.Null(provider.GetDescriptor("COM9"));
+        }

Relevance

●●● Strong
Recent test-review precedents accept strengthening assertions and preventing platform-specific tests
from passing vacuously.
PR-#509
PR-#454

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The only assertion in the test is guarded by a runtime OS check; when the condition is false (i.e.,
on Windows) the test body completes without any assertion.
src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[80-93]

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 `GetDescriptor_OffWindows_ReturnsNullWithoutTouchingTheMap` performs assertions only when not on Windows. When running on Windows, it executes no assertions and still reports as passing.

## Issue Context
The test is explicitly about the off-Windows short-circuit behavior; on Windows it should be marked skipped or should return early with a clear reason.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[80-93]

## Suggested change
- Add an early `if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) return;` at the top of the test, or use a skipping mechanism used elsewhere in this repo (e.g., a custom `SkippableFact`/`Skip.If`) so the intent is explicit.

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

3. ~~Overbroad CA1416 suppression~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new tests blanket-disable CA1416 for the entire file, which can hide real platform-guard
violations in future edits to this test class. This is avoidable by making only the Windows-only
entrypoint platform-annotated (or by scoping the suppression more narrowly).

Code

src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[R20-22]
+#pragma warning disable CA1416
+public class WindowsUsbPortDescriptorProviderTests
+{

Relevance

●●● Strong
Recent repository precedent accepts maintainability fixes that narrow or clarify cross-platform test
behavior.
PR-#529
PR-#509

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test file disables CA1416 globally, and the provider type is annotated as Windows-only, forcing
suppressions even for calling the pure Resolve method.
src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[12-22]
src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[11-33]

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 tests use `#pragma warning disable CA1416` for the whole file to allow construction/use of `WindowsUsbPortDescriptorProvider`, even though the logic under test (`Resolve`) is OS-agnostic.

## Issue Context
Broad suppression can mask genuine accidental calls to Windows-only APIs in future test edits.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs[20-22]
- src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs[11-33]

## Suggested change
- Prefer moving `[SupportedOSPlatform("windows")]` from the class to only `GetDescriptor` (or to a small Windows-only wrapper) so `Resolve` can be called without CA1416 suppression.
- If you keep the attribute on the class, scope the pragma to the minimal lines that need it (constructor call) rather than the whole file.

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

…r tests - Resolve now throws ArgumentNullException for a null map instead of letting the catch-all swallow the resulting NullReferenceException into an indistinguishable "no descriptor" answer. - Moved [SupportedOSPlatform("windows")] from the class down to just GetDescriptor (the only member that reaches WMI); Resolve is pure and carries no platform attribute, so tests no longer need a blanket CA1416 suppression to call it — only the two GetDescriptor call sites do, each scoped narrowly. Updated UsbPortDescriptorProviderFactory's now-stale suppression comment/pragma to match (the constructor is unannotated). - Replaced the vacuous off-Windows test (asserted nothing when run on Windows) with the LinuxUsbPortDescriptorProviderTests convention: an unconditional GetDescriptor call whose answer is the same on both sides of the gate, plus one explicitly guarded to the platform where the gate itself has to produce the answer. dotnet test full suite green on net9.0 and net10.0 (3715 passed, 2 skipped, 0 failed both times). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tylerkron · 2026-08-22T16:41:41Z

/agentic_review

qodo-code-review · 2026-08-22T16:49:01Z

Code review by qodo was updated up to the latest commit 4eb9e7b

…r tests - Removed GetDescriptor_PortNoEntityClaims_ReturnsNull: on Windows it reached WindowsPnpPortMap.Shared, whose first lookup can rebuild the map and run a real WMI query, making the test slow and potentially flaky. The Windows-side lookup logic it duplicated is already fully covered by the Resolve tests against a fake map. - Corrected Resolve's doc comment, which claimed it "never touches WMI" — true only because it never calls System.Management directly; whether a WMI query happens depends on which WindowsPnpPortMap is passed in. dotnet test full suite green on net9.0 and net10.0 (3714 passed, 2 skipped, 0 failed both times). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

tylerkron · 2026-08-22T16:52:36Z

/agentic_review

qodo-code-review · 2026-08-22T16:53:36Z

Code review by qodo was updated up to the latest commit 30ee80b

tylerkron · 2026-08-22T16:59:18Z

Qodo-clean, CI green — ready for review.

tylerkron requested a review from a team as a code owner August 22, 2026 16:33

qodo-code-review Bot reviewed Aug 22, 2026

View reviewed changes

Comment thread src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs

Comment thread src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs Outdated

Comment thread src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs Outdated

qodo-code-review Bot reviewed Aug 22, 2026

View reviewed changes

Comment thread src/Daqifi.Core.Tests/Device/Discovery/WindowsUsbPortDescriptorProviderTests.cs Outdated

Comment thread src/Daqifi.Core/Device/Discovery/WindowsUsbPortDescriptorProvider.cs Outdated

tylerkron added this pull request to the merge queue Aug 22, 2026

Merged via the queue into main with commit 24e337d Aug 22, 2026
1 check passed

tylerkron deleted the test/464-windows-usb-port-descriptor-provider branch August 22, 2026 20:28

tylerkron mentioned this pull request Aug 22, 2026

test: the extracted collaborators have no direct unit tests — coverage rides the facades they were split from #464

Closed

3 tasks

This was referenced Sep 18, 2026

test: fix SerialDeviceFinder silent-pass and kill vacuous facts #752

Merged

refactor(core): extract shared SafeLog/SafeTrace helper #763

Merged

qodo-code-review Bot mentioned this pull request Sep 27, 2026

docs(core): fix DeviceMetadata.Health and HID finder XML #757

Merged

Navigation Menu

test: add direct unit tests for WindowsUsbPortDescriptorProvider - #572

test: add direct unit tests for WindowsUsbPortDescriptorProvider#572
tylerkron merged 3 commits into
mainfrom
test/464-windows-usb-port-descriptor-provider

tylerkron commented Aug 22, 2026

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

qodo-code-review Bot commented Aug 22, 2026

Uh oh!

qodo-code-review Bot commented Aug 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 22, 2026

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

qodo-code-review Bot commented Aug 22, 2026

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

tylerkron commented Aug 22, 2026

What was wrong

How it was fixed

Verification

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

qodo-code-review Bot commented Aug 22, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 22, 2026

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

qodo-code-review Bot commented Aug 22, 2026

Uh oh!

tylerkron commented Aug 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-code-review Bot commented Aug 22, 2026 •

edited

Loading