Ludovic Courtès schreef op di 25-01-2022 om 08:54 [+0100]:
> So tests/minetest.scm needs more than pre-programmed responses that are
> returned in a specified order?
> 
> In ideal black-box testing, sure, you would make the test HTTP server
> completely oblivious to what’s being tested, in particular oblivious to
> the order in which requests might arrive.
> 
> But in practice, you also want to keep the test infrastructure as simple
> and concise as possible, or [...]


I think the most concise test infrastructure here, with the least
potential of breakage (and hence the least need to test the test
infrastructure), would be to use mocking instead of creating a HTTP
server listening to a port: 

  * mocking doesn't require threads, which eliminates the problem of
    deciding when to stop the thread (e.g., the current version doesn't
    stop the thread if the thunk throws an exception) and looking out
    for concurrency-related problems.

  * mocking doesn't use ports, which eliminates the problem of having
    to choose a _free_ port and eventually close it

  * somehow, when using mocking instead of threads, the
    ECONNREFUSED/par-map from [PATH 3/9] doesn't happen

'with-http-server' could then be renamed to 'with-http-mocking'
and be based on mocking.  'with-http-server*' could be removed;
tests/minetest.scm would keep mocking directly.

As such, mocking seems a lot simpler to me and 'with-http-server' could
be made simpler by implementing it on top of mocking.  Guile's
optimiser has been beginning to do some inlining for a while,
so maybe not though.

> [...] or you’ll need tests for that infrastructure
> too.  I guess that’s my main concern.

I don't think guix/tests/http.scm has become significantly more complex
and less concise, the changes seem more splitting a procedure doing
multiple somewhat unrelated things (call-with-http-server, which did
both construct a HTTP server and decide what to respond to a request)
into two separate procedures (call-with-http-server*, which constructs
a HTTP server and lets the handler procedure choose how to map requests
to responses, and call-with-http-server's handler procedure).

Additionally, it would seem to me that all tests using
call-with-http-server and call-with-http-server* are also tests of
guix/tests.scm

Still, I could write some tests for (guix tests http)?

> So I would opt for minimal changes.  There are 6 files under tests/
> that mock ‘http-fetch’.  Perhaps we can start converting them one by
> one to the (guix tests http) infrastructure as it exists, and only
> change that infrastructure when needed?

One of these files is tests/minetest.scm.  The main purpose of this
patch series was to convert tests/minetest.scm from mocking to
(guix tests http).  However, the tests in tests/minetest.scm did not
fit the original (guix tests http).  As such, some changes to the
(guix tests http) infrastructure were needed, in [PATCH 1/9].  These
changes seem rather minimal to me.

That said, there might also be other minimal changes possible.
E.g. call-with-packages could generate a map from URI -> response in
advance.  But that would require modifying both tests/minetest.scm
quite a bit and (guix tests http) (to allow optionally ignoring
ordering, adding a new flag and hence some complexity).  That doesn't
seem minimal to me?

It would also make things more complicated later, e.g. I would like to
someday teach the Minetest importer to use http-fetch/cached, If-
Modified-Since and friends to reduce network traffic and some degree of
resiliency (in case of flaky interruptions or even being offline) (*).
To test that, a static URI->response map would not suffice.  Another
tweak to the tests would be to verify the content type (for the
Minetest importer, ContentDB doesn't care currently, but for the GitHub
updater, GitHub does IIUC).

(*) Could be useful for supporting something like

(packages->manifest
  (map specification->imported-package
    '("minetest-not-in-guix-yet@2.1.0" "minetest-mod-old-or-newer-version@9.0.0")))

without incurring excessive network traffic, and having a chance of
working when offline.

Greetings,
Maxime.

p.s. I'll take some time off and write a v2 for the Minetest documentation
patch later (before the v2 of this patch series).