Fix flaky accessible prompter Password test timeout#13304
Conversation
The beforePasswordSendTimeout was set to 100 microseconds, which is insufficient for huh to disable echo mode on the PTY in slow or constrained environments (e.g. network-isolated build containers). Increase to 100 milliseconds to avoid the race condition.
|
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 fixes a flaky TestAccessiblePrompter/Password subtest by increasing the delay used before sending password input, giving the PTY time to have echo disabled before keystrokes are written.
Changes:
- Increase
beforePasswordSendTimeoutfrom100µsto100msto reduce race-related flakes in password/token prompt tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No need for an issue. |
|
I'm gonna approve this because as you say, it's negligible extra cost in test run time. However, do you have a reliable reproduction of this failure? I think I'd prefer to poll the tty to see whether the echo mode has changed instead of having this sleep at all. Something like: func waitForEchoDisabled(t *testing.T, tty *os.File, timeout time.Duration) {
t.Helper()
deadline := time.Now().Add(timeout)
for time.Now().Before(deadline) {
termios, err := unix.IoctlGetTermios(int(tty.Fd()), unix.TIOCGETA)
require.NoError(t, err)
if termios.Lflag&unix.ECHO == 0 {
return
}
time.Sleep(time.Millisecond)
}
t.Fatal("timed out waiting for echo mode to be disabled")
}Since this isn't run on windows. Can look into it myself as a follow up if I have time 😬 |
|
Hello @williammartin, thanks for the approval! The failure happens reliably in the Open Build Service (OBS) where we build openSUSE packages - builds run in isolated containers without network access, which tend to be slower and more resource-constrained. I put together a follow-up PR implementing the polling approach you suggested: #13305. It replaces the sleep with a loop that checks the TTY's termios |
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.
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 #13304.
Summary
beforePasswordSendTimeoutfrom100 * time.Microsecond(0.1ms) to100 * time.Millisecond(100ms) inTestAccessiblePrompterhuhto disable echo mode on the PTY before input was sent, causing the password to be echoed to the terminal and the subsequent regex assertion to failDetails
The
TestAccessiblePrompter/Passwordsubtest sends a password to the terminal after a brief delay (beforePasswordSendTimeout) that giveshuhtime to setEchoModeNoneon the PTY. With only 100 microseconds, the echo mode is not yet disabled when input arrives, so the password appears in the terminal output and the assertion that verifies the password is hidden fails:Increasing to 100 milliseconds adds negligible overhead (at most 300ms total across the 3 uses of this timeout) while providing sufficient headroom for the PTY echo mode to be configured.