Test failure for `prop_aeson_canonical`

[maralorn]

When trying to build hackage-security-0.6.0.1 in nixpkgs we get this error:

  Canonical JSON
    prop_roundtrip_canonical:        OK (0.37s)
      +++ OK, passed 100 tests.
    prop_roundtrip_pretty:           OK (0.47s)
      +++ OK, passed 100 tests.
    prop_canonical_pretty:           OK (0.80s)
      +++ OK, passed 100 tests.
    prop_aeson_canonical:            FAIL
      *** Failed! Falsified (after 5 tests and 1 shrink):
      JSObject [("",JSObject [("\SI",JSString "")])]
      Use --quickcheck-replay=74285 to reproduce.

1 out of 16 tests failed (2.39s)

We recently updated a lot of dependencies, so it is hard to tell, what exactly broke this. I can just say, that all dependencies match the version bounds in the cabal file. This is with aeson 1.5.4.1.

Full build log at https://hydra.nixos.org/build/130472285/nixlog/1

[ezzieyguywuf]

I get the same error trying to build this and run tests on gentoo.

[ezzieyguywuf]

I can confirm that this seems to be an issue with the way in which the tests are executed. The following patch, while admittedly a bit of a hack, shows how some minor tweaks to the test code can resolve the issue:

diff --git a/hackage-security/hackage-security.cabal b/hackage-security/hackage-security.cabal
index 1835e09..381ac43 100644
--- a/hackage-security/hackage-security.cabal
+++ b/hackage-security/hackage-security.cabal
@@ -261,8 +261,8 @@ test-suite TestSuite
   build-depends:       tasty            == 1.2.*,
                        tasty-hunit      == 0.10.*,
                        tasty-quickcheck == 0.10.*,
-                       QuickCheck       >= 2.11 && <2.14,
-                       aeson            == 1.4.*,
+                       QuickCheck       >= 2.11,
+                       aeson            >= 1.4,
                        vector           == 0.12.*,
                        unordered-containers >=0.2.8.0 && <0.3,
                        temporary        >= 1.2 && < 1.4
diff --git a/hackage-security/tests/TestSuite/JSON.hs b/hackage-security/tests/TestSuite/JSON.hs
index 5ea2c7f..39e93e2 100644
--- a/hackage-security/tests/TestSuite/JSON.hs
+++ b/hackage-security/tests/TestSuite/JSON.hs
@@ -23,6 +23,9 @@ import Data.String (fromString)
 import qualified Data.Vector as V
 import qualified Data.HashMap.Strict as HM
 
+-- text
+import qualified Data.Text as Text
+
 prop_aeson_canonical, prop_roundtrip_canonical, prop_roundtrip_pretty, prop_canonical_pretty
   :: JSValue -> Bool
 
@@ -48,6 +51,9 @@ canonicalise   (JSArray  vs) = JSArray  [ canonicalise v | v <- vs]
 canonicalise   (JSObject vs) = JSObject [ (k, canonicalise v)
                                         | (k,v) <- sortBy (compare `on` fst) vs ]
 
+sanitizeString :: String -> String
+sanitizeString s = Text.unpack (Text.replace (Text.pack "\\") (Text.pack "\\\\") (Text.pack (show s)))
+
 instance Arbitrary JSValue where
   arbitrary =
     sized $ \sz ->
@@ -55,9 +61,9 @@ instance Arbitrary JSValue where
       [ (1, pure JSNull)
       , (1, JSBool   <$> arbitrary)
       , (2, JSNum    <$> arbitrary)
-      , (2, JSString . getASCIIString <$> arbitrary)
+      , (2, JSString . sanitizeString . getASCIIString <$> arbitrary)
       , (3, JSArray                <$> resize (sz `div` 2) arbitrary)
-      , (3, JSObject . mapFirst getASCIIString .  noDupFields <$> resize (sz `div` 2) arbitrary)
+      , (3, JSObject . mapFirst (sanitizeString . getASCIIString) .  noDupFields <$> resize (sz `div` 2) arbitrary)
       ]
     where
       noDupFields = nubBy (\(x,_) (y,_) -> x==y)

[Mikolaj]

Hi! Thank you very much for the report and for the patch. I hope this is fixed now.

I'm planning to release a new version of hackage-security RSN. Could you check the current codebase works for you and nothing extremely urgent is missing? Thank you!

[Bodigrim]

It would be nice to support aeson-2.0 in tests:

+#if MIN_VERSION_aeson(2,0,0)
+import qualified Data.Aeson.KeyMap as KM
+#else
 import qualified Data.HashMap.Strict as HM
+#endif

+#if MIN_VERSION_aeson(2,0,0)
+toAeson (JSObject xs) = Object $ KM.fromList [ (fromString k, toAeson v) | (k, v) <- xs ]
+#else
 toAeson (JSObject xs) = Object $ HM.fromList [ (fromString k, toAeson v) | (k, v) <- xs ]
+#endif

[Mikolaj]

Sure, I'd merge it. Would you create a PR?

[Mikolaj]

@Bodigrim: thank you for the #258 to support aeson-2.0 in tests. Merged.