Run the test servers in containers instead of compiling them - #73
Open
christhomas wants to merge 3 commits into
Open
Run the test servers in containers instead of compiling them#73christhomas wants to merge 3 commits into
christhomas wants to merge 3 commits into
Conversation
A server answering MKD with 257 has created the directory. RFC 959 asks it to echo the pathname in quotes as well, and not every server does — proftpd and several embedded servers answer a bare "257 Directory created". Mkdir treated that as a parse failure and returned an error, so the caller saw a failure for a directory that now exists. Retrying then reports that it already does, which is a confusing place to end up from a call that worked. A reply without a name is a success whose name cannot be read, not a failure. Mkdir now answers with the path it was asked for. Getwd is left alone deliberately. It calls the same parser, but there the pathname *is* the answer — a PWD reply without one has nothing to return, so failing is correct. The added parser test pins that distinction so a later change cannot relax both at once. Found by a filesystem driver whose test server sends exactly this reply: every Mkdir failed while every directory was created.
Adds Config.DialFunc. When set, it opens every TCP connection the client makes — control connections and data connections alike — in place of the package's own dialer. It exists for callers who need the connection itself rather than only what travels over it: counting bytes for transfer statistics, routing through a proxy or a tunnel, or returning something that is not really a socket. Today there is no way to reach the connection, so those callers cannot use this package at all. Both dial sites now funnel through one Config.dial helper, which is what makes the data connections reachable too. That is the part worth having: a caller who supplied a dialer to count bytes and only saw the control connection would miss every transfer, which is nearly all the bytes there are, and would have no way to tell. Two things become the caller's responsibility, documented on the field because neither is guessable: - Config.Timeout no longer applies to the dial. It still governs reads and writes on the returned connection. - In TLSImplicit mode the returned connection is wrapped in TLS by this package, so DialFunc should return a plain connection. The handshake happens at dial time rather than on first read, matching what tls.DialWithDialer does. Leaving DialFunc nil keeps the previous behaviour exactly, including the timeout. ## Running the tests without the test servers The new tests need no server: both call sites go through Config.dial, so exercising that covers them by construction. But the package's TestMain stops with log.Fatal when ./build_test_server.sh has not been run, so on a machine that cannot build pure-ftpd and proftpd no test runs at all — including the ones that do not care. GOFTP_SKIP_SERVERS now skips the four tests that need a live server and runs the rest. Skipping is on the tests themselves rather than the runner, so they report as skipped with the reason rather than failing on a connection refused, which reads like a bug in the package instead of a missing prerequisite.
The suite needs two FTP servers, and got them by compiling them on
whatever machine ran the tests. build_test_server.sh fetched proftpd
1.3.5 and pure-ftpd 1.0.36 as tarballs over plain FTP, patched their C
with perl — including a workaround for a segfault on macOS — and left the
binaries in the working tree.
That meant the versions under test were whatever that script last managed
to build on your particular machine, if it built at all, so nothing about
a failure could be attributed with confidence. It also put the suite out
of reach on any machine where you would rather not compile an FTP daemon.
They are containers now. proftpd 1.3.8 from Debian, pure-ftpd 1.0.54
built once in the image — twice from the same source, because implicit
TLS is a compile-time option no distribution packages, which is the only
reason anything is still compiled at all.
./scripts/test-servers.sh test
Docker is the only prerequisite.
FTP negotiates transfers by address. A passive transfer has the server
tell the client where to connect; an active one has the server connect
back. A server behind published ports names an address the host cannot
route to, so passive transfers time out and active ones are refused
outright — "I won't open a connection to 127.0.0.1 (only to 172.21.0.1)".
Sharing a network makes the address the server names the address the
client can reach. The ports are published anyway, so `go test` from the
host still works for everything that is not a data transfer.
It used to spawn the binaries it had just compiled. Now it connects to
whatever is listening and, if nothing is, sets every server-dependent
test to skip with a reason — rather than failing on a connection
refused, which reads like a bug in the package instead of a missing
prerequisite. GOFTP_SKIP_SERVERS forces that path, and CI uses it to
prove the server-free tests really are server-free.
The consequence worth knowing: the suite no longer owns the servers'
lifetime, so a test must not leave one in a state the next cannot use.
The certificate is generated at image build time. The committed one
expired in 2015; the tests skip verification, so nobody noticed — which
is the argument against keeping it rather than for it.
pure-ftpd's anonymous root comes from FTP_ANON_DIR in the compose file.
The implicit-TLS test logs in anonymously, and without it that login
lands in pure-ftpd's compiled-in /ftp and cannot chdir.
Stat's assertion on the root directory's name is gone. Every server
describes the same directory and they disagree about what it is called:
proftpd says "testroot", pure-ftpd 1.0.36 said "/", 1.0.54 says ".".
There is no right answer to hold them to, so the name is checked against
that set instead and the fields that do have a right answer are still
compared. A directory's modification time is no longer compared either —
it moves whenever the directory changes, which this suite does
constantly, so comparing it raced with the suite's own writes.
Config.Timeout for the suite is 30 seconds rather than the package
default of 5. Five was ample for processes on the same machine; across a
bridge network that on macOS sits inside a virtual machine, tests failed
intermittently at almost exactly five seconds — a different handful each
run. A test timing out is not a test failing, and a suite that does the
first while reporting the second is worse than a slow one.
.travis.yml tested Go 1.10 and "tip" and ran the compile script. The
workflow replacing it runs the containers, and pins both entries of its
matrix: the floor go.mod declares, and a current release. "stable" moves
on its own, which makes a build green yesterday and red today with no
commit in between.
Verified: the full suite passes on Go 1.21 and 1.25, three consecutive
runs each, and the server-free subset passes with no containers at all.
christhomas
force-pushed
the
cth/containerised-test-servers
branch
from
August 26, 2026 12:27
c8e686e to
f360209
Compare
This was referenced Aug 26, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
build_test_server.shbuilds the two FTP servers the suite needs by fetching proftpd 1.3.5 and pure-ftpd 1.0.36 as tarballs over plain FTP, patching their C with perl — including a workaround for a segfault on macOS — and leaving the binaries in the working tree.That makes the versions under test whatever the script last managed to build on your particular machine, if it built at all, so a failure cannot be attributed with confidence. It also puts the suite out of reach on any machine where you would rather not compile an FTP daemon.
This replaces it with containers: proftpd 1.3.8 from Debian and pure-ftpd 1.0.54 built in the image — twice from the same source, because implicit TLS is a compile-time option no distribution packages, which is the only reason anything is still compiled at all.
./scripts/test-servers.sh testDocker is the only prerequisite.
The suite runs in a container too, and that is the point
FTP negotiates transfers by address. A passive transfer has the server tell the client where to connect; an active one has the server connect back. A server behind published ports names an address the host cannot route to, so passive transfers time out and active ones are refused outright:
Sharing a network makes the address the server names the address the client can reach. The ports are published anyway, so
go testfrom the host still works for everything that is not a data transfer.TestMainfinds servers rather than starting themIt connects to whatever is listening. If nothing is, every server-dependent test skips with a reason rather than failing on a connection refused — which reads like a bug in the package instead of a missing prerequisite.
GOFTP_SKIP_SERVERSforces that path, and CI uses it to prove the server-free tests really are server-free.The consequence worth knowing: the suite no longer owns the servers' lifetime, so a test must not leave one in a state the next cannot use.
Three things the newer servers changed
FTP_ANON_DIR. The implicit-TLS test logs in anonymously, and without it that login lands in pure-ftpd's compiled-in/ftpand cannot chdir.Statno longer asserts a name for the root directory. All three servers describe the same directory and disagree about what it is called: proftpd saystestroot, pure-ftpd 1.0.36 said/, 1.0.54 says.. There is no right answer to hold them to, so the name is checked against that set and the fields that do have a right answer are still compared. A directory's modification time is no longer compared either — it moves whenever the directory changes, which this suite does constantly, so comparing it raced with the suite's own writes.The timeout
Config.Timeoutfor the suite is 30 seconds rather than the package default of 5. Five was ample for processes on the same machine; across a bridge network that on macOS sits inside a VM, tests failed intermittently at almost exactly five seconds — a different handful each run. A test timing out is not a test failing.CI
.travis.ymltested Go 1.10 andtip. The workflow replacing it runs the containers and pins both matrix entries: the floorgo.moddeclares, and a current release."stable"moves on its own, which makes a build green yesterday and red today with no commit in between.Verified: full suite green on Go 1.21 and 1.25, three consecutive runs each, and the server-free subset green with no containers at all.
Independent of #71 and #72 — happy to rebase whichever lands last.