[bug#53389,1/9] tests: Support arbitrary HTTP request handlers.

An incompatible change to with-http-server has been made: it now
also exits when the thunk exits.  This change allows implementing
with-http-server*.  It also keeps threads from lingering if the
thunk doesn't access all of RESPONSES+DATA.

Usually, this change is fine, but it does not interact nicely with
monads in tests/challenge, so a variant with-http-server/lingering
preserving the old behaviour has been defined.

* guix/tests/http.scm
  (call-with-http-server): Extract most functionality to ...
  (call-with-http-server*): ... this new procedure.  Also stop the
  server thread after 'thunk' returns instead of when the last response
  has been sent unless requested not to.
  (with-http-server/keep-lingering): New macro.
* tests/challenge.scm (call-mismatch-test): Use the 'keep-lingering'
  variant of 'with-http-server'.
---
 guix/tests/http.scm | 96 +++++++++++++++++++++++++++++++--------------
 tests/challenge.scm | 24 ++++++------
 2 files changed, 80 insertions(+), 40 deletions(-)


base-commit: 1bd250783d7118c3101dd2a6e090f3d6904b24a0

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> An incompatible change to with-http-server has been made: it now
> also exits when the thunk exits.  This change allows implementing
> with-http-server*.  It also keeps threads from lingering if the
> thunk doesn't access all of RESPONSES+DATA.
>
> Usually, this change is fine, but it does not interact nicely with
> monads in tests/challenge, so a variant with-http-server/lingering
> preserving the old behaviour has been defined.
>
> * guix/tests/http.scm
>   (call-with-http-server): Extract most functionality to ...
>   (call-with-http-server*): ... this new procedure.  Also stop the
>   server thread after 'thunk' returns instead of when the last response
>   has been sent unless requested not to.
>   (with-http-server/keep-lingering): New macro.
> * tests/challenge.scm (call-mismatch-test): Use the 'keep-lingering'
>   variant of 'with-http-server'.

[...]

>    #:export (with-http-server
> +            with-http-server/keep-lingering
> +            with-http-server*
>              call-with-http-server
> +            call-with-http-server*
>              %http-server-port
>              %local-url))

My first reaction was: have we gone overboard?  :-)

Since it’s an internal module and a test helper, I’m in favor of keeping
it as simple as possible.  Can we keep a single ‘with-http-server’ form
that would cover all cases?

We can update existing tests to include the expected URL path (or a
wildcard, if needed), instead of keeping several forms.  We don’t need
to worry about backward compatibility at all.

