Skip to content

test: add 34 device exclusion and defseq preservation tests#541

Merged
malpern merged 1 commit into
masterfrom
test/533-device-exclusion-defseq
May 22, 2026
Merged

test: add 34 device exclusion and defseq preservation tests#541
malpern merged 1 commit into
masterfrom
test/533-device-exclusion-defseq

Conversation

@malpern
Copy link
Copy Markdown
Owner

@malpern malpern commented May 22, 2026

Summary

Closes #533

  • KanataDefchordsParser (10 tests): single/multiple groups, chord key extraction, empty config, no defchords, inline comments, timeout token formats, single-line groups, key set computation
  • Chord group references (3 tests): finds references in mappings, empty mappings, no chord actions
  • KanataDefseqParser (7 tests): single/multi format, empty config, no defseq, comments, deduplication, hyphenated names
  • Device exclusion parsing (5 tests): empty input, non-matching lines, VirtualHID detection, physical device exclusion, sorted results
  • Model tests (4 tests): ChordGroupConfig/ChordDefinition Codable round-trips, Equatable, ParsedSequence Equatable
  • Config generation (4 tests): preserved chord groups and sequences included, empty collections omitted
  • Round-trip preservation (2 tests): defchords and defseq survive config regeneration

Test plan

  • All 34 new tests pass
  • Tests run in <1s
  • Build passes

Cover KanataDefchordsParser unit tests (parsing, edge cases, inline
comments, timeout tokens, single-line groups, key sets), chord group
references in mappings, KanataDefseqParser edge cases (single/multi
format, comments, deduplication, hyphens), device exclusion parsing
(empty/non-matching/VirtualHID/physical/sorted), ChordGroupConfig
and ParsedSequence model tests (Codable, Equatable), config
generation with preserved chord groups and sequences, empty
preservation omission, and round-trip tests verifying both defchords
and defseq survive config regeneration.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@malpern malpern merged commit 938d45c into master May 22, 2026
@malpern malpern deleted the test/533-device-exclusion-defseq branch May 22, 2026 15:42
@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

Code Review: PR #541 — 34 device exclusion and defseq preservation tests

Note: This PR is already merged — feedback is retrospective and aimed at informing future test authoring.

Overview

Adds a single 428-line test file covering KanataDefchordsParser, KanataDefseqParser, device exclusion parsing, model round-trips, config generation, and parse→regenerate→reparse round-trips. The intent is solid and the round-trip tests in particular are a strong addition.


Issues

1. Test overlap with existing KanataMacOSDeviceExclusionParserTests

The 5 new device exclusion tests (testParseExcludedDevices_*) overlap significantly with the existing KanataMacOSDeviceExclusionParserTests.swift:

New test Already covered by existing test
testParseExcludedDevices_OnlyNonMatchingLines testParseExcludedMacOSDeviceNamesDeduplicatesAndIgnoresNonMatchingLines
testParseExcludedDevices_VirtualHIDDetected testParseExcludedMacOSDeviceNamesExtractsHashAndProductKey
testParseExcludedDevices_PhysicalDeviceExcluded testParseExcludedMacOSDeviceNamesExtractsHashAndProductKey
testParseExcludedDevices_ResultIsSorted testParseExcludedMacOSDeviceNamesExtractsHashAndProductKey

Only testParseExcludedDevices_EmptyInput is genuinely new. The granular single-assertion style has value, but the duplication adds maintenance surface without coverage gain. Future tests in this area should check the existing file first.

2. Deduplication test doesn't verify which entry wins

func testDefseqParser_DuplicateSequencesDeduped() {
    let config = """
    (defseq hello (a b c))
    (defseq hello (a b c))
    """
    let sequences = KanataDefseqParser.parseSequences(from: config)
    XCTAssertEqual(sequences.count, 1)
}

Both duplicates are identical, so the test can't catch a bug where the second entry silently overwrites the first with different keys. A stronger version would use (defseq hello (x y z)) for the second occurrence and assert which keys are preserved.

3. Class name doesn't match scope

DeviceExclusionDefseqPreservationTests covers six distinct areas (defchords parser, defseq parser, device exclusion, models, config generation, round-trips). Either split into per-feature files (matching the existing KanataMacOSDeviceExclusionParserTests, ChordGroupsIntegrationTests pattern) or choose a broader name like KanataParserPreservationTests. The current name makes tests hard to discover via file-based navigation.

4. Weak contains() assertions in config generation tests

XCTAssertTrue(config.contains("defchords"), "Config should include chord groups")
XCTAssertTrue(config.contains("nav"), "Config should include chord group name")

"nav" as a substring will pass even if it appears in a comment or unrelated block. Consider checking for something more structural like "(defchords nav" to make the assertion meaningful.

5. Missing malformed-input coverage

Parser tests cover empty input and missing blocks, but not malformed input: unbalanced parentheses, missing timeout token, chord group with zero chords, defseq with empty key list (defseq empty ()). Parsers that silently return partial results on malformed input are a source of confusing runtime behavior.


Positives

  • Round-trip tests are excellent. testDefchordsRoundTrip_ParsedGroupsSurviveRegeneration and testDefseqRoundTrip_ParsedSequencesSurviveRegeneration catch the class of bug where serialization changes the shape of data. These are the highest-value tests in the file.
  • #if os(macOS) gating is correct for device exclusion tests.
  • Naming convention (testXxx_Condition) is consistent with the rest of the test suite.
  • MARK sections make the file easy to scan.
  • Sub-1-second runtime is verified per the PR description, satisfying the project's <5s total constraint.
  • testParseGroups_KeySetComputed testing keySet as a Set<String> rather than an ordered array is appropriately robust.

Suggestions for follow-up

  • Remove the 4 overlapping device exclusion tests (or consolidate into KanataMacOSDeviceExclusionParserTests) — they add noise without coverage.
  • Strengthen testDefseqParser_DuplicateSequencesDeduped with differing key sequences.
  • Add at least one malformed-input test per parser to document and lock in graceful-degradation behavior.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: add device exclusion and defseq preservation tests

1 participant