Skip to content

Commit 772caa7

Browse files
committed
ui-tests: do verify the SSH hang fix
In 0ae6a6f (Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader, 2025-06-27), a quite problematic bug was fixed where somewhat large-ish repositories could not be cloned via SSH anymore. This fix was not accompanied by a corresponding test case in Cygwin's test suite, i.e. there is no automated way to ensure that there won't be any regressions on that bug (and therefore it would fall onto end users to deal with those). This constitutes what Michael C. Feathers famously characterized as "legacy code" in his book "Working Effectively with Legacy Code": To me, legacy code is simply code without tests. I've gotten some grief for this definition. What do tests have to do with whether code is bad? To me, the answer is straightforward, and it is a point that I elaborate throughout the book: Code without tests is bad code. It doesn’t matter how well written it is; it doesn’t matter how pretty or object-oriented or well-encapsulated it is. With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don’t know if our code is getting better or worse. Just to drive this point home, let me pull out Exhibit A: The bug fix in question, which is the latest (and hopefully last) commit in a _long_ chain of bug fixes that fix bugs introduced by preceding bug fixes: - 9e4d308 (Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe., 2021-11-10) fixed a bug where Cygwin hung by mistake while piping output from one .NET program as input to another .NET program (potentially introduced by 3651990 (Cygwin: pipe: Avoid false EOF while reading output of C# programs., 2021-11-07), which was itself a bug fix). It introduced a bug that was fixed by... - fc691d0 (Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps., 2024-03-11). Which introduced a bug that was purportedly fixed by... - 7ed9adb (Cygwin: pipe: Switch pipe mode to blocking mode by default, 2024-09-05). Which introduced a bug that was fixed by... - cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06). Which introduced a bug that was fixed by... the SSH hang fix in 0ae6a6f (Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader, 2025-06-27). There is not only the common thread here that each of these bug fixes introduced a new bug, but also the common thread that none of the commits introduced new test cases into the test suite that could potentially have helped prevent future breakages in this code. So let's at least add an integration test here. Side note: I am quite unhappy with introducing integration tests. I know there are a lot of fans out there, but I cannot help wondering whether they favor the convenience of writing tests quickly over the vast cost of making debugging any regression a highly cumbersome and unenjoyable affair (try single-stepping through a test case that requires several processes to be orchestrated in unison). Also, integration tests have the large price of introducing moving parts outside the code base that is actually to be tested, opening the door for breakages caused by software (or infrastructure, think: network glitches!) that are completely outside the power or responsibility of the poor engineer tasked with fixing the breakages. Nevertheless, I have been unable despite days of trying to wrap my head around the issue to figure out a way to reproduce the `fhandler_pipe_fifo::raw_write()` hang without involving a MINGW `git.exe` and an MSYS2/Cygwin `ssh.exe`. So: It's the best I could do with any reasonable amount of effort. It's better to have integration tests that would demonstrate regressions than not having any tests for that at all. Signed-off-by: Johannes Schindelin <[email protected]>
1 parent f6c4c7f commit 772caa7

File tree

1 file changed

+17
-0
lines changed

1 file changed

+17
-0
lines changed

ui-tests/ctrl-c.ahk

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,23 @@ if (openSSHPath != '' and FileExist(openSSHPath . '\sshd.exe')) {
142142

143143
if DirExist(largeGitClonePath)
144144
ExitWithError('`large-clone` was unexpectedly not deleted on interrupt')
145+
146+
; Now verify that the SSH-based clone actually works and does not hang
147+
Info('Re-starting SSH server')
148+
Run(openSSHPath . '\sshd.exe ' . sshdOptions, '', 'Hide', &sshdPID)
149+
if A_LastError
150+
ExitWithError 'Error starting SSH server: ' A_LastError
151+
Info('Started SSH server: ' sshdPID)
152+
153+
Info('Starting clone')
154+
Send('git -c core.sshCommand="ssh ' . sshOptions . '" clone ' . cloneOptions . '{Enter}')
155+
Sleep 500
156+
Info('Waiting for clone to finish')
157+
WinActivate('ahk_id ' . hwnd)
158+
WaitForRegExInWindowsTerminal('Receiving objects: .*, done\.`r?`nPS .*>[ `n`r]*$', 'Timed out waiting for clone to finish', 'Clone finished', 15000, 'ahk_id ' . hwnd)
159+
160+
if not DirExist(largeGitClonePath)
161+
ExitWithError('`large-clone` did not work?!?')
145162
}
146163

147164
Send('exit{Enter}')

0 commit comments

Comments
 (0)