Poll TTY echo mode instead of sleeping in password tests#13305
Conversation
|
Thanks for your pull request! Unfortunately, it doesn't meet the minimum requirements for review:
Please update your PR to address the above. Requirements:
This PR will be automatically closed in 7 days if these requirements are not met. |
There was a problem hiding this comment.
Pull request overview
This PR removes a fixed sleep in the accessible prompter password/token tests and replaces it with a deterministic wait that polls the PTY’s echo flag until it’s actually disabled, reducing test flakiness in slow environments.
Changes:
- Replace
time.Sleepbefore sending password/token input with awaitForEchoDisabledpolling helper. - Add OS-specific ioctl constants for retrieving termios flags on Linux and macOS.
- Introduce termios polling via
golang.org/x/sys/unixin the tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/prompter/accessible_prompter_test.go | Adds waitForEchoDisabled and uses it in Password/AuthToken tests instead of a fixed sleep. |
| internal/prompter/echo_test_linux.go | Adds Linux ioctl constant for IoctlGetTermios. |
| internal/prompter/echo_test_darwin.go | Adds macOS ioctl constant for IoctlGetTermios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9afa693 to
119ea9f
Compare
|
This pull request appears to be a resubmission of a recently closed PR (#13305) that did not meet the contribution requirements. Rather than resubmitting, please ensure your contribution:
If you believe this is a different contribution, please comment with details and we'll be happy to reopen. |
|
Oops, didn't triage this in time, but I do want it. |
|
Thanks for your pull request! Unfortunately, it doesn't meet the requirements for review:
Please update your PR to address the above. This PR will be automatically closed in 4 days if these requirements are not met. Full contribution requirements
|
Replace the fixed-duration sleep with a polling loop that checks the actual TTY echo flag before sending password input. This eliminates the race condition where huh has not yet disabled echo mode, which caused flaky test failures in slow environments. Follow-up to cli#13304.
- Rename echo_test_{linux,darwin}.go to echo_{linux,darwin}_test.go so
they are only compiled during tests
- Narrow build tag from !windows to linux || darwin to avoid compile
failures on other Unix platforms
- Return error from waitForEchoDisabled instead of calling t.Fatal,
since the function is called from goroutines where FailNow would only
terminate the calling goroutine
The Go toolchain infers constraints from _darwin/_linux filename suffixes, but explicit //go:build tags make the constraint visible without relying on filename conventions, consistent with modern Go style. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
119ea9f to
a44721d
Compare
Summary
time.Sleepwith a polling loop that checks the actual TTY echo flag before sending password inputhuhhas not yet disabled echo mode, which caused flaky test failures in slow environments (e.g. isolated build containers)Details
The previous approach used a
100 * time.Microsecondsleep to givehuhtime to setEchoModeNoneon the PTY. This was inherently racy - in constrained environments the sleep was too short, and increasing it was just a less-racy guess.The new
waitForEchoDisabledhelper polls the TTY's termios flags every millisecond untilECHOis cleared, with a generous 5-second timeout. This is deterministic - we only proceed once echo mode is actually disabled.Platform-specific ioctl constants (
TCGETSon Linux,TIOCGETAon macOS) are defined in small separate files.golang.org/x/sysis already a dependency.Changed files
internal/prompter/accessible_prompter_test.go- replace sleep with polling, addwaitForEchoDisabledhelperinternal/prompter/echo_test_linux.go- defineioctlGetTermios = unix.TCGETS(new file)internal/prompter/echo_test_darwin.go- defineioctlGetTermios = unix.TIOCGETA(new file)