> +        (unless keep-lingering?
> +          ;; exit the server thread
> +          (system-async-mark (lambda () (throw 'quit)) server))

When do we need ‘keep-lingering?’?  So far, all uses of
‘with-http-server’ expected that the server would quit once the last
response has been sent.  It would be nice if we could keep it that way.

> +        (apply values results)))))
> +
> +
> +(define* (call-with-http-server responses+data thunk #:key (keep-lingering? #false))
> +  "Call THUNK with an HTTP server running and returning RESPONSES+DATA
> +on HTTP requests.  Each element of RESPONSES+DATA must be a tuple containing a
> +response and a string, or an HTTP response code and a string.
> +
> +The argument RESPONSES+DATA is thunked.  As such, RESPONSES+DATA can use
> +%http-server-port.  %http-server-port will be set to the port listened at.
> +It will be set for the dynamic extent of THUNK and RESPONSES+DATA.
> +
> +The server will exit after the last response.  When KEEP-LINGERING? is false,
> +the server will also exit after THUNK returns."

Within tests, it would be nice if we could avoid using the ‘thunk’ form
and instead always use the declarative form (list of URL path/response
code/response body).  That should make the tests more concise and
readable.

Or are there new uses where the declarative form is insufficiently
expressive?

Thanks,
Ludo’.

Maxime Devos schreef op do 20-01-2022 om 13:08 [+0000]:
> -      (when (null? responses)
> -        (quit #t))                                ;exit the server thread
> +      (when (last-response?)
> +        (throw 'quit))

(quit #t) can be preserved here (TBD in v2)

Hi,

Ludovic Courtès schreef op za 22-01-2022 om 17:48 [+0100]:
> > +        (unless keep-lingering?
> > +          ;; exit the server thread
> > +          (system-async-mark (lambda () (throw 'quit)) server))
> 
> When do we need ‘keep-lingering?’?

In tests/challenge.scm (call-mismatch-test), due to how the store monad
work, the thunk technically returns (*) before we are done with the
querying the server.  Perhaps this can be resolved with sufficient
monadology, but I don't quite see how.

(*) a monadic value.

> So far, all uses of ‘with-http-server’ expected that the server would
> quit once the last response has been sent.

AFAIK, they don't expect that the server quits. But they don't expect
that the server does not quit either.  Rather, they need the server
to keep running as long as needed -- i.e., after the last request
has been answered.

The only reason that the server quits, is to perform a form of garbage
collection, to avoid accumulating threads and server ports.

There appear to be two criteria for deciding when to exit the server:

 (a) Exit when the thunk returns. 

     This is similar to 'call-with-port' and 'call-with-output-file'
     automatically closing the port when the procedure returns.
     There don't seem to be any drawbacks with this criterium.

 (b) Exit when there are no responses left.

     This is problematic when there is no list of reponses but rather
     some function mapping requests to responses without any
     limitations on how often a resource is requested, or when there
     are multiple resources available and the ‘client’ is allowed to
     query a proper subset ...

     E.g., the way the tests in tests/minetest.scm are written, the
     tests don't care in which order resources are accessed and whether
     a resource is accessed multiple time.  Furthermore, the procedure
     for creating a testing model of ContentDB (call-with-packages) has
     no idea what parts of the model will be accessed.

     That is, the same model can be used for searches (either
     sorted by score or download), for requesting a description of a
     ContentDB ‘package’ and for requesting a specific release of a
     package.

     Furthermore, the space of resources is even infinite (due to the
     search API).

     In principle, that procedure could be modified to accept a few
     arguments specifying what things will be asked and with
     which parameters, but doing that and figuring out the arguments
     for each test would be rather tedious.

     Aside from verifying that no more traffic than strictly necessary
     happens (which would be a nice property but not really required),
     I don't see the point of verifying which resources exactly are
     queried.

     Furthermore, which resources precisely are queried and how often,
     seems more of an implementation detail to me.  I would rather
     focus on the end result: verifying that the imported package
     definition is what is expected.

     Most tests in tests/minetest.scm are like that: they tell
     call-with-packages to create a ContentDB model with some data
     (using make-package-json etc.) ask (guix import minetest) to
     import some package from the model and compare the result with a
     prediction (make-package-sexp) (can this called integration
     testing?).

     These tests do not care how (guix import minetest) works -- they
     don't care about which resources are queried.  Instead, they
     simply test that the end result (the package definition) is as
     predicted.

While criterium (b) might suffice for various unit tests
(e.g. in tests/lint.scm), it is rather impractical and limiting
for the kind of tests that tests/minetest.scm does.

OTOH, criterium (a) not only suffices for tests/lint.scm-style tests,
but also for tests/minetest.scm-style tests.  It seems to work
everywhere, except for the single exception tests/challenge.scm. 

> It would be nice if we could keep it that way.

Compare (a) and (b), then I think it will be easy to infer what I would
prefer 😉.

Greetings,
Maxime.

Ludovic Courtès schreef op za 22-01-2022 om 17:48 [+0100]:
> > +(define* (call-with-http-server responses+data thunk #:key (keep-
> > lingering? #false))
> > +  "Call THUNK with an HTTP server running and returning
> > RESPONSES+DATA
> > +on HTTP requests.  Each element of RESPONSES+DATA must be a tuple
> > containing a
> > +response and a string, or an HTTP response code and a string.
> > +
> > +The argument RESPONSES+DATA is thunked.  As such, RESPONSES+DATA
> > can use
> > +%http-server-port.  %http-server-port will be set to the port
> > listened at.
> > +It will be set for the dynamic extent of THUNK and RESPONSES+DATA.
> > +
> > +The server will exit after the last response.  When KEEP-
> > LINGERING? is false,
> > +the server will also exit after THUNK returns."
> 
> Within tests, it would be nice if we could avoid using the ‘thunk’
> form

We can't unthunk 'thunk', otherwise the code querying the server will
be run before the server starts, which isn't very useful.  It's the
same reasn why unthunking the 'thunk' argument of 'with-output-to-file'
is not useful -- unless you only want to make a file empty I suppose.

Did you mean 'responses+data'?  For some context, consider
tests/cpan.scm:

-  (with-http-server `((200 ,test-json)
-                      (200 ,test-source)
-                      (200 "{ \"distribution\" : \"Test-Script\" }"))
-    (parameterize ((%metacpan-base-url (%local-url))
-                   (current-http-proxy (%local-url)))
+  (with-http-server `(("/release/Foo-Bar" 200 ,(test-json))
+                      ("/Foo-Bar-0.1.tar.gz" 200 ,test-source)
+                      ("/module/Test::Script?fields=distribution"
+                       200 "{ \"distribution\" : \"Test-Script\" }"))
+    (parameterize ((%metacpan-base-url (%local-url* ""))
+                   (current-http-proxy #false))

(Side note: should parametrising current-http-proxy be moved into
'with-http-server', to avoid forgetting to do it in individual tests?)

This 'with-http-server' is ‘self-referrent’: the responses depend on
the port that the HTTP server bound to -- (test-json) refers to
http://localhost:THE-PORT/Foo-Bar-0.1.tar.gz.  As such, the
responses+data needs to be thunked, because this port is not known in
advance.

In principle, thunking can be avoided here by running two HTTP servers
by nesting two with-http-server forms, but why make things more
complicated by running multiple servers when a single one is
sufficient?  It can also be avoided by doing this proxy thing the
original code did, but why complicate things with proxies when
a regular server suffices?

Also, tests don't really see that thunking happens unless they actually
use the thunking to do make self-references, because all tests use
with-http-server (*) (which does thunking automatically, not unlike
'package' records).  So except for simplifying implementation details
of call-with-http-server, I don't see how unthunking would make things
nicer.

(*) Unless they use with-http-server*.

>  and instead always use the declarative form (list of URL
> path/response code/response body).

Thunking and declarative forms don't appear mutually exclusive to me;
the tests/cpan.scm example above uses a thunked declarative form.

It would be nice to always specify the paths in the declarative
form though, to make tests more precise, but there are a lot
of tests to convert.

>  That should make the tests more concise and readable.

The declarative form is still available and isn't going away.

> 
> Or are there new uses where the declarative form is insufficiently
> expressive?

tests/minetest.scm, see other mail.

with-http-server is still declarative, so maybe you meant the
functional with-http-server* instead?

Greetings,
Maxime

Ludovic Courtès schreef op za 22-01-2022 om 17:48 [+0100]:
> My first reaction was: have we gone overboard?  :-)
> 
> Since it’s an internal module and a test helper, I’m in favor of keeping
> it as simple as possible.
> 

I don't see what it matters that this module is only available from
a git checkout (or extracted tarball) and that it is only used by
tests.

Keeping things simple is good, but making it simpler in such a way
that it becomes unusable to some tests (tests/minetest.scm) somewhat
defeats the purpose of the test helper.

>   Can we keep a single ‘with-http-server’ form
> that would cover all cases?

We have a single form that covers all cases: with-http-server*.
However, the full power of the functinal with-http-server*, accepting
an arbitrary mapping from requests to responses, often isn't necessary.
For those cases, we have the declarative with-http-server, which is
quite a bit simpler to use, but much less flexible.

We could remove 'with-http-server' and keep a single macro
'with-http-server*' but I don't think that's what you were going for.

This seems a bit like trivial-build-system/copy-build-system.
trivial-build-system is rather complicated to use, but can in theory do
anything.  copy-build-system is rather limited in what it can do, but
when it is suitable to the problem, it is very easy to use.
There is no attempt to somehow shove everything trivial-build-system
can do into copy-build-system.

There's also the option of letting 'call-with-http-server' test
if the (responses+data) is a procedure or a list, and in the first
case behave like the old 'with-http-server*' and in the second
case like 'with-http-server'.  This overloading doesn't seem
great though, I would rather have two separate procedures for
two separate APIs -- albeit with one implemented with the other.

> We can update existing tests to include the expected URL path (or a
> wildcard, if needed), instead of keeping several forms.  We don’t need
> to worry about backward compatibility at all.

Always including the URL path in the declarative forms
(with-http-server) seems good to me, but it would be a lot of work
-- actually, on second though, I ran
"git grep -F with-http-server | wc -l" and there were 51 hits, which
seems doable.  Let's do that in the v2.

However, the declarative form is too limiting and not sufficiently
expressive for some tests (tests/minetest.scm) and tests/minetest.scm
doesn't have any use for wildcards, so with-http-server remains
unsuitable for some tests.

Greetings,
Maxime.

Ludovic Courtès schreef op za 22-01-2022 om 17:48 [+0100]:
> My first reaction was: have we gone overboard?  :-)
> 
> Since it’s an internal module and a test helper, I’m in favor of keeping
> it as simple as possible.
> 

I don't see what it matters that this module is only available from
a git checkout (or extracted tarball) and that it is only used by
tests.

Keeping things simple is good, but making it simpler in such a way
that it becomes unusable to some tests (tests/minetest.scm) somewhat
defeats the purpose of the test helper.

>   Can we keep a single ‘with-http-server’ form
> that would cover all cases?

We have a single form that covers all cases: with-http-server*.
However, the full power of the functinal with-http-server*, accepting
an arbitrary mapping from requests to responses, often isn't necessary.
For those cases, we have the declarative with-http-server, which is
quite a bit simpler to use, but much less flexible.

We could remove 'with-http-server' and keep a single macro
'with-http-server*' but I don't think that's what you were going for.

This seems a bit like trivial-build-system/copy-build-system.
trivial-build-system is rather complicated to use, but can in theory do
anything. copy-build-system is rather limited in what it can do, but
when it is suitable to the problem, it is very easy to use.
There is no attempt to somehow move everything trivial-build-system
can do into copy-build-system.

There's also the option of letting 'call-with-http-server' test
if the (responses+data) is a procedure or a list, and in the first
case behave like the old 'with-http-server*' and in the second
case like 'with-http-server'.  This overloading doesn't seem
great though, I would rather have two separate procedures for
two separate APIs -- albeit with one implemented using the other.

> 
> We can update existing tests to include the expected URL path (or a
> wildcard, if needed), instead of keeping several forms.  We don’t need
> to worry about backward compatibility at all.

Always including the URL path in the declarative forms
(with-http-server) seems good to me, but it would be a lot of work
-- actually, on second though, I ran
"git grep -F with-http-server | wc -l" and there were 51 hits, which
seems doable. Let's do that in the v2.

However, the declarative form is too limiting and not sufficiently
expressive for some tests (tests/minetest.scm), tests/minetest.scm
doesn't have any use for wildcards, so with-http-server remains
unsuitable for some tests.

Greetings,
Maxime.

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op za 22-01-2022 om 17:48 [+0100]:
>> > +        (unless keep-lingering?
>> > +          ;; exit the server thread
>> > +          (system-async-mark (lambda () (throw 'quit)) server))
>> 
>> When do we need ‘keep-lingering?’?
>
> In tests/challenge.scm (call-mismatch-test), due to how the store monad
> work, the thunk technically returns (*) before we are done with the
> querying the server.  Perhaps this can be resolved with sufficient
> monadology, but I don't quite see how.
>
> (*) a monadic value.

Oh I see.

>> So far, all uses of ‘with-http-server’ expected that the server would
>> quit once the last response has been sent.
>
> AFAIK, they don't expect that the server quits. But they don't expect
> that the server does not quit either.  Rather, they need the server
> to keep running as long as needed -- i.e., after the last request
> has been answered.
>
> The only reason that the server quits, is to perform a form of garbage
> collection, to avoid accumulating threads and server ports.

Yes.

> There appear to be two criteria for deciding when to exit the server:
>
>  (a) Exit when the thunk returns. 
>
>      This is similar to 'call-with-port' and 'call-with-output-file'
>      automatically closing the port when the procedure returns.
>      There don't seem to be any drawbacks with this criterium.
>
>  (b) Exit when there are no responses left.
>
>      This is problematic when there is no list of reponses but rather
>      some function mapping requests to responses without any
>      limitations on how often a resource is requested, or when there
>      are multiple resources available and the ‘client’ is allowed to
>      query a proper subset ...
>
>      E.g., the way the tests in tests/minetest.scm are written, the
>      tests don't care in which order resources are accessed and whether
>      a resource is accessed multiple time.  Furthermore, the procedure
>      for creating a testing model of ContentDB (call-with-packages) has
>      no idea what parts of the model will be accessed.

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 you’ll need tests for that infrastructure
too.  I guess that’s my main concern.

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?

Thanks,
Ludo’.

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).