[bug#35812] [PATCH] fix hackage cabal tests

[bug#35812] [PATCH] fix hackage cabal tests

[Robert Vollmert]

[-- Attachment #1: Type: text/plain, Size: 156 bytes --]

Hackage cabal tests didn’t run independently due to memoization,
and test-cabal-6 was failing.

Let me know if anything should be done differently.


[-- Attachment #2: 0001-tests-fix-cabal-tests-to-test-and-pass.patch --]
[-- Type: application/octet-stream, Size: 5447 bytes --]

From 8b6dac85a9f6c4e851c1a75a4958dff7915fb2f1 Mon Sep 17 00:00:00 2001
From: Robert Vollmert <rob@vllmrt.net>
Date: Fri, 17 May 2019 10:48:42 +0200
Subject: [PATCH] tests: fix cabal tests to test and pass

* guix/import/hackage.scm: export unmemoized import function
* tests/hackage.scm: use unmemoized import function to make tests independent
* tests/hackage.scm: fix failing test-cabal-6 by providing expected output
---
 guix/import/hackage.scm | 31 +++++++++++++++++--------------
 tests/hackage.scm       | 39 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 17 deletions(-)

diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 2a51420d14..5b80a7ea1d 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -40,6 +40,7 @@
   #:use-module (guix packages)
   #:use-module ((guix utils) #:select (call-with-temporary-output-file))
   #:export (hackage->guix-package
+            hackage->guix-package-impl
             hackage-recursive-import
             %hackage-updater
 
@@ -277,13 +278,12 @@ representation of a Cabal file as produced by 'read-cabal'."
         (license ,(string->license (cabal-package-license cabal))))
      (append hackage-dependencies hackage-native-dependencies))))
 
-(define hackage->guix-package
-  (memoize
-   (lambda* (package-name #:key
-                          (include-test-dependencies? #t)
-                          (port #f)
-                          (cabal-environment '()))
-     "Fetch the Cabal file for PACKAGE-NAME from hackage.haskell.org, or, if the
+(define hackage->guix-package-impl
+  (lambda* (package-name #:key
+                         (include-test-dependencies? #t)
+                         (port #f)
+                         (cabal-environment '()))
+   "Fetch the Cabal file for PACKAGE-NAME from hackage.haskell.org, or, if the
 called with keyword parameter PORT, from PORT.  Return the `package'
 S-expression corresponding to that package, or #f on failure.
 CABAL-ENVIRONMENT is an alist defining the environment in which the Cabal
@@ -293,13 +293,16 @@ symbol 'true' or 'false'.  The value associated with other keys has to conform
 to the Cabal file format definition.  The default value associated with the
 keys \"os\", \"arch\" and \"impl\" is \"linux\", \"x86_64\" and \"ghc\"
 respectively."
-     (let ((cabal-meta (if port
-                           (read-cabal (canonical-newline-port port))
-                           (hackage-fetch package-name))))
-       (and=> cabal-meta (compose (cut hackage-module->sexp <>
-                                       #:include-test-dependencies?
-                                       include-test-dependencies?)
-                                  (cut eval-cabal <> cabal-environment)))))))
+   (let ((cabal-meta (if port
+                         (read-cabal (canonical-newline-port port))
+                         (hackage-fetch package-name))))
+     (and=> cabal-meta (compose (cut hackage-module->sexp <>
+                                     #:include-test-dependencies?
+                                     include-test-dependencies?)
+                                (cut eval-cabal <> cabal-environment))))))
+
+(define hackage->guix-package
+  (memoize hackage->guix-package-impl))
 
 (define* (hackage-recursive-import package-name . args)
   (recursive-import package-name #f
diff --git a/tests/hackage.scm b/tests/hackage.scm
index e17851a213..e56aa996d6 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -161,7 +161,7 @@ library
     (lambda (name-version)
       (call-with-input-string test-cabal
         read-cabal)))
-   (match (hackage->guix-package "foo" #:cabal-environment cabal-environment)
+   (match (hackage->guix-package-impl "foo" #:cabal-environment cabal-environment)
      (('package
         ('name "ghc-foo")
         ('version "1.0.0")
@@ -207,8 +207,41 @@ library
                         #:cabal-environment '(("impl" . "ghc-7.8"))))
 
 (test-assert "hackage->guix-package test 6"
-  (eval-test-with-cabal test-cabal-6
-                        #:cabal-environment '(("impl" . "ghc-7.8"))))
+  (mock
+   ((guix import hackage) hackage-fetch
+    (lambda (name-version)
+      (call-with-input-string test-cabal-6
+        read-cabal)))
+   (match (hackage->guix-package-impl "foo")
+     (('package
+        ('name "ghc-foo")
+        ('version "1.0.0")
+        ('source
+         ('origin
+           ('method 'url-fetch)
+           ('uri ('string-append
+                  "https://hackage.haskell.org/package/foo/foo-"
+                  'version
+                  ".tar.gz"))
+           ('sha256
+            ('base32
+             (? string? hash)))))
+        ('build-system 'haskell-build-system)
+        ('inputs
+         ('quasiquote
+          (("ghc-b" ('unquote 'ghc-b))
+           ("ghc-http" ('unquote 'ghc-http))
+           ("ghc-mtl" ('unquote 'ghc-mtl)))))
+        ('native-inputs
+         ('quasiquote
+          (("ghc-haskell-gi" ('unquote 'ghc-haskell-gi)))))
+        ('home-page "http://test.org")
+        ('synopsis (? string?))
+        ('description (? string?))
+        ('license 'bsd-3))
+      #t)
+     (x
+      (pk 'fail x #f)))))
 
 (test-assert "read-cabal test 1"
   (match (call-with-input-string test-read-cabal-1 read-cabal)
-- 
2.21.0

[bug#35812] [PATCH] fix hackage cabal tests

[Ludovic Courtès]

Hello Robert,

Robert Vollmert <rob@vllmrt.net> skribis:

> Hackage cabal tests didn’t run independently due to memoization,
> and test-cabal-6 was failing.

I don’t think memoization can get in the way here: the argument list is
used as a key in the memoization hash table.  Thus, if you pass
different arguments, you get a cache miss and call the underlying
procedure.

Or am I missing something?

> From 8b6dac85a9f6c4e851c1a75a4958dff7915fb2f1 Mon Sep 17 00:00:00 2001
> From: Robert Vollmert <rob@vllmrt.net>
> Date: Fri, 17 May 2019 10:48:42 +0200
> Subject: [PATCH] tests: fix cabal tests to test and pass
>
> * guix/import/hackage.scm: export unmemoized import function
> * tests/hackage.scm: use unmemoized import function to make tests independent
> * tests/hackage.scm: fix failing test-cabal-6 by providing expected output

[...]

>  (test-assert "hackage->guix-package test 6"
> -  (eval-test-with-cabal test-cabal-6
> -                        #:cabal-environment '(("impl" . "ghc-7.8"))))
> +  (mock
> +   ((guix import hackage) hackage-fetch
> +    (lambda (name-version)
> +      (call-with-input-string test-cabal-6
> +        read-cabal)))
> +   (match (hackage->guix-package-impl "foo")
> +     (('package
> +        ('name "ghc-foo")
> +        ('version "1.0.0")
> +        ('source
> +         ('origin
> +           ('method 'url-fetch)
> +           ('uri ('string-append
> +                  "https://hackage.haskell.org/package/foo/foo-"
> +                  'version
> +                  ".tar.gz"))
> +           ('sha256
> +            ('base32
> +             (? string? hash)))))
> +        ('build-system 'haskell-build-system)
> +        ('inputs
> +         ('quasiquote
> +          (("ghc-b" ('unquote 'ghc-b))
> +           ("ghc-http" ('unquote 'ghc-http))
> +           ("ghc-mtl" ('unquote 'ghc-mtl)))))
> +        ('native-inputs
> +         ('quasiquote
> +          (("ghc-haskell-gi" ('unquote 'ghc-haskell-gi)))))
> +        ('home-page "http://test.org")
> +        ('synopsis (? string?))
> +        ('description (? string?))
> +        ('license 'bsd-3))
> +      #t)
> +     (x
> +      (pk 'fail x #f)))))

So this test needed to be changed as a result of turning off
memoization?

Thanks,
Ludo’.

[bug#35812] [PATCH] fix hackage cabal tests

[Robert Vollmert]

Hi Ludo’,

> On 21. May 2019, at 16:48, Ludovic Courtès <ludo@gnu.org> wrote:
> Robert Vollmert <rob@vllmrt.net> skribis:
> 
>> Hackage cabal tests didn’t run independently due to memoization,
>> and test-cabal-6 was failing.
> 
> I don’t think memoization can get in the way here: the argument list is
> used as a key in the memoization hash table.  Thus, if you pass
> different arguments, you get a cache miss and call the underlying
> procedure.
> 
> Or am I missing something?

I agree that memoization of a pure function shouldn’t have such effects,
but my (limited) understanding is that hackage->guix-packages would
cache import results by package name on the assumption that cabal
files for the same package name don’t change between calls. That
observation is consistent with the test behaviour, but I may well be
missing something.

>> From 8b6dac85a9f6c4e851c1a75a4958dff7915fb2f1 Mon Sep 17 00:00:00 2001
>> From: Robert Vollmert <rob@vllmrt.net>
>> Date: Fri, 17 May 2019 10:48:42 +0200
>> Subject: [PATCH] tests: fix cabal tests to test and pass
>> 
>> * guix/import/hackage.scm: export unmemoized import function
>> * tests/hackage.scm: use unmemoized import function to make tests independent
>> * tests/hackage.scm: fix failing test-cabal-6 by providing expected output
> 
> [...]
> 
>> (test-assert "hackage->guix-package test 6"
>> -  (eval-test-with-cabal test-cabal-6
>> -                        #:cabal-environment '(("impl" . "ghc-7.8"))))
>> +  (mock
>> +   ((guix import hackage) hackage-fetch
>> +    (lambda (name-version)
>> +      (call-with-input-string test-cabal-6
>> +        read-cabal)))
>> +   (match (hackage->guix-package-impl "foo")
>> +     (('package
>> +        ('name "ghc-foo")
>> +        ('version "1.0.0")
>> +        ('source
>> +         ('origin
>> +           ('method 'url-fetch)
>> +           ('uri ('string-append
>> +                  "https://hackage.haskell.org/package/foo/foo-"
>> +                  'version
>> +                  ".tar.gz"))
>> +           ('sha256
>> +            ('base32
>> +             (? string? hash)))))
>> +        ('build-system 'haskell-build-system)
>> +        ('inputs
>> +         ('quasiquote
>> +          (("ghc-b" ('unquote 'ghc-b))
>> +           ("ghc-http" ('unquote 'ghc-http))
>> +           ("ghc-mtl" ('unquote 'ghc-mtl)))))
>> +        ('native-inputs
>> +         ('quasiquote
>> +          (("ghc-haskell-gi" ('unquote 'ghc-haskell-gi)))))
>> +        ('home-page "http://test.org")
>> +        ('synopsis (? string?))
>> +        ('description (? string?))
>> +        ('license 'bsd-3))
>> +      #t)
>> +     (x
>> +      (pk 'fail x #f)))))
> 
> So this test needed to be changed as a result of turning off
> memoization?

Precisely. The other tests all expect various cabal fragments
to yield the same package definition, while the extra fragment

custom-setup
  setup-depends: base >= 4.7 && < 5,
                 Cabal >= 1.24,
                 haskell-gi == 0.21.*

in test-cabal-6 causes the extra native input ghc-haskell-gi.
As I understand it, this test failure used to be shadowed
because hackage->guix-packages would just return the successful
parse of the test-cabal-1.

(There’s probably some more elegant way to avoid duplicating
the whole package definition, but I generally prefer a bit of
explicit verbosity in test data.)

Cheers
Rob

bug#35812: [PATCH] fix hackage cabal tests

[Ludovic Courtès]

Hi,

Robert Vollmert <rob@vllmrt.net> skribis:

>> On 21. May 2019, at 16:48, Ludovic Courtès <ludo@gnu.org> wrote:
>> Robert Vollmert <rob@vllmrt.net> skribis:
>> 
>>> Hackage cabal tests didn’t run independently due to memoization,
>>> and test-cabal-6 was failing.
>> 
>> I don’t think memoization can get in the way here: the argument list is
>> used as a key in the memoization hash table.  Thus, if you pass
>> different arguments, you get a cache miss and call the underlying
>> procedure.
>> 
>> Or am I missing something?
>
> I agree that memoization of a pure function shouldn’t have such effects,
> but my (limited) understanding is that hackage->guix-packages would
> cache import results by package name on the assumption that cabal
> files for the same package name don’t change between calls.

Oh, got it.

I pushed a variant of the patch as commit
ad7466aafd7f166d0b6be5eb32dda1d3ee8a6445.

Thanks!

Ludo’.