Test suite audit: remove dead E2E tests, run CI on dev, cover relay protocols - #292
Merged
Conversation
The tests/MultiRoomAudio.E2ETests directory had no .csproj, so nothing ever compiled or ran it. It could not have compiled in any case: it references a PlaywrightFixture that exists nowhere in the repo, there is no Playwright package reference, and the files lack the Playwright and xunit usings. Every one of its 21 assertions was Assert.True(true, ...), so all 34 tests would have passed unconditionally even once wired up. Also add MultiRoomAudio.Tests.csproj to the solution's /tests/ folder, which was an empty placeholder. Without it, solution-level build and test silently skipped the entire suite; CI only worked because both workflows invoke dotnet test against the csproj path directly. Verified: Release build clean, 223 tests pass via both the csproj and the solution.
Feature branches target dev, but nothing ran on a pull_request to dev: lint.yml triggered only on main, and docker-publish.yml triggers on push only. Every dev-targeted PR therefore opened with zero CI, and the test suite first ran only after the merge had already landed on dev. Add dev to both triggers in lint.yml so PRs to dev get the same build/format/test gate that PRs to main already get. This also brings the dotnet format --verify-no-changes gate to dev, where it previously fired only at the dev->main merge. Verified the tree is currently format-clean, so it introduces no pre-existing failure.
The LCUS checksum, the Modbus LRC, and the Denkovi FTDI pin maps encode
protocol details that cannot be verified without the physical boards, and
an error in any of them silently switches the wrong relay. None of the
three had any test coverage.
Expected values come from the protocols documented in CLAUDE.md rather
than from the implementations, so the tests fail if either side drifts.
The Denkovi DAE-CB/Ro4-USB odd-pin map (D1/D3/D5/D7) is pinned explicitly,
including a guard against "simplifying" it back to sequential pins.
Three pure functions were widened to internal so they can be exercised
directly; InternalsVisibleTo for the test assembly was already present.
Behavior is unchanged:
- ModbusRelayBoard.BuildModbusCommand: private static -> internal static
- FtdiRelayBoard.GetBitMaskForChannel: private instance -> internal
static taking channelCount
- LcusRelayBoard.BuildCommand: extracted from the inline frame build in
SetRelay, which required an open serial port
Verified: 246 tests pass (223 + 23 new), Release build clean, dotnet
format makes no changes to the edited files. Mutating the odd-pin map,
the LCUS checksum, and the LRC negation fails 18 of the 23 new tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audits the test suite, removes dead weight, closes a CI gap, and adds coverage for the relay wire protocols.
1. Remove dead E2E tests
tests/MultiRoomAudio.E2ETests/(1,141 lines, 34[Fact]s) had no.csproj, so nothing ever compiled or ran it. It could not have compiled in any case: it references aPlaywrightFixturethat exists nowhere in the repo, there is no Playwright package reference, and the files lack the Playwright and xunit usings.More to the point, all 21 of its assertions were
Assert.True(true, ...)— each test computed a real value, then discarded it into the failure message while asserting the literaltrue. Even fully wired up, all 34 would have passed against a completely broken app.Also adds
MultiRoomAudio.Tests.csprojto the solution's/tests/folder, which was an empty placeholder. Without it, solution-level build and test silently skipped the entire suite.2. Run CI on dev
lint.ymltriggered only onmain, anddocker-publish.ymlfires onpushonly — so a PR targetingdevran no CI at all, and the suite first ran only after the merge had already landed. Addsdevto both triggers inlint.yml.This also brings the
dotnet format --verify-no-changesgate to dev PRs, where it previously fired only at the dev to main merge. The tree was verified format-clean first, so it introduces no pre-existing failure.3. Relay wire protocol tests (+23)
The LCUS checksum, the Modbus LRC, and the Denkovi FTDI pin maps encode protocol details that cannot be verified without the physical boards, and an error in any of them silently switches the wrong relay. None had coverage.
Expected values come from the protocol tables documented in
CLAUDE.mdrather than from the implementations, so the tests fail if either side drifts. The Denkovi DAE-CB/Ro4-USB odd-pin map (D1/D3/D5/D7) is pinned explicitly, with a guard against "simplifying" it back to sequential pins.Three pure functions were widened to
internalso they can be exercised directly (InternalsVisibleTofor the test assembly already existed). Behavior is unchanged.Verification
dotnet formatmakes no changes to the edited filesNote
This is the first PR in this repo that CI will actually run on.