[PATCH]: tests: do not hard code HTTP ports

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Maxime Devos
Owner
unassigned
Submitted by
Maxime Devos
Severity
normal
M
M
Maxime Devos wrote on 20 Feb 2021 23:00
(address . guix-patches@gnu.org)
1728b9290c1b0e248b71c8b35623939853895a7f.camel@telenet.be
Hi Guix,

I encountered a ‘Cannot listen to port: is bound’ error
or something like that when testing some changes to the
substituter. I'm not sure yet what the cause is, but this
patch should eliminate some potential trouble, by not
hard-coding ports in tests that require a temporary http
server, and instead automatically assigning a free port.

Greetings,
Maxime
-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYDGGfxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7g8WAP9mcb02py49RvzgdTX8o0vtn0Nv
QtDzjdiv68xrpoVHxAEAostXqVhDcQQseJyPxAwR/lL4bC1jRueYWNukJm31jQ8=
=nH5f
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 1 Mar 2021 16:46
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 46668@debbugs.gnu.org)
87r1kyri5l.fsf@gnu.org
Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (33 lines)
> From 6a5ea1f1a9155e23e46a38577adf74527ba50b2c Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sat, 20 Feb 2021 22:04:59 +0100
> Subject: [PATCH] tests: do not hard code HTTP ports
>
> Previously, test cases could fail if some process was listening
> at a hard-coded port. This patch eliminates most of these potential
> failures, by automatically assigning an unbound port. This should
> allow for building multiple guix trees in parallel outside a build
> container, though this is currently untested.
>
> The test "home-page: Connection refused" in tests/lint.scm still
> hardcodes port 9999, however.
>
> * guix/tests/http.scm
> (http-server-can-listen?): remove now unused procedure.
> (%http-server-port): default to port 0, meaning the OS
> will automatically choose a port.
> (open-http-server-socket): remove the false statement claiming
> this procedure is exported and also return the allocated port
> number.
> (%local-url): raise an error if the port is obviously unbound.
> (call-with-http-server): set %http-server-port to the allocated
> port while the thunk is called.
> * tests/derivations.scm: adjust test cases to use automatically
> assign a port. As there is no risk of a port conflict now,
> do not make any tests conditional upon 'http-server-can-listen?'
> anymore.
> * tests/elpa.scm: likewise.
> * tests/lint.scm: likewise, and add a TODO comment about a port
> that is still hard-coded.
> * tests/texlive.scm: likewise.

Nice!

Some comments below.

Toggle quote (2 lines)
> + #:use-module (ice-9 receive)

Please use (srfi srfi-71) instead, or (srfi srfi-11).

Toggle quote (36 lines)
> -(unless (http-server-can-listen?)
> - (test-skip 1))
> (test-assert "'download' built-in builder, check mode"
> ;; Make sure rebuilding the 'builtin:download' derivation in check mode
> ;; works. See <http://bugs.gnu.org/25089>.
> - (let* ((text (random-text))
> - (drv (derivation %store "world"
> - "builtin:download" '()
> - #:env-vars `(("url"
> - . ,(object->string (%local-url))))
> - #:hash-algo 'sha256
> - #:hash (gcrypt:sha256 (string->utf8 text)))))
> - (and (with-http-server `((200 ,text))
> - (build-derivations %store (list drv)))
> - (with-http-server `((200 ,text))
> - (build-derivations %store (list drv)
> - (build-mode check)))
> - (string=? (call-with-input-file (derivation->output-path drv)
> - get-string-all)
> - text))))
> + (let* ((text (random-text)))
> + (with-http-server `((200 ,text))
> + (let ((drv (derivation %store "world"
> + "builtin:download" '()
> + #:env-vars `(("url"
> + . ,(object->string (%local-url))))
> + #:hash-algo 'sha256
> + #:hash (gcrypt:sha256 (string->utf8 text)))))
> + (and drv (build-derivations %store (list drv))
> + (with-http-server `((200 ,text))
> + (build-derivations %store (list drv)
> + (build-mode check)))
> + (string=? (call-with-input-file (derivation->output-path drv)
> + get-string-all)
> + text))))))

This hunk shouldn’t be here.

Toggle quote (17 lines)
> -(test-equal "home-page: Connection refused"
> - "URI http://localhost:9999/foo/bar unreachable: Connection refused"
> - (let ((pkg (package
> - (inherit (dummy-package "x"))
> - (home-page (%local-url)))))
> - (single-lint-warning-message
> - (check-home-page pkg))))
> +(parameterize ((%http-server-port 9999))
> + ;; TODO skip this test if some process is currently listening at 9999
> + (test-equal "home-page: Connection refused"
> + "URI http://localhost:9999/foo/bar unreachable: Connection refused"
> + (let ((pkg (package
> + (inherit (dummy-package "x"))
> + (home-page (%local-url)))))
> + (single-lint-warning-message
> + (check-home-page pkg)))))

Likewise.

Toggle quote (8 lines)
> -(test-equal "home-page: 200 but short length"
> - "URI http://localhost:9999/foo/bar returned suspiciously small file (18 bytes)"
> - (with-http-server `((200 "This is too small."))
> +(with-http-server `((200 "This is too small."))
> + (test-equal "home-page: 200 but short length"
> + (format #f "URI ~a returned suspiciously small file (18 bytes)"
> + (%local-url))

Likewise.

Toggle quote (7 lines)
> -(test-equal "home-page: 404"
> - "URI http://localhost:9999/foo/bar not reachable: 404 (\"Such is life\")"
> - (with-http-server `((404 ,%long-string))
> +(with-http-server `((404 ,%long-string))
> + (test-equal "home-page: 404"
> + (format #f "URI ~a not reachable: 404 (\"Such is life\")" (%local-url))

Likewise.

Toggle quote (7 lines)
> -(test-equal "home-page: 301, invalid"
> - "invalid permanent redirect from http://localhost:9999/foo/bar"
> - (with-http-server `((301 ,%long-string))
> +(with-http-server `((301 ,%long-string))
> + (test-equal "home-page: 301, invalid"
> + (format #f "invalid permanent redirect from ~a" (%local-url))

Likewise.

Toggle quote (17 lines)
> -(test-equal "home-page: 301 -> 200"
> - "permanent redirect from http://localhost:10000/foo/bar to http://localhost:9999/foo/bar"
> - (with-http-server `((200 ,%long-string))
> - (let* ((initial-url (%local-url))
> - (redirect (build-response #:code 301
> - #:headers
> - `((location
> - . ,(string->uri initial-url))))))
> - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> - (with-http-server `((,redirect ""))
> +(with-http-server `((200 ,%long-string))
> + (let* ((initial-url (%local-url))
> + (redirect (build-response #:code 301
> + #:headers
> + `((location
> + . ,(string->uri initial-url))))))

Likewise.

Toggle quote (12 lines)
> -(test-equal "home-page: 301 -> 404"
> - "URI http://localhost:10000/foo/bar not reachable: 404 (\"Such is life\")"
> - (with-http-server '((404 "booh!"))
> - (let* ((initial-url (%local-url))
> - (redirect (build-response #:code 301
> - #:headers
> - `((location
> - . ,(string->uri initial-url))))))
> - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> - (with-http-server `((,redirect ""))
> +(with-http-server `((404 "booh!"))

Likewise.

Toggle quote (8 lines)
> -(test-equal "source: 200 but short length"
> - "URI http://localhost:9999/foo/bar returned suspiciously small file (18 bytes)"
> - (with-http-server '((200 "This is too small."))
> +(with-http-server '((200 "This is too small."))
> + (test-equal "source: 200 but short length"
> + (format #f "URI ~a returned suspiciously small file (18 bytes)"
> + (%local-url))

Likewise.

Toggle quote (8 lines)
> -(test-equal "source: 404"
> - "URI http://localhost:9999/foo/bar not reachable: 404 (\"Such is life\")"
> - (with-http-server `((404 ,%long-string))
> +(with-http-server `((404 ,%long-string))
> + (test-equal "source: 404"
> + (format #f "URI ~a not reachable: 404 (\"Such is life\")"
> + (%local-url))

Likewise.

Toggle quote (12 lines)
> -(test-equal "source: 301 -> 200"
> - "permanent redirect from http://localhost:10000/foo/bar to http://localhost:9999/foo/bar"
> - (with-http-server `((200 ,%long-string))
> - (let* ((initial-url (%local-url))
> - (redirect (build-response #:code 301
> - #:headers
> - `((location
> - . ,(string->uri initial-url))))))
> - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> - (with-http-server `((,redirect ""))
> +(with-http-server `((200 ,%long-string))

Likewise.

Toggle quote (12 lines)
> -(test-equal "source, git-reference: 301 -> 200"
> - "permanent redirect from http://localhost:10000/foo/bar to http://localhost:9999/foo/bar"
> - (with-http-server `((200 ,%long-string))
> - (let* ((initial-url (%local-url))
> - (redirect (build-response #:code 301
> - #:headers
> - `((location
> - . ,(string->uri initial-url))))))
> - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> - (with-http-server `((,redirect ""))
> +(with-http-server `((200 ,%long-string))

Likewise.

Toggle quote (12 lines)
> -(test-equal "source: 301 -> 404"
> - "URI http://localhost:10000/foo/bar not reachable: 404 (\"Such is life\")"
> - (with-http-server '((404 "booh!"))
> - (let* ((initial-url (%local-url))
> - (redirect (build-response #:code 301
> - #:headers
> - `((location
> - . ,(string->uri initial-url))))))
> - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> - (with-http-server `((,redirect ""))
> +(with-http-server '((404 "booh!"))

Likewise.

Could you send an updated patch?

Thanks!

Ludo’.
M
M
Maxime Devos wrote on 1 Mar 2021 18:23
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 46668@debbugs.gnu.org)
3ae8be5f05d73106c867fe2cfc7f619673fd738b.camel@telenet.be
On Mon, 2021-03-01 at 16:46 +0100, Ludovic Courtès wrote:
Toggle quote (11 lines)
> [...]
> Maxime Devos <maximedevos@telenet.be> skribis:
> > [...]
>
> Nice!
>
> Some comments below.
>
> > + #:use-module (ice-9 receive)
> Please use (srfi srfi-71) instead, or (srfi srfi-11).

Sure, will do.

You made some comments about ‘Hunks that shouldn't be here’ below.
I disagree. As my explanation is exactly the same for almost all hunks,
I've numbered them and the explanations.

Explanations:

A. (Hunk 2--12, i.e. all hunks except the first)
In some tests, the port number is hardcoded.
E.g., you'll see (test-equal "Some string http://localhost:9999" expression).
Removing the hard-coding is the whole point of this patch.
B. See later (hunk #1).
C. See later (hunk #2).

Hunk #1.
Toggle quote (38 lines)
> > -(unless (http-server-can-listen?)
> > - (test-skip 1))
> > (test-assert "'download' built-in builder, check mode"
> > ;; Make sure rebuilding the 'builtin:download' derivation in check mode
> > ;; works. See <http://bugs.gnu.org/25089>;.
> > - (let* ((text (random-text))
> > - (drv (derivation %store "world"
> > - "builtin:download" '()
> > - #:env-vars `(("url"
> > - . ,(object->string (%local-url))))
> > - #:hash-algo 'sha256
> > - #:hash (gcrypt:sha256 (string->utf8 text)))))
> > - (and (with-http-server `((200 ,text))
> > - (build-derivations %store (list drv)))
> > - (with-http-server `((200 ,text))
> > - (build-derivations %store (list drv)
> > - (build-mode check)))
> > - (string=? (call-with-input-file (derivation->output-path drv)
> > - get-string-all)
> > - text))))
> > + (let* ((text (random-text)))
> > + (with-http-server `((200 ,text))
> > + (let ((drv (derivation %store "world"
> > + "builtin:download" '()
> > + #:env-vars `(("url"
> > + . ,(object->string (%local-url))))
> > + #:hash-algo 'sha256
> > + #:hash (gcrypt:sha256 (string->utf8 text)))))
> > + (and drv (build-derivations %store (list drv))
> > + (with-http-server `((200 ,text))
> > + (build-derivations %store (list drv)
> > + (build-mode check)))
> > + (string=? (call-with-input-file (derivation->output-path drv)
> > + get-string-all)
> > + text))))))
>
> This hunk shouldn’t be here.

Explanation #B: If the hunk wasn't applied, then the first %local-url won't
work, as no web server is running yet (so we cannot yet know the port to
include in %local-url). I've added a check to %local-url to throw an
exception when %http-server-port is 0 to prevent silently returning
"http://localhost:0/foo/bar",which is rather meaningless.

The "let" and "with-http-server" forms needed to be restructured,
and the %local-url of the first server needed to be saved with a "let",
to use the URL of the correct HTTP server. There are two HTTP servers
in this test ...

Hunk #2.
Toggle quote (19 lines)
> > -(test-equal "home-page: Connection refused"
> > - "URI http://localhost:9999/foo/bar unreachable: Connection refused"
> > - (let ((pkg (package
> > - (inherit (dummy-package "x"))
> > - (home-page (%local-url)))))
> > - (single-lint-warning-message
> > - (check-home-page pkg))))
> > +(parameterize ((%http-server-port 9999))
> > + ;; TODO skip this test if some process is currently listening at 9999
> > + (test-equal "home-page: Connection refused"
> > + "URI http://localhost:9999/foo/bar unreachable: Connection refused"
> > + (let ((pkg (package
> > + (inherit (dummy-package "x"))
> > + (home-page (%local-url)))))
> > + (single-lint-warning-message
> > + (check-home-page pkg)))))
>
> Likewise.

Explanation #A. Also, explanation #C:

The "(parameterize ((%http-server-port 9999)) ...)"
form is required I think, as otherwise this test will try to connect to port 0,
which doesn't make much sense IMHO. However, a quick test in Guile on Linux
shows:

Toggle quote (1 lines)
> (socket AF_INET SOCK_STREAM 0)
$1 = #<input-output: socket 13>
Toggle quote (1 lines)
> (connect $1 AF_INET INADDR_LOOPBACK 0)
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure connect: Connection refused

It seems like connecting to port 0 results in ‘Connection refused’, but this
connecting to port 0 seems a rather obscure to me, so I would rather not rely
on this, though your opinion could vary. (If we drop the parameterize,
then (%local-url) would need to be replaced with "http://localhost:0".)

-- And why hardcode the port in the first case? Well, there isn't exactly
a procedure for asking the OS to reserve a port, but not bind to it. Perhaps
something to figure out in a future patch ...

Hunk #3.
Toggle quote (10 lines)
> > -(test-equal "home-page: 200 but short length"
> > - "URI http://localhost:9999/foo/bar returned suspiciously small file (18 bytes)"
> > - (with-http-server `((200 "This is too small."))
> > +(with-http-server `((200 "This is too small."))
> > + (test-equal "home-page: 200 but short length"
> > + (format #f "URI ~a returned suspiciously small file (18 bytes)"
> > + (%local-url))
>
> Likewise.

Explanation #A.

Hunk #4.
Toggle quote (9 lines)
> > -(test-equal "home-page: 404"
> > - "URI http://localhost:9999/foo/bar not reachable: 404 (\"Such is life\")"
> > - (with-http-server `((404 ,%long-string))
> > +(with-http-server `((404 ,%long-string))
> > + (test-equal "home-page: 404"
> > + (format #f "URI ~a not reachable: 404 (\"Such is life\")" (%local-url))
>
> Likewise.

Explanation #A.

Hunk #5.
Toggle quote (9 lines)
> > -(test-equal "home-page: 301, invalid"
> > - "invalid permanent redirect from http://localhost:9999/foo/bar"
> > - (with-http-server `((301 ,%long-string))
> > +(with-http-server `((301 ,%long-string))
> > + (test-equal "home-page: 301, invalid"
> > + (format #f "invalid permanent redirect from ~a" (%local-url))
>
> Likewise.

Explanation #A.

Hunk #6.
Toggle quote (19 lines)
> > -(test-equal "home-page: 301 -> 200"
> > - "permanent redirect from http://localhost:10000/foo/bar to http://localhost:9999/foo/bar"
> > - (with-http-server `((200 ,%long-string))
> > - (let* ((initial-url (%local-url))
> > - (redirect (build-response #:code 301
> > - #:headers
> > - `((location
> > - . ,(string->uri initial-url))))))
> > - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> > - (with-http-server `((,redirect ""))
> > +(with-http-server `((200 ,%long-string))
> > + (let* ((initial-url (%local-url))
> > + (redirect (build-response #:code 301
> > + #:headers
> > + `((location
> > + . ,(string->uri initial-url))))))
>
> Likewise.

Explanation #A.

Hunk #7.
Toggle quote (14 lines)
> > -(test-equal "home-page: 301 -> 404"
> > - "URI http://localhost:10000/foo/bar not reachable: 404 (\"Such is life\")"
> > - (with-http-server '((404 "booh!"))
> > - (let* ((initial-url (%local-url))
> > - (redirect (build-response #:code 301
> > - #:headers
> > - `((location
> > - . ,(string->uri initial-url))))))
> > - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> > - (with-http-server `((,redirect ""))
> > +(with-http-server `((404 "booh!"))
>
> Likewise.

Explanation #A.

Hunk #8.
Toggle quote (10 lines)
> > -(test-equal "source: 200 but short length"
> > - "URI http://localhost:9999/foo/bar returned suspiciously small file (18 bytes)"
> > - (with-http-server '((200 "This is too small."))
> > +(with-http-server '((200 "This is too small."))
> > + (test-equal "source: 200 but short length"
> > + (format #f "URI ~a returned suspiciously small file (18 bytes)"
> > + (%local-url))
>
> Likewise.

Explanation #A.

Hunk #9.
Toggle quote (10 lines)
> > -(test-equal "source: 404"
> > - "URI http://localhost:9999/foo/bar not reachable: 404 (\"Such is life\")"
> > - (with-http-server `((404 ,%long-string))
> > +(with-http-server `((404 ,%long-string))
> > + (test-equal "source: 404"
> > + (format #f "URI ~a not reachable: 404 (\"Such is life\")"
> > + (%local-url))
>
> Likewise.

Explanation #A.

Hunk #10.
Toggle quote (14 lines)
> > -(test-equal "source: 301 -> 200"
> > - "permanent redirect from http://localhost:10000/foo/bar to http://localhost:9999/foo/bar"
> > - (with-http-server `((200 ,%long-string))
> > - (let* ((initial-url (%local-url))
> > - (redirect (build-response #:code 301
> > - #:headers
> > - `((location
> > - . ,(string->uri initial-url))))))
> > - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> > - (with-http-server `((,redirect ""))
> > +(with-http-server `((200 ,%long-string))
>
> Likewise.

Explanation #A.

Hunk #11.
Toggle quote (14 lines)
> > -(test-equal "source, git-reference: 301 -> 200"
> > - "permanent redirect from http://localhost:10000/foo/bar to http://localhost:9999/foo/bar"
> > - (with-http-server `((200 ,%long-string))
> > - (let* ((initial-url (%local-url))
> > - (redirect (build-response #:code 301
> > - #:headers
> > - `((location
> > - . ,(string->uri initial-url))))))
> > - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> > - (with-http-server `((,redirect ""))
> > +(with-http-server `((200 ,%long-string))
>
> Likewise.

Explanation #A.

Hunk #12.
Toggle quote (14 lines)
> > -(test-equal "source: 301 -> 404"
> > - "URI http://localhost:10000/foo/bar not reachable: 404 (\"Such is life\")"
> > - (with-http-server '((404 "booh!"))
> > - (let* ((initial-url (%local-url))
> > - (redirect (build-response #:code 301
> > - #:headers
> > - `((location
> > - . ,(string->uri initial-url))))))
> > - (parameterize ((%http-server-port (+ 1 (%http-server-port))))
> > - (with-http-server `((,redirect ""))
> > +(with-http-server '((404 "booh!"))
>
> Likewise.

Explanation #A.

Toggle quote (2 lines)
> Could you send an updated patch?

Is explanation #A clear, and does explanation #B make sense to you?
What do you think of explanation #C?

If you remove these hunks, you should see that the tests will fail
(make check TESTS=$tests).

Thanks,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYD0i9RccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7iLmAP9yGQZ5LEPqBL6NCk+lrL2H+n5N
KQyYcKt6aiNIKZRZYAD8DlK4Lg2o45jK3jhkm+W9iBsKmbWbmRiMU3TvPsIOPwQ=
=6aAB
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 1 Mar 2021 22:40
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 46668@debbugs.gnu.org)
87h7lupn7h.fsf@gnu.org
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (13 lines)
> You made some comments about ‘Hunks that shouldn't be here’ below.
> I disagree. As my explanation is exactly the same for almost all hunks,
> I've numbered them and the explanations.
>
> Explanations:
>
> A. (Hunk 2--12, i.e. all hunks except the first)
> In some tests, the port number is hardcoded.
> E.g., you'll see (test-equal "Some string http://localhost:9999" expression).
> Removing the hard-coding is the whole point of this patch.
> B. See later (hunk #1).
> C. See later (hunk #2).

Oooh I see, my bad! I thought ‘test-equal’ & co. were vanishing, when
in fact they were just moved down. Your explanations make perfect
sense.

IWBN to keep the (test-xyz …) forms at the top level as much as possible
(it’s more convenient, especially when working from Geiser); when it’s
not possible, changes like you did are the right thing.

Thank you, and apologies for the confusion!

Ludo’.
M
M
Maxime Devos wrote on 2 Mar 2021 09:15
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 46668@debbugs.gnu.org)
a9549150450e97d5f02ae789af24a320b5315749.camel@telenet.be
On Mon, 2021-03-01 at 22:40 +0100, Ludovic Courtès wrote:
Toggle quote (5 lines)
> [...]
> Thank you, and apologies for the confusion!
>
> Ludo’.

No problem! The revised patch that uses "let-values" instead of "receive"
is attached.

Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYD30PhccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7ordAQDk2pD+ZIO6Q26l/kQyXsA4NB/y
Vdmz5902hsVs3ZIvOAD8Dm2sCApyvDuXdA5/YyAdtHuNkcoAkYYV+fAa1Y1X0gA=
=V6SC
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 2 Mar 2021 22:29
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 46668@debbugs.gnu.org)
87eegxtfbl.fsf_-_@gnu.org
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (33 lines)
> From 933cb85de0f50c54190e7c60420bef5245a3f2ed Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sat, 20 Feb 2021 22:04:59 +0100
> Subject: [PATCH] tests: do not hard code HTTP ports
>
> Previously, test cases could fail if some process was listening
> at a hard-coded port. This patch eliminates most of these potential
> failures, by automatically assigning an unbound port. This should
> allow for building multiple guix trees in parallel outside a build
> container, though this is currently untested.
>
> The test "home-page: Connection refused" in tests/lint.scm still
> hardcodes port 9999, however.
>
> * guix/tests/http.scm
> (http-server-can-listen?): remove now unused procedure.
> (%http-server-port): default to port 0, meaning the OS
> will automatically choose a port.
> (open-http-server-socket): remove the false statement claiming
> this procedure is exported and also return the allocated port
> number.
> (%local-url): raise an error if the port is obviously unbound.
> (call-with-http-server): set %http-server-port to the allocated
> port while the thunk is called.
> * tests/derivations.scm: adjust test cases to use automatically
> assign a port. As there is no risk of a port conflict now,
> do not make any tests conditional upon 'http-server-can-listen?'
> anymore.
> * tests/elpa.scm: likewise.
> * tests/lint.scm: likewise, and add a TODO comment about a port
> that is still hard-coded.
> * tests/texlive.scm: likewise.

Minor comment but nothing blocking:

Toggle quote (16 lines)
> + (let* ((text (random-text)))
> + (with-http-server `((200 ,text))
> + (let ((drv (derivation %store "world"
> + "builtin:download" '()
> + #:env-vars `(("url"
> + . ,(object->string (%local-url))))
> + #:hash-algo 'sha256
> + #:hash (gcrypt:sha256 (string->utf8 text)))))
> + (and drv (build-derivations %store (list drv))
> + (with-http-server `((200 ,text))
> + (build-derivations %store (list drv)
> + (build-mode check)))
> + (string=? (call-with-input-file (derivation->output-path drv)
> + get-string-all)
> + text))))))

It’s a tad confusing that the second ‘with-http-server’ is now nested;
it shouldn’t change the semantics though, so it’s okay.

Anyway, you’re welcome to push to ‘master’ if “make check” agrees. :-)

Thanks again!

Ludo’.
M
M
Maxime Devos wrote on 2 Mar 2021 22:49
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 46668@debbugs.gnu.org)
9da09b98e911a047e1d296aad4c2be96053bbb44.camel@telenet.be
On Tue, 2021-03-02 at 22:29 +0100, Ludovic Courtès wrote:
Toggle quote (7 lines)
>
> [...]
> It’s a tad confusing that the second ‘with-http-server’ is now nested;
> it shouldn’t change the semantics though, so it’s okay.
>
> Anyway, you’re welcome to push to ‘master’ if “make check” agrees. :-)

I do not have commit access, and I haven't applied for commit access either,
so you will have to push it yourself. "make check" agreed when I wrote the
patch, though perhaps check it yourself just in case.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYD6zAxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7gl/AP47uCRmkwSYsK5dfnjHQntpspuL
XUP/PG4HXSd1NiZwTAEAjBXlnC4WiP05e61hGBZ5rMlphho+fRnmqWZSCAtGUQQ=
=/OM6
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 6 Mar 2021 11:23
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 46668-done@debbugs.gnu.org)
871rcsk2bq.fsf_-_@gnu.org
Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (11 lines)
> On Tue, 2021-03-02 at 22:29 +0100, Ludovic Courtès wrote:
>>
>> [...]
>> It’s a tad confusing that the second ‘with-http-server’ is now nested;
>> it shouldn’t change the semantics though, so it’s okay.
>>
>> Anyway, you’re welcome to push to ‘master’ if “make check” agrees. :-)
>
> I do not have commit access, and I haven't applied for commit access either,
> so you will have to push it yourself.

Oops, my bad; applied now, thanks!

Ludo’.
Closed
?