| Message ID | 20240203230807.25751-6-ngraves@ngraves.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#62202,v4,1/6] import: utils: Add function git->origin. | expand |
Hi, As part of this v4, I would recommend merging patches 2, 3, and 6, such that there’s a single self-contained patch adding ‘guix import juliahub’. (That’s what we usually do and I find it clearer because we immediately see what goes together.) Nicolas Graves <ngraves@ngraves.fr> skribis: > * tests/juliahub.scm : Add unit tests juliahub-redirect, > julia-general-registry-parsing, juliahub-fetch. Just “New file.” Some of the other files lack a commit log; we can add it for you, but it’d be great if you could do it upfront. > --- > tests/juliahub.scm | 185 +++++++++++++++++++++++++++++++++++++++++++++ Please add it to ‘Makefile.am’. [...] > +(define (mock-http-fetch testcase) > + (lambda (url . rest) > + (let ((body (assoc-ref testcase url))) > + (if body > + (open-input-string body) > + (error "mocked http-fetch Unexpected URL: " url))))) > + > +(define (mock-http-get testcase) > + (lambda (url . rest) > + (let ((body (assoc-ref testcase url)) > + (response-header > + (build-response > + #:version '(1 . 1) I strongly encourage using ‘with-http-server’ using the same strategy that’s used in ‘tests/pypi.scm’ and others instead of mocking. (‘mock’ is very sensitive to inlining, plus you sorta have to make assumptions about the code path to be able to mock the right things.) > +(test-equal "juliahub-fetch" > + #t > + (mock ((web client) http-get > + (mock-http-get fixtures-juliahub-check-test)) > + (mock ((guix http-client) http-fetch > + (mock-http-fetch fixtures-juliahub-check-test)) > + (mock ((guix import utils) git->origin mock-git->origin) > + ((@@ (guix import juliahub) juliahub-package?) > + ((@@ (guix import juliahub) juliahub-fetch) "MyPackage")))))) Checking for ‘juliahub-package?’ doesn’t tell us much; what about checking the whole package, similar to what is done in other importer tests? Thanks, Ludo’.
On 2024-04-01 22:50, Ludovic Courtès wrote: > Hi, > > As part of this v4, I would recommend merging patches 2, 3, and 6, such > that there’s a single self-contained patch adding ‘guix import > juliahub’. (That’s what we usually do and I find it clearer because we > immediately see what goes together.) Will do. > > Nicolas Graves <ngraves@ngraves.fr> skribis: > >> * tests/juliahub.scm : Add unit tests juliahub-redirect, >> julia-general-registry-parsing, juliahub-fetch. > > Just “New file.” > > Some of the other files lack a commit log; we can add it for you, but > it’d be great if you could do it upfront. Sorry for these past contribution, I do it now. > >> --- >> tests/juliahub.scm | 185 +++++++++++++++++++++++++++++++++++++++++++++ > > Please add it to ‘Makefile.am’. Will do. > > [...] > >> +(define (mock-http-fetch testcase) >> + (lambda (url . rest) >> + (let ((body (assoc-ref testcase url))) >> + (if body >> + (open-input-string body) >> + (error "mocked http-fetch Unexpected URL: " url))))) >> + >> +(define (mock-http-get testcase) >> + (lambda (url . rest) >> + (let ((body (assoc-ref testcase url)) >> + (response-header >> + (build-response >> + #:version '(1 . 1) > > I strongly encourage using ‘with-http-server’ using the same strategy > that’s used in ‘tests/pypi.scm’ and others instead of mocking. (‘mock’ > is very sensitive to inlining, plus you sorta have to make assumptions > about the code path to be able to mock the right things.) > >> +(test-equal "juliahub-fetch" >> + #t >> + (mock ((web client) http-get >> + (mock-http-get fixtures-juliahub-check-test)) >> + (mock ((guix http-client) http-fetch >> + (mock-http-fetch fixtures-juliahub-check-test)) >> + (mock ((guix import utils) git->origin mock-git->origin) >> + ((@@ (guix import juliahub) juliahub-package?) >> + ((@@ (guix import juliahub) juliahub-fetch) "MyPackage")))))) > > Checking for ‘juliahub-package?’ doesn’t tell us much; what about > checking the whole package, similar to what is done in other importer > tests? Couldn't manage to get it to work, don't remember why exactly but it was related to gexps. Will retry it when moving to 'with-http-server' as advised. > > Thanks, > Ludo’.
On 2024-04-01 22:50, Ludovic Courtès wrote: > Hi, > > As part of this v4, I would recommend merging patches 2, 3, and 6, such > that there’s a single self-contained patch adding ‘guix import > juliahub’. (That’s what we usually do and I find it clearer because we > immediately see what goes together.) > > Nicolas Graves <ngraves@ngraves.fr> skribis: > >> * tests/juliahub.scm : Add unit tests juliahub-redirect, >> julia-general-registry-parsing, juliahub-fetch. > > Just “New file.” > > Some of the other files lack a commit log; we can add it for you, but > it’d be great if you could do it upfront. > >> --- >> tests/juliahub.scm | 185 +++++++++++++++++++++++++++++++++++++++++++++ > > Please add it to ‘Makefile.am’. > > [...] > >> +(define (mock-http-fetch testcase) >> + (lambda (url . rest) >> + (let ((body (assoc-ref testcase url))) >> + (if body >> + (open-input-string body) >> + (error "mocked http-fetch Unexpected URL: " url))))) >> + >> +(define (mock-http-get testcase) >> + (lambda (url . rest) >> + (let ((body (assoc-ref testcase url)) >> + (response-header >> + (build-response >> + #:version '(1 . 1) > > I strongly encourage using ‘with-http-server’ using the same strategy > that’s used in ‘tests/pypi.scm’ and others instead of mocking. (‘mock’ > is very sensitive to inlining, plus you sorta have to make assumptions > about the code path to be able to mock the right things.) I can't however mock a git server, right? I still must mock at least the git repo instead of getting it through a custom server, or is there a better solution here? > >> +(test-equal "juliahub-fetch" >> + #t >> + (mock ((web client) http-get >> + (mock-http-get fixtures-juliahub-check-test)) >> + (mock ((guix http-client) http-fetch >> + (mock-http-fetch fixtures-juliahub-check-test)) >> + (mock ((guix import utils) git->origin mock-git->origin) >> + ((@@ (guix import juliahub) juliahub-package?) >> + ((@@ (guix import juliahub) juliahub-fetch) "MyPackage")))))) > > Checking for ‘juliahub-package?’ doesn’t tell us much; what about > checking the whole package, similar to what is done in other importer > tests? > > Thanks, > Ludo’.
On 2024-04-09 09:29, Nicolas Graves wrote: > On 2024-04-01 22:50, Ludovic Courtès wrote: > >> Hi, >> >> As part of this v4, I would recommend merging patches 2, 3, and 6, such >> that there’s a single self-contained patch adding ‘guix import >> juliahub’. (That’s what we usually do and I find it clearer because we >> immediately see what goes together.) >> >> Nicolas Graves <ngraves@ngraves.fr> skribis: >> >>> * tests/juliahub.scm : Add unit tests juliahub-redirect, >>> julia-general-registry-parsing, juliahub-fetch. >> >> Just “New file.” >> >> Some of the other files lack a commit log; we can add it for you, but >> it’d be great if you could do it upfront. >> >>> --- >>> tests/juliahub.scm | 185 +++++++++++++++++++++++++++++++++++++++++++++ >> >> Please add it to ‘Makefile.am’. >> >> [...] >> >>> +(define (mock-http-fetch testcase) >>> + (lambda (url . rest) >>> + (let ((body (assoc-ref testcase url))) >>> + (if body >>> + (open-input-string body) >>> + (error "mocked http-fetch Unexpected URL: " url))))) >>> + >>> +(define (mock-http-get testcase) >>> + (lambda (url . rest) >>> + (let ((body (assoc-ref testcase url)) >>> + (response-header >>> + (build-response >>> + #:version '(1 . 1) >> >> I strongly encourage using ‘with-http-server’ using the same strategy >> that’s used in ‘tests/pypi.scm’ and others instead of mocking. (‘mock’ >> is very sensitive to inlining, plus you sorta have to make assumptions >> about the code path to be able to mock the right things.) > > I can't however mock a git server, right? I still must mock at least the > git repo instead of getting it through a custom server, or is there a > better solution here? It's actually simpler than I thought, but there's an impediment in guile http server implementation that doesn't allow me to push this effort to the end. https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols I'm currently writing it, it'll result in a handy helper for tests, such as : (with-git-forge ; spawns a dumb but functional git server '(("MyPackage" . ((add "a.txt" "A") (commit "First commit") (tag "v1.0.0" "Release 1.0")))) (with-julia-test-servers `(("/juliahub/MyPackage/" 200 ,juliahub-redirect.html) ("/juliahub/MyPackage/" 200 ,juliahub-redirect.html) ("/juliahub/MyPackage/MySlg/1.0.0/pkg.json" 200 ,(lambda (port) (display (fixture-pkg.json) port))) ("/general/M/MyPackage/Package.toml" 200 ,(lambda (port) (display (pk 'd (general-Package.toml)) port)))) (juliahub->guix-package "MyPackage"))) However, for that I'll need the http server to be able to respond with a (content-type . (application/x-git-upload-pack-advertisement)) header to git. But in guile's web server implementation, this is not possible because of sanitize-response's charset addition, which is not configurable. That's outside my field, how can we progress further ? We do indeed need such a server to properly test juliahub since we go get the tag from the actual repo (this is justified in the patch series).
Hi, Nicolas Graves <ngraves@ngraves.fr> skribis: > I'm currently writing it, it'll result in a handy helper for tests, such > as : > > (with-git-forge ; spawns a dumb but functional git server > '(("MyPackage" . ((add "a.txt" "A") > (commit "First commit") > (tag "v1.0.0" "Release 1.0")))) > (with-julia-test-servers > `(("/juliahub/MyPackage/" 200 ,juliahub-redirect.html) > ("/juliahub/MyPackage/" 200 ,juliahub-redirect.html) > ("/juliahub/MyPackage/MySlg/1.0.0/pkg.json" 200 > ,(lambda (port) (display (fixture-pkg.json) port))) > ("/general/M/MyPackage/Package.toml" 200 > ,(lambda (port) (display (pk 'd (general-Package.toml)) port)))) > (juliahub->guix-package "MyPackage"))) Nice! > However, for that I'll need the http server to be able to respond with a > (content-type . (application/x-git-upload-pack-advertisement)) > header to git. But in guile's web server implementation, this is not > possible because of sanitize-response's charset addition, which is not > configurable. D’oh. I’m not sure what this is about; it’s definitely possible to add pretty much any ‘content-type’ header (we do that in ‘guix publish’ for instance), but if it’s going too far, then maybe you should go back to procedure mocks. > That's outside my field, how can we progress further ? We do indeed need > such a server to properly test juliahub since we go get the tag from the > actual repo (this is justified in the patch series). > > _____________________________________________________________________________ > ;;; Git Forge = Git HTTP Server with Dump transfer protocol and repositories This work’s not lost: we can definitely switch back to it when the limitations you encountered has been fixed on the Guile side, and/or in other, simpler cases. Thanks! Ludo’.
On 2024-04-16 18:52, Ludovic Courtès wrote: > Hi, > > Nicolas Graves <ngraves@ngraves.fr> skribis: > >> I'm currently writing it, it'll result in a handy helper for tests, such >> as : >> >> (with-git-forge ; spawns a dumb but functional git server >> '(("MyPackage" . ((add "a.txt" "A") >> (commit "First commit") >> (tag "v1.0.0" "Release 1.0")))) >> (with-julia-test-servers >> `(("/juliahub/MyPackage/" 200 ,juliahub-redirect.html) >> ("/juliahub/MyPackage/" 200 ,juliahub-redirect.html) >> ("/juliahub/MyPackage/MySlg/1.0.0/pkg.json" 200 >> ,(lambda (port) (display (fixture-pkg.json) port))) >> ("/general/M/MyPackage/Package.toml" 200 >> ,(lambda (port) (display (pk 'd (general-Package.toml)) port)))) >> (juliahub->guix-package "MyPackage"))) > > Nice! > >> However, for that I'll need the http server to be able to respond with a >> (content-type . (application/x-git-upload-pack-advertisement)) >> header to git. But in guile's web server implementation, this is not >> possible because of sanitize-response's charset addition, which is not >> configurable. > > D’oh. I’m not sure what this is about; it’s definitely possible to add > pretty much any ‘content-type’ header (we do that in ‘guix publish’ for > instance), but if it’s going too far, then maybe you should go back to > procedure mocks. Just to write that in a clearer way: libgit2, which is behind guile-git, expects this exact header or fails, with no additional charset field. The sanitize-response function in the guile web server implementation on its side ensures that a charset field is added no matter what. So indeed we can set (content-type . (application/x-git-upload-pack-advertisement)) but in reality guile-git // libgit2 will read (content-type . (application/x-git-upload-pack-advertisement (charset . "utf-8")) and will fail in this case. Since it's exterior (expected by libgit2), I thought the more relevant would be to allow more flexibility on the guile web server side. But if you think it's way better to ask on the libgit2 or guile-git side, I'll be happy to. WDYT ? > >> That's outside my field, how can we progress further ? We do indeed need >> such a server to properly test juliahub since we go get the tag from the >> actual repo (this is justified in the patch series). >> >> _____________________________________________________________________________ >> ;;; Git Forge = Git HTTP Server with Dump transfer protocol and repositories > > This work’s not lost: we can definitely switch back to it when the > limitations you encountered has been fixed on the Guile side, and/or in > other, simpler cases. > > Thanks! > > Ludo’.
Nicolas Graves <ngraves@ngraves.fr> skribis: > libgit2, which is behind guile-git, expects this exact header or fails, > with no additional charset field. The sanitize-response function in the > guile web server implementation on its side ensures that a charset field > is added no matter what. > > So indeed we can set > > (content-type . (application/x-git-upload-pack-advertisement)) > > but in reality guile-git // libgit2 will read > > (content-type . (application/x-git-upload-pack-advertisement > (charset . "utf-8")) > > and will fail in this case. Oh I see, I had misunderstood that. Note that ‘sanitize-response’ does not add a ‘charset’ header when BODY is #f or a bytevector. Maybe we could do that? Thanks, Ludo’.
On 2024-04-17 10:51, Ludovic Courtès wrote: > Nicolas Graves <ngraves@ngraves.fr> skribis: > >> libgit2, which is behind guile-git, expects this exact header or fails, >> with no additional charset field. The sanitize-response function in the >> guile web server implementation on its side ensures that a charset field >> is added no matter what. >> >> So indeed we can set >> >> (content-type . (application/x-git-upload-pack-advertisement)) >> >> but in reality guile-git // libgit2 will read >> >> (content-type . (application/x-git-upload-pack-advertisement >> (charset . "utf-8")) >> >> and will fail in this case. > > Oh I see, I had misunderstood that. > > Note that ‘sanitize-response’ does not add a ‘charset’ header when BODY > is #f or a bytevector. Maybe we could do that? I've just taken a look at your suggestion. That's possible although not elegant. I stumbled accross a bigger issue however: the dumb protocol is implemented for git but not for libgit2, thus is not present in guile-git. https://github.com/libgit2/libgit2/issues/6609 The smart protocol is however way too complex / unadapted for such a light use in tests, and requires more that a simple http server easily configured. That's sad, such a light implementation would've been quite convenient in my juliahub case. > > Thanks, > Ludo’. > > >
diff --git a/tests/juliahub.scm b/tests/juliahub.scm new file mode 100644 index 0000000000..21fdc3a4eb --- /dev/null +++ b/tests/juliahub.scm @@ -0,0 +1,185 @@ +;;; GNU Guix --- Functional package management for GNU +;;; Copyright © 2024 Nicolas Graves <ngraves@ngraves.fr> +;;; +;;; This file is part of GNU Guix. +;;; +;;; GNU Guix is free software; you can redistribute it and/or modify it +;;; under the terms of the GNU General Public License as published by +;;; the Free Software Foundation; either version 3 of the License, or (at +;;; your option) any later version. +;;; +;;; GNU Guix is distributed in the hope that it will be useful, but +;;; WITHOUT ANY WARRANTY; without even the implied warranty of +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;;; GNU General Public License for more details. +;;; +;;; You should have received a copy of the GNU General Public License +;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. + +;;; Summary +;; Tests for guix/import/juliahub.scm + +(define-module (tests-import-juliahub) + #:use-module (guix base32) + #:use-module (json) + #:use-module (guix import juliahub) + #:use-module (guix import utils) + #:use-module ((guix utils) #:select (call-with-temporary-directory)) + #:use-module (guix tests) + #:use-module (ice-9 match) + #:use-module (srfi srfi-19) + #:use-module (srfi srfi-64) + #:use-module (srfi srfi-71) + #:use-module (web response)) + +;; XXX: Copied from tests/go.scm + +(define (mock-http-fetch testcase) + (lambda (url . rest) + (let ((body (assoc-ref testcase url))) + (if body + (open-input-string body) + (error "mocked http-fetch Unexpected URL: " url))))) + +(define (mock-http-get testcase) + (lambda (url . rest) + (let ((body (assoc-ref testcase url)) + (response-header + (build-response + #:version '(1 . 1) + #:code 200 + #:reason-phrase "Ok" + #:headers `( + (content-type text/html (charset . "utf-8")) + (date . ,(make-date 0 10 58 12 6 3 2021 0)) + (transfer-encoding (chunked))) + #:port #f + #:validate-headers? #t))) + (if body + (values response-header body) + (error "mocked http-get Unexpected URL: " url))))) + +;; Mock an empty directory by replacing hash. +(define* (mock-git->origin repo-url ref #:key (ref->commit #f)) + (let* ((version (if (pair? ref) + (cdr ref) + #f)) + (vcommit (match ref->commit + (#t commit) + (#f version) + ((? procedure?) (ref->commit version)) + (_ #f)))) + `(origin + (method git-fetch) + (uri (git-reference + (url ,(and (not (eq? repo-url 'null)) repo-url)) + (commit ,vcommit))) + (file-name (git-file-name name version)) + (sha256 + (base32 + "0sjjj9z1dhilhpc8pq4154czrb79z9cm044jvn75kxcjv6v5l2m5"))))) + +;; Fixtures. + +(define fixture-pkg.json + (scm->json-string + `(("hosted_uri" . "https://git.savannah.gnu.org/cgit/MyPackage.git") + ("installable" . "missing") + ("license" . "GPL-3.0") + ("tags" . #("my-tag")) + ("uuid" . "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa") + ("url" . "https://git.savannah.gnu.org/cgit/MyPackage.git") + ("contributors" . #((("url" . "https://github.com/me") ("type" . "User") ("contributions" . 1) ("name" . "me")))) + ("success" . #t) + ("reversedeps" . #()) + ("pkgeval" + ("report_url" . "https://github.com/JuliaCI/NanosoldierReports/blob/master/pkgeval/by_date/2024-02/01/report.md") + ("version" ("minor" . 18) ("patch" . 1) ("build" . #()) ("prerelease" . #()) ("major" . 1)) + ("reason" . "time_limit") + ("log_url" . "https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_date/2024-02/01/MyPackage.jl.primary.log") + ("status" . "fail") + ("duration" . 0)) + ("stargazers_count" . 0) + ("deps" . #((("slug" . "3FQLY") + ("registry" . "") + ("versions" . #("0.0.0" "1.6.0-1")) + ("uuid" . "de0858da-6303-5e67-8744-51eddeeeb8d7") + ("name" . "Printf") + ("direct" . #t)) + (("slug" . "SIw1t") + ("registry" . "") + ("versions" . #("*" "0.0.0" "1")) + ("uuid" . "cf7118a7-6976-5b1a-9a39-7adc72f591a4") + ("name" . "UUIDs") + ("direct" . #f)) + (("slug" . "THl1k") + ("registry" . "General") + ("versions" . #("0.7" "1" "1-1.10" "1.0-1.5" "1.3.0-1" "1.6.0-1" "1.9.0-1" "1.9.4-1")) + ("uuid" . "1222c4b2-2114-5bfd-aeef-88e4692bbb3e") + ("name" . "julia") + ("direct" . #f)) + (("slug" . "THl1k") + ("registry" . "General") + ("versions" . #("1.6.0-1")) + ("uuid" . "1222c4b2-2114-5bfd-aeef-88e4692bbb3e") + ("name" . "julia") + ("direct" . #t)))) + ("description" . "My description") + ("version" . "1.0.0") + ("documenter_errored" . "missing") + ("slug" . "MySlg") + ("owner" . "me") + ("release_date" . "Feb 2024") + ("name" . "MyPackage.jl") + ("readme" . "My readme") + ("homepage" . "https://git.savannah.gnu.org/cgit/MyPackage.git") + ("doctype" . "hosted") + ("license_url" . "/docs/MyPackage/MySlg/1.0.0/_packagesource/LICENSE.md")))) + +(define fixtures-juliahub-check-test + `(("https://docs.juliahub.com/MyPackage/" . "\ +<head> +<meta http-equiv=\"refresh\" content=\"0; url=MySlg/1.0.0\" /> +</head>") + ("https://raw.githubusercontent.com/JuliaRegistries/General/master\ +/M/MyPackage/Package.toml" . "\ +name = \"MyPackage\" +uuid = \"aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa\" +repo = \"https://git.savannah.gnu.org/cgit/MyPackage.git\" +") + ("https://docs.juliahub.com/MyPackage/MySlg/1.0.0/pkg.json" . ,fixture-pkg.json))) + +(test-begin "juliahub") + +;;; Unit tests for (guix import juliahub) + +(test-equal "juliahub-redirect" + "https://docs.juliahub.com/MyPackage/MySlg/1.0.0/" + (mock ((web client) http-get + (mock-http-get fixtures-juliahub-check-test)) + (mock ((guix http-client) http-fetch + (mock-http-fetch fixtures-juliahub-check-test)) + ((@@ (guix import juliahub) juliahub-url) "MyPackage")))) + +(test-equal "julia-general-registry-parsing" + "https://git.savannah.gnu.org/cgit/MyPackage.git" + (mock ((web client) http-get + (mock-http-get fixtures-juliahub-check-test)) + (mock ((guix http-client) http-fetch + (mock-http-fetch fixtures-juliahub-check-test)) + (assoc-ref + ((@@ (guix import juliahub) ini-fetch) + ((@@ (guix import juliahub) general-url) "MyPackage" "Package.toml")) + 'repo)))) + +(test-equal "juliahub-fetch" + #t + (mock ((web client) http-get + (mock-http-get fixtures-juliahub-check-test)) + (mock ((guix http-client) http-fetch + (mock-http-fetch fixtures-juliahub-check-test)) + (mock ((guix import utils) git->origin mock-git->origin) + ((@@ (guix import juliahub) juliahub-package?) + ((@@ (guix import juliahub) juliahub-fetch) "MyPackage")))))) + +(test-end "juliahub")