test: create initial mock tty for testing#916
Conversation
WalkthroughAdds a new mock terminal implementation for tcell testing: a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #916 +/- ##
==========================================
+ Coverage 19.74% 27.95% +8.20%
==========================================
Files 39 40 +1
Lines 3404 3456 +52
==========================================
+ Hits 672 966 +294
+ Misses 2681 2399 -282
- Partials 51 91 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
mock/mock.go (1)
119-133: Consider renaming toInitorEnsureInit.The method only initializes on first call and is a no-op thereafter. The name "Reset" implies it would reset state on subsequent calls, which is misleading.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mock/mock.go(1 hunks)mock/mock_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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:
mock/mock_test.gomock/mock.go
🧬 Code graph analysis (2)
mock/mock_test.go (2)
mock/mock.go (1)
MockTty(32-54)tscreen.go (1)
NewTerminfoScreenFromTty(136-147)
mock/mock.go (3)
color.go (3)
Color(32-32)ColorWhite(73-73)ColorBlack(58-58)attr.go (2)
AttrMask(22-22)AttrNone(35-35)resize.go (1)
WindowSize(46-51)
🔇 Additional comments (6)
mock/mock.go (5)
24-30: LGTM!The
Cellstruct is well-defined with appropriate fields for representing terminal cell content.
32-54: LGTM!The
MockTtystruct is well-structured with appropriate fields for simulating a terminal. Per the retrieved learnings, synchronization is the caller's responsibility per the Tty interface contract. Based on learnings, the tscreen.go layer handles synchronization for Tty implementations.
56-70: LGTM!The
Stopmethod correctly guards against double-close panics. For a testing mock, the current behavior is acceptable—though note that callingStartmultiple times without interveningStopwould orphan the previousstopQ.
85-94: LGTM!The
Writemethod correctly iterates over the buffer and handles graceful termination viastopQ.
111-117: LGTM!Simple and correct implementations. Per the retrieved learnings, synchronization is the caller's responsibility.
mock/mock_test.go (1)
23-38: Good initial test coverage.The test validates the basic integration between
MockTtyand tcell's terminfo screen, verifying default dimensions. As noted in the PR description, additional tests for command parsing and query responses are planned.
| func (mt *MockTty) Read(b []byte) (int, error) { | ||
|
|
||
| for n := range len(b) { | ||
| select { | ||
| case b[n] = <-mt.ReadQ: | ||
| case <-mt.stopQ: | ||
| return n, nil | ||
| } | ||
| n++ | ||
| } | ||
| return len(b), nil | ||
| } |
There was a problem hiding this comment.
Remove dead code: n++ has no effect.
The for n := range len(b) loop reassigns n on each iteration, making the n++ on line 80 dead code.
🔎 Proposed fix
func (mt *MockTty) Read(b []byte) (int, error) {
-
for n := range len(b) {
select {
case b[n] = <-mt.ReadQ:
case <-mt.stopQ:
return n, nil
}
- n++
}
return len(b), nil
}🤖 Prompt for AI Agents
In mock/mock.go around lines 72 to 83, the loop header "for n := range len(b)"
is invalid and reassigns n each iteration making the explicit "n++" dead;
replace the loop with an index-based loop (e.g., for n := 0; n < len(b); n++)
and remove the extra "n++" inside the body, keep the select filling b[n] from
mt.ReadQ and returning the current index when <-mt.stopQ occurs, and finally
return len(b), nil after successful completion.
| func (mt *MockTty) Drain() error { | ||
| close(mt.stopQ) | ||
| loop: | ||
| for { | ||
| select { | ||
| case <-mt.ReadQ: | ||
| default: | ||
| break loop | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Potential panic: double-close of stopQ.
If Stop() has already been called, stopQ is already closed. Calling close(mt.stopQ) again in Drain() will panic. Apply the same guard pattern used in Stop().
🔎 Proposed fix
func (mt *MockTty) Drain() error {
- close(mt.stopQ)
+ select {
+ case <-mt.stopQ:
+ default:
+ close(mt.stopQ)
+ }
loop:
for {
select {
case <-mt.ReadQ:
default:
break loop
}
}
return nil
}📝 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.
| func (mt *MockTty) Drain() error { | |
| close(mt.stopQ) | |
| loop: | |
| for { | |
| select { | |
| case <-mt.ReadQ: | |
| default: | |
| break loop | |
| } | |
| } | |
| return nil | |
| } | |
| func (mt *MockTty) Drain() error { | |
| select { | |
| case <-mt.stopQ: | |
| default: | |
| close(mt.stopQ) | |
| } | |
| loop: | |
| for { | |
| select { | |
| case <-mt.ReadQ: | |
| default: | |
| break loop | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In mock/mock.go around lines 98 to 109, Drain() closes mt.stopQ unconditionally
which can panic if Stop() already closed it; mirror the guard pattern used in
Stop(): acquire the same mutex, check the "closed" flag (or nil/closed check)
before closing, set the flag and close mt.stopQ only when it hasn't been closed
yet, then proceed with the existing read-queue drain logic.
This is very primitive right now, but we'll add tests. This MockTty is, wait for it, a terminal emulator! (or simulator). More tests will be added, and a full parser for incoming commands, as well as responses from the terminal to queries made. We will also support injecting commands for the purposes of simulation.
798d6c9 to
bdccd18
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mock/mock_test.go (1)
31-39: Deferscreen.Fini()for reliable cleanup.If the size assertion at lines 36-38 fails,
screen.Fini()at line 39 will never execute, potentially leaking resources. Deferring the cleanup ensures it runs regardless of test outcome.🔎 Proposed fix
if err = screen.Init(); err != nil { t.Fatalf("failed to init screen: %v", err) } + defer screen.Fini() w, h := screen.Size() if w != 80 || h != 24 { t.Errorf("bad window size %d x %d", w, h) } - screen.Fini()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mock/mock.go(1 hunks)mock/mock_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mock/mock.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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:
mock/mock_test.go
🧬 Code graph analysis (1)
mock/mock_test.go (2)
mock/mock.go (1)
MockTty(32-54)tscreen.go (1)
NewTerminfoScreenFromTty(136-147)
This is very primitive right now, but we'll add tests. This MockTty is, wait for it, a terminal emulator! (or simulator).
More tests will be added, and a full parser for incoming commands, as well as responses from the terminal to queries made. We will also support injecting commands for the purposes of simulation.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.