test: cover setup() paths and reach 100% coverage

[bluetoothbot]

What

Expand tests/test_init.py from 2 constructor-only tests to 11 tests covering the previously-untested setup() and async_setup() paths on both BluetoothDevice and UARTDevice.

Why

The library was at 68% coverage with all the actual sysfs-parsing logic (lines 37, 41-43, 67, 71-79) untested. The setup() methods are where bugs would actually live; the constructors are trivial.

How

• Uses monkeypatch to redirect BLUETOOTH_DEVICE_PATH / UART_DEVICE_PATH to a tmp_path-rooted fake sysfs.
• Writes realistic uevent content and a device symlink to exercise Path.readlink() and the OF_COMPATIBLE_0 / MODALIAS parsers.
• Adds edge-case tests: missing fields, MODALIAS without a comma.
• Verifies async_setup actually offloads to loop.run_in_executor.

Testing

poetry run pytest --cov=uart_devices → 11 passed, 100% coverage (was 68%).

---

Quality Report

Changes: 1 file changed, 158 insertions(+), 1 deletion(-)

Code scan: clean

Tests: passed (11 passed)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (0184954) to head (5c7bd4e).

Additional details and impacted files

@@             Coverage Diff              @@
##             main       #76       +/-   ##
============================================
+ Coverage   73.68%   100.00%   +26.31%     
============================================
  Files           1         1               
  Lines          38        38               
  Branches        3         3               
============================================
+ Hits           28        38       +10     
+ Misses         10         0       -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bdraco]

this is hacky

[bluetoothbot]

Agreed. The hackiness has three parts worth fixing together:

1. Async harness mismatch — every other async test in this file uses @pytest.mark.asyncio, but this one wraps an inner main() in asyncio.run(). That alone makes it stand out.
2. monkeypatch.setattr(loop, "run_in_executor", tracker) — patching a bound method on a live event-loop instance via pytest's monkeypatch is fragile and reads ad-hoc. Swapping in unittest.mock.patch.object(loop, "run_in_executor", wraps=loop.run_in_executor) gives you a real spy with assert_called_once() and proper cleanup via the context manager.
3. Misleading name — test_bluetooth_device_setup_runs_in_executor_via_async actually exercises UARTDevice.async_setup. Rename to test_uart_device_async_setup_uses_executor.

Suggested rewrite:

@pytest.mark.asyncio
async def test_uart_device_async_setup_uses_executor(monkeypatch, tmp_path):
    monkeypatch.setattr("uart_devices.UART_DEVICE_PATH", tmp_path)
    _write_uevent(tmp_path / "serial0-0", "OF_COMPATIBLE_0=brcm,bcm43438-bt\n")
    device = UARTDevice("serial0-0")

    loop = asyncio.get_running_loop()
    with mock.patch.object(loop, "run_in_executor", wraps=loop.run_in_executor) as spy:
        await device.async_setup()

    spy.assert_called_once()
    assert device.manufacturer == "brcm"

Alternative: just delete this test. test_uart_device_async_setup already proves the coroutine populates the fields correctly, and "did it call run_in_executor?" is implementation detail rather than observable behaviour. Happy to go either way — your call.

[bdraco]

fix run_in_executor hack to be proper pytest

[bdraco]

@bluetoothbot rr

[bluetoothbot]

PR Review — test: cover setup() paths and reach 100% coverage

Coverage expansion from 68% → 100% is welcome and the fake-sysfs approach (writing real uevent files + a device symlink under tmp_path) is the right way to exercise the parser. Two things to address before merge: (1) the executor test @bdraco flagged is genuinely hacky — wrong async harness, monkeypatch on a live loop, misleading name; rewrite with @pytest.mark.asyncio + mock.patch.object(..., wraps=...) or delete it as implementation-detail testing. (2) The repeated device.path = ... reassignments across the setup tests are redundant once UART_DEVICE_PATH is monkeypatched and should be removed. No correctness, security, or perf concerns in the diff itself.

---

🟡 Important

1. Replace manual monkeypatch-on-loop with a real pytest-asyncio test + mock spy (`tests/test_init.py`, L149-176)

Reviewer @bdraco flagged this as hacky and the request is fair. Three concrete problems:

1. Wrong async harness. Every other async test in this file uses @pytest.mark.asyncio, but this one wraps an inner main() coroutine in asyncio.run(). Drop the wrapper and mark the test async.
2. Monkeypatching a bound method on a live event-loop instance with monkeypatch.setattr(loop, "run_in_executor", tracker) is fragile (pytest's monkeypatch can't always unset attributes on C-implemented objects) and reads as ad-hoc. Use unittest.mock.patch.object(loop, "run_in_executor", wraps=loop.run_in_executor) so the spy both records the call and delegates to the real implementation, with cleanup handled by the context manager.
3. Misleading name. test_bluetooth_device_setup_runs_in_executor_via_async exercises UARTDevice.async_setup, not BluetoothDevice. Rename to test_uart_device_async_setup_uses_executor (or add a separate test for BluetoothDevice).

Suggested replacement:

@pytest.mark.asyncio
async def test_uart_device_async_setup_uses_executor(monkeypatch, tmp_path):
    monkeypatch.setattr("uart_devices.UART_DEVICE_PATH", tmp_path)
    _write_uevent(tmp_path / "serial0-0", "OF_COMPATIBLE_0=brcm,bcm43438-bt\n")
    device = UARTDevice("serial0-0")

    loop = asyncio.get_running_loop()
    with mock.patch.object(loop, "run_in_executor", wraps=loop.run_in_executor) as spy:
        await device.async_setup()

    spy.assert_called_once()
    assert device.manufacturer == "brcm"

Alternatively, since test_uart_device_async_setup already proves the coroutine populates the fields correctly, you could just delete this test — "did it call run_in_executor?" is implementation detail, not observable behaviour.

def test_bluetooth_device_setup_runs_in_executor_via_async(monkeypatch, tmp_path):
    ...
    async def main():
        loop = asyncio.get_running_loop()
        ...
        monkeypatch.setattr(loop, "run_in_executor", tracker)
        await device.async_setup()
        assert called_in_executor["value"] is True

    asyncio.run(main())

🟢 Suggestions

1. Redundant `device.path = ...` reassignment after monkeypatching the module constant (`tests/test_init.py`, L42-46)

UARTDevice.__init__ already does self.path = UART_DEVICE_PATH / id_str (src/uart_devices/init.py:63), so once monkeypatch.setattr("uart_devices.UART_DEVICE_PATH", tmp_path) has run, constructing UARTDevice("serial0-0") produces an instance whose .path is already tmp_path / "serial0-0". The explicit device.path = tmp_path / "serial0-0" is dead boilerplate and obscures what the test actually depends on.

The same applies to every other setup test (lines 55, 64, 73, 100–101 for BluetoothDevice.path/device_path, and 128–129). Drop the assignments — if any future refactor breaks the constructor wiring, you want the tests to fail loudly, not paper over it.

    device = UARTDevice("serial0-0")
    device.path = tmp_path / "serial0-0"  # redundant: constructor already set this
    device.setup()

---

Checklist

• Tests cover new/changed branches
• Edge cases covered (missing fields, MODALIAS without comma)
• Tests verify behavior, not implementation — warning ＃1
• Test setup avoids redundant boilerplate — suggestion ＃2
• Test names accurately describe what is tested — warning ＃1
• Async tests use pytest-asyncio consistently — warning ＃1
• No hardcoded secrets or unsafe operations

---

Summary

Coverage expansion from 68% → 100% is welcome and the fake-sysfs approach (writing real uevent files + a device symlink under tmp_path) is the right way to exercise the parser. Two things to address before merge: (1) the executor test @bdraco flagged is genuinely hacky — wrong async harness, monkeypatch on a live loop, misleading name; rewrite with @pytest.mark.asyncio + mock.patch.object(..., wraps=...) or delete it as implementation-detail testing. (2) The repeated device.path = ... reassignments across the setup tests are redundant once UART_DEVICE_PATH is monkeypatched and should be removed. No correctness, security, or perf concerns in the diff itself.

---

To rebase specific severity levels, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or just @bluetoothbot rebase for all.

---

Automated review by Kōan2bbe576
4132645

[bluetoothbot]

Rebase with requested adjustments

Branch koan/test-setup-methods was rebased onto main and review feedback was applied.

Changes applied

• Changes applied. Summary:
• Rewrote test_bluetooth_device_setup_runs_in_executor_via_async per @bluetoothbot's review:
• Renamed to test_uart_device_async_setup_uses_executor (it exercises UARTDevice.async_setup, not BluetoothDevice).
• Switched to @pytest.mark.asyncio harness for consistency with sibling async tests; dropped the nested main() + asyncio.run() pattern.
• Replaced the ad-hoc monkeypatch.setattr(loop, "run_in_executor", tracker) with unittest.mock.patch.object(loop, "run_in_executor", wraps=loop.run_in_executor) so the spy provides assert_called_once() and proper cleanup.
• Added from unittest import mock import.

Stats

1 file changed, 154 insertions(+), 1 deletion(-)

Actions performed

• Already-solved check: negative (confidence=high, reasoning=Recent main commits are all dependency bumps and CI updates; none add test coverage for setup() path)
• Rebased koan/test-setup-methods onto upstream/main
• Applied review feedback
• Pre-push CI check: previous run #26583445354 failed
• Pre-push CI fix: no changes needed or Claude found nothing to fix
• Force-pushed koan/test-setup-methods to origin
• CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.

---

Automated by Kōan