From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id UDDBGDwjPWCYQAAA0tVLHw (envelope-from ) for ; Mon, 01 Mar 2021 17:24:12 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id oIyCFDwjPWDLJQAAbx9fmQ (envelope-from ) for ; Mon, 01 Mar 2021 17:24:12 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 814CB8D1E for ; Mon, 1 Mar 2021 18:24:11 +0100 (CET) Received: from localhost ([::1]:41514 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lGmHC-0005Ug-2n for larch@yhetil.org; Mon, 01 Mar 2021 12:24:10 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:38188) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lGmH4-0005TH-W5 for guix-patches@gnu.org; Mon, 01 Mar 2021 12:24:03 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:39148) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lGmH4-0001dk-8t for guix-patches@gnu.org; Mon, 01 Mar 2021 12:24:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lGmH4-0007r6-44 for guix-patches@gnu.org; Mon, 01 Mar 2021 12:24:02 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#46668] [PATCH]: tests: do not hard code HTTP ports Resent-From: Maxime Devos Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 01 Mar 2021 17:24:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 46668 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 46668@debbugs.gnu.org Received: via spool by 46668-submit@debbugs.gnu.org id=B46668.161461939230140 (code B ref 46668); Mon, 01 Mar 2021 17:24:02 +0000 Received: (at 46668) by debbugs.gnu.org; 1 Mar 2021 17:23:12 +0000 Received: from localhost ([127.0.0.1]:50694 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lGmGF-0007q3-BL for submit@debbugs.gnu.org; Mon, 01 Mar 2021 12:23:11 -0500 Received: from laurent.telenet-ops.be ([195.130.137.89]:49478) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lGmGC-0007pt-U6 for 46668@debbugs.gnu.org; Mon, 01 Mar 2021 12:23:10 -0500 Received: from ptr-bvsjgyjmffd7q9timvx.18120a2.ip6.access.telenet.be ([IPv6:2a02:1811:8c09:9d00:aaf1:9810:a0b8:a55d]) by laurent.telenet-ops.be with bizsmtp id b5P62400K0mfAB4015P6vY; Mon, 01 Mar 2021 18:23:07 +0100 Message-ID: <3ae8be5f05d73106c867fe2cfc7f619673fd738b.camel@telenet.be> From: Maxime Devos Date: Mon, 01 Mar 2021 18:23:00 +0100 In-Reply-To: <87r1kyri5l.fsf@gnu.org> References: <1728b9290c1b0e248b71c8b35623939853895a7f.camel@telenet.be> <87r1kyri5l.fsf@gnu.org> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-1pXjYIk3M2qxf3O0S+Mi" User-Agent: Evolution 3.34.2 MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telenet.be; s=r21; t=1614619387; bh=uhDwVhtwtA5c7h11YIQR5wSzo4VjPOFxIqWupSeYZ4Y=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=hNraRhKBCluaMPNFLm12N7JoPMh1olnW9PTBUXcp2939BQsK3DLLa012/7m8/m6BS MZeYRUDqYsgOrImxzQqrJ6OaRyXUAH9qLpUqLMNG2KRzC0Mf8lUK4tF3yTLp9G7fbW IPrNfuLYZpMQE0vGC63eCzmjGx4FKT7yoLegV2P8HwKahP2+3xX2ZuMtIfJQX2h5ZN O0otZdjPJEvmGDUa6COuGKgpLkgfeH3+HVHGbaSfXdLB85nkpw+vDmVvRZgLUAitmX pG5hKcBe2m2iW4RoHWzM+Tqx/uqCytvCx6BvznRcxTRS/DY8V/JuzxBTjT60X9rxER HJgl+fx4n+Ujg== X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1614619452; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post: dkim-signature; bh=uhDwVhtwtA5c7h11YIQR5wSzo4VjPOFxIqWupSeYZ4Y=; b=X33ui/i8Rv076fy4IxeUyAqcvmcUEcEIQ6cVgIFBVqmJ6EOLlUg1aDnWqao6aD1mL+IAPS oebirH0XK3g0ToW/Xyl5LCTVh3PIsLbDL21x8HZn6R3lY9IEbrIUTCjs+VK5/gXtp3yAGu 8Gk0n8x7rY4L0YX6BZYHrtT07uW3DmDeqwjMDIel0ZOEw6NLa4QViJK9XQqvG4fWAMvEGO BE9QPj3toR+1T3D/2tsdVsi0J64KKpL/EYP1SdDhMq+TJ/zkjFLK9plUrG5fUh1CLsNipq 6CIGBEUnDRkRqxbFoOsWgh4qwQHtDE4zKT4wRppLOOfYl7LvaaPMwa854JYSUw== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1614619452; a=rsa-sha256; cv=none; b=NYdj31PYMSx8qlBF9BD38zy9wHJtcH9YRJDqYPDcK1Nv+AOQRqDfEZyZ4DSdeDuKsOjm/k woiQP0VPJmCYgYLfpAGSt/CE+Xmx++Pt6WqPmOyIoWFyDx982bLdBq38d/2Yx2Ey7n+tfX WIlimBq7tl45MFa7EbxWcCVlUq0wr9w3jf8TgPxoIF2+oulbnkdMhnRLMck7RJPziCwobu 9YbB4yQwK3FcUlbn5L6HxjoSvRMzMW09p70tDng8YnPGWn6yBT0S1bGeQg99DTP1vreN2/ 8MRJeygqXeNPLLAE+Uz2zwJ3asvYDlHOhOU3l+S0Lc8hTj3rUObLD/qALid2oQ== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=telenet.be header.s=r21 header.b=hNraRhKB; dmarc=fail reason="SPF not aligned (relaxed)" header.from=telenet.be (policy=none); spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Spam-Score: -3.37 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=telenet.be header.s=r21 header.b=hNraRhKB; dmarc=fail reason="SPF not aligned (relaxed)" header.from=telenet.be (policy=none); spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Migadu-Queue-Id: 814CB8D1E X-Spam-Score: -3.37 X-Migadu-Scanner: scn0.migadu.com X-TUID: ytYHEemgT0F4 --=-1pXjYIk3M2qxf3O0S+Mi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 2021-03-01 at 16:46 +0100, Ludovic Court=C3=A8s wrote: > [...] > Maxime Devos skribis:=20 > > [...] >=20 > Nice! >=20 > Some comments below. >=20 > > + #:use-module (ice-9 receive) > Please use (srfi srfi-71) instead, or (srfi srfi-11). Sure, will do. You made some comments about =E2=80=98Hunks that shouldn't be here=E2=80=99= 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" expres= sion). Removing the hard-coding is the whole point of this patch. B. See later (hunk #1). C. See later (hunk #2). Hunk #1. > > -(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 m= ode > > ;; works. See ;. > > - (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=3D? (call-with-input-file (derivation->output-path dr= v) > > - 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=3D? (call-with-input-file (derivation->output-pat= h drv) > > + get-string-all) > > + text)))))) >=20 > This hunk shouldn=E2=80=99t 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. > > -(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 999= 9 > > + (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))))) >=20 > 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 por= t 0, which doesn't make much sense IMHO. However, a quick test in Guile on Linu= x shows: > (socket AF_INET SOCK_STREAM 0) $1 =3D # > (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 =E2=80=98Connection refused= =E2=80=99, but this connecting to port 0 seems a rather obscure to me, so I would rather not re= ly 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. Perha= ps something to figure out in a future patch ... Hunk #3. > > -(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)) >=20 > Likewise. Explanation #A. Hunk #4. > > -(test-equal "home-page: 404" > > - "URI http://localhost:9999/foo/bar not reachable: 404 (\"Such is lif= e\")" > > - (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)) >=20 > Likewise. Explanation #A. Hunk #5. > > -(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)) >=20 > Likewise. Explanation #A. Hunk #6. > > -(test-equal "home-page: 301 -> 200" > > - "permanent redirect from http://localhost:10000/foo/bar to http://lo= calhost: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)))))= ) >=20 > Likewise. Explanation #A. Hunk #7. > > -(test-equal "home-page: 301 -> 404" > > - "URI http://localhost:10000/foo/bar not reachable: 404 (\"Such is li= fe\")" > > - (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!")) >=20 > Likewise. Explanation #A. Hunk #8. > > -(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)) >=20 > Likewise. Explanation #A. Hunk #9. > > -(test-equal "source: 404" > > - "URI http://localhost:9999/foo/bar not reachable: 404 (\"Such is lif= e\")" > > - (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)) >=20 > Likewise. Explanation #A. Hunk #10. > > -(test-equal "source: 301 -> 200" > > - "permanent redirect from http://localhost:10000/foo/bar to http://lo= calhost: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)) >=20 > Likewise. Explanation #A. Hunk #11. > > -(test-equal "source, git-reference: 301 -> 200" > > - "permanent redirect from http://localhost:10000/foo/bar to http://lo= calhost: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)) >=20 > Likewise. Explanation #A. Hunk #12. > > -(test-equal "source: 301 -> 404" > > - "URI http://localhost:10000/foo/bar not reachable: 404 (\"Such is li= fe\")" > > - (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!")) >=20 > Likewise. Explanation #A. > 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=3D$tests). Thanks, Maxime. --=-1pXjYIk3M2qxf3O0S+Mi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYD0i9RccbWF4aW1lZGV2 b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7iLmAP9yGQZ5LEPqBL6NCk+lrL2H+n5N KQyYcKt6aiNIKZRZYAD8DlK4Lg2o45jK3jhkm+W9iBsKmbWbmRiMU3TvPsIOPwQ= =6aAB -----END PGP SIGNATURE----- --=-1pXjYIk3M2qxf3O0S+Mi--