Commit 1a8808d
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 42a98f2 commit 1a8808d
1 file changed
+17
-0
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
142 | 142 | | |
143 | 143 | | |
144 | 144 | | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
145 | 162 | | |
146 | 163 | | |
147 | 164 | | |
| |||
0 commit comments