tests: add test for dec private mode parser#914
Conversation
WalkthroughAdds a new test function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #914 +/- ##
==========================================
+ Coverage 18.56% 19.74% +1.17%
==========================================
Files 39 39
Lines 3404 3404
==========================================
+ Hits 632 672 +40
+ Misses 2717 2681 -36
+ Partials 55 51 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
input_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
input_test.go
🧬 Code graph analysis (1)
input_test.go (1)
event.go (1)
Event(23-26)
🔇 Additional comments (2)
input_test.go (2)
21-21: LGTM!The
fmtimport is correctly added and used for formatting subtest names.
633-648: LGTM!The event handling, type assertions, and timeout logic follow the established patterns in this test file. The error messages provide clear context for debugging test failures.
| // Send null in initial state | ||
| ip.ScanUTF8([]byte(tt.bytes)) |
There was a problem hiding this comment.
Incorrect comment (copy-paste artifact).
The comment says "Send null in initial state" but the code actually sends the private mode escape sequence bytes. This appears to be copied from TestInputStateTransitions.
🔎 Proposed fix
- // Send null in initial state
+ // Send private mode response sequence
ip.ScanUTF8([]byte(tt.bytes))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Send null in initial state | |
| ip.ScanUTF8([]byte(tt.bytes)) | |
| // Send private mode response sequence | |
| ip.ScanUTF8([]byte(tt.bytes)) |
🤖 Prompt for AI Agents
In input_test.go around lines 630 to 631, the inline comment "Send null in
initial state" is incorrect (copy-paste artifact) because the code calls
ip.ScanUTF8([]byte(tt.bytes)) with a private mode escape sequence; update the
comment to accurately describe the action (e.g., "Send private mode escape
sequence bytes" or similar) or remove the misleading comment so it matches the
actual test input being sent.
ce1a08c to
6157216
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
input_test.go (2)
614-626: Unusedusablefield in test struct.The
usablefield is defined in each test case but never used in assertions (line 638 comparespev.usable()tott.result.usable()instead oftt.usable). Either remove the unused field or update the assertion to compare againsttt.usable.🔎 Proposed fix: Update assertion to use tt.usable
- if pev.usable() != tt.result.usable() { - t.Errorf("Private mode usability wrong %v != %v", pev.usable(), tt.result.usable()) + if pev.usable() != tt.usable { + t.Errorf("Private mode usability wrong %v != %v", pev.usable(), tt.usable) }Or alternatively, remove the unused field from the test table:
tests := []struct { bytes string result eventPrivateMode - usable bool }{ - {"\x1b[?1001;0$y", eventPrivateMode{Mode: privateMode(1001)}, false}, - {"\x1b[?1004;2$y", eventPrivateMode{Mode: privateMode(1004), Supported: true}, true}, - {"\x1b[?7;1$y", eventPrivateMode{Mode: privateMode(7), Supported: true, Enabled: true}, true}, - {"\x1b[?25;3$y", eventPrivateMode{Mode: privateMode(25), Supported: true, Enabled: true, Permanent: true}, false}, - {"\x1b[?12;4$y", eventPrivateMode{Mode: privateMode(12), Supported: true, Enabled: false, Permanent: true}, false}, - {"\x1b[?990;$y", eventPrivateMode{Mode: privateMode(990), Supported: false}, false}, - {"\x1b[?991$y", eventPrivateMode{Mode: privateMode(991), Supported: false}, false}, + {"\x1b[?1001;0$y", eventPrivateMode{Mode: privateMode(1001)}}, + {"\x1b[?1004;2$y", eventPrivateMode{Mode: privateMode(1004), Supported: true}}, + {"\x1b[?7;1$y", eventPrivateMode{Mode: privateMode(7), Supported: true, Enabled: true}}, + {"\x1b[?25;3$y", eventPrivateMode{Mode: privateMode(25), Supported: true, Enabled: true, Permanent: true}}, + {"\x1b[?12;4$y", eventPrivateMode{Mode: privateMode(12), Supported: true, Enabled: false, Permanent: true}}, + {"\x1b[?990;$y", eventPrivateMode{Mode: privateMode(990), Supported: false}}, + {"\x1b[?991$y", eventPrivateMode{Mode: privateMode(991), Supported: false}}, }
632-633: Incorrect comment (copy-paste artifact).The comment states "Send null in initial state" but the code sends private mode escape sequence bytes. This appears to be copied from
TestInputStateTransitions.🔎 Proposed fix
- // Send null in initial state + // Send private mode response sequence ip.ScanUTF8([]byte(tt.bytes))
🧹 Nitpick comments (1)
input_test.go (1)
628-628: Consider more descriptive subtest names.The current subtest naming
"case #%d"is generic. Consider including the mode number or expected outcome for better test identification.💡 Example improvement
- t.Run(fmt.Sprintf("case #%d", i), func(t *testing.T) { + t.Run(fmt.Sprintf("mode_%d_%s", tt.result.Mode, tt.bytes), func(t *testing.T) {Or even simpler:
- for i, tt := range tests { - t.Run(fmt.Sprintf("case #%d", i), func(t *testing.T) { + for _, tt := range tests { + t.Run(fmt.Sprintf("mode_%d", tt.result.Mode), func(t *testing.T) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
input_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
input_test.go
🧬 Code graph analysis (1)
input_test.go (3)
key.go (2)
KeyHome(348-348)ModShift(317-317)event.go (1)
Event(23-26)tscreen.go (2)
t(387-426)t(313-360)
🔇 Additional comments (3)
input_test.go (3)
21-21: LGTM!The
fmtimport is appropriately used for formatting subtest names.
561-561: LGTM!The new test case for the rxvt CSI-1-$ sequence is correctly structured and consistent with the existing test cases.
613-652: Well-structured test for DEC private mode responses.The test function correctly validates decoding of DEC private mode response sequences into
eventPrivateModestructures. The test cases cover various combinations of supported/enabled/permanent states, and the timeout mechanism is appropriate.
6157216 to
2bb583b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
input_test.go (1)
633-634: Incorrect comment (copy-paste artifact).The comment says "Send null in initial state" but the code sends a DEC private mode escape sequence (
tt.bytes). This appears to be copied fromTestInputStateTransitionsat line 492.🔎 Proposed fix
- // Send null in initial state + // Send private mode response sequence ip.ScanUTF8([]byte(tt.bytes))
🧹 Nitpick comments (1)
input_test.go (1)
642-644: Consider improving error message clarity.The error message only shows the mode number but not the expected vs actual field values, making debugging more difficult.
🔎 Proposed enhancement
if *pev != tt.result { - t.Errorf("Private mode mismatch for %d", tt.result.Mode) + t.Errorf("Private mode mismatch for mode %d: got %+v, want %+v", tt.result.Mode, *pev, tt.result) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
input_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
input_test.go
🧬 Code graph analysis (1)
input_test.go (2)
key.go (2)
KeyHome(348-348)ModShift(317-317)event.go (1)
Event(23-26)
🔇 Additional comments (2)
input_test.go (2)
21-21: LGTM!The
fmtimport is correctly added and used at line 629 for formatting subtest names.
561-561: LGTM!The new test case for rxvt-specific Home key with Shift modifier is consistent with the existing test structure.
2bb583b to
895b705
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
input_test.go (1)
633-633: Incorrect comment (copy-paste artifact).The comment says "Send null in initial state" but the code actually sends DEC private mode escape sequence bytes from
tt.bytes.🔎 Proposed fix
- // Send null in initial state + // Send DEC private mode response sequence ip.ScanUTF8([]byte(tt.bytes))
🧹 Nitpick comments (1)
input_test.go (1)
643-643: Consider improving error message informativeness.The error message only prints the mode number, making it harder to debug failures. Consider showing both the expected and actual structs.
🔎 Proposed improvement
if *pev != tt.result { - t.Errorf("Private mode mismatch for %d", tt.result.Mode) + t.Errorf("Private mode mismatch: got %+v, want %+v", *pev, tt.result) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
input_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
input_test.go
🧬 Code graph analysis (1)
input_test.go (2)
key.go (2)
KeyHome(348-348)ModShift(317-317)event.go (1)
Event(23-26)
🔇 Additional comments (3)
input_test.go (3)
21-21: LGTM!The
fmtimport is used forfmt.Sprintfat line 629 in the new test function.
561-561: LGTM!The new test case for the rxvt-specific
CSI-1-$sequence is consistent with the existing test structure and properly maps toKeyHomewithModShift.
614-627: LGTM! Good test coverage for DEC private mode responses.The test table covers various response scenarios effectively: unsupported modes, supported-only, enabled modes, and permanent modes. The structure follows the established pattern in the file.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.