Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hashable-1.3.1.0 breaks the ordering in testsuites #5878

Closed
juhp opened this issue Feb 20, 2021 · 42 comments
Closed

hashable-1.3.1.0 breaks the ordering in testsuites #5878

juhp opened this issue Feb 20, 2021 · 42 comments

Comments

@juhp
Copy link
Contributor

juhp commented Feb 20, 2021

hashable-1.3.1.0 break the testsuites of various packages that use aeson (in Stackage Nightly).

For now to try to manage I will temporarily put an upper bound on hashable to workaround this.
Please see haskell/aeson#837 for more details and related discussion.

@juhp juhp changed the title hashable-1.3.1.0 hashable-1.3.1.0 reverses ordering of aeson properties Feb 20, 2021
@juhp juhp changed the title hashable-1.3.1.0 reverses ordering of aeson properties hashable-1.3.1.0 breaks the ordering of aeson properties in testsuites Feb 20, 2021
@phadej
Copy link
Contributor

phadej commented Feb 20, 2021

@juhp do you have a list of packages (or have you opened issues in their bug trackers). I guess that without pinging maintainers this won't get resolved by itself any time soon.

@juhp
Copy link
Contributor Author

juhp commented Feb 20, 2021

Unfortunately not yet: the Stackage buildlog is quite long: >180k lines...
I suppose as a first step I could upload the log somewhere to make it visible at least.

@juhp
Copy link
Contributor Author

juhp commented Feb 20, 2021

I uploaded the affected Stackage Nightly log here: https://petersen.fedorapeople.org/nightly-build.log-2021-02-20.gz (~1.1MB) if one wants to inspect it. I'd like to do some text processing on it but not sure if I can find time to do that soon...

@bergmark bergmark changed the title hashable-1.3.1.0 breaks the ordering of aeson properties in testsuites hashable-1.3.1.0 breaks the ordering in testsuites Mar 13, 2021
@bergmark
Copy link
Member

bergmark commented Mar 13, 2021

Here's the full list of test failures, sorry for the delay on this! There may be false positives in here (if so, I apologize for the noise!), all i checked is that a test suite failed. If your package (transitively) uses hashable and the test suite assumes an ordering based on this then please either update your test suite to be ordering-agnostic or bump the hashable lower bound for the test suite. This hashable change may cause actual bugs in your library if it also depends on a specific ordering. At some point we will need bite the bullet and upgrade hashable in stackage and disable any test suites that have not been updated.

This data is from 2021-02-20, so if you have already updated you don't need to do anything else, we'll notice when we upgrade hashable!

See https://petersen.fedorapeople.org/nightly-build.log-2021-02-20.gz if you want to review the actual test failure for your package.

To reproduce, make sure you are building against hashable >= 1.3.1.0

maintainer package test suite
@AndrewRademacher aeson-casing casing failed
@turboMaCk aeson-combinators doctest & spec failed
@newhoggy aeson-lens doctest failed
@brandon-leapyear aeson-schemas aeson-schemas-test failed
@brendanhay amazonka-sqs amazonka-sqs-test failed
@jonathanknowles bech32-th bech32-th-test failed
@centromere cacophony hlint failed
@tomphp cfenv cfenv-test failed
@aiya000 character-cases doctest failed
@chessai chronos doctest failed
@SamProtas composable-associations-aeson doctest & tests failed
@ludat conferer-aeson specs failed
@rblaze credential-store credential-store-test failed
Grandfathered crypto-pubkey test-crypto-pubkey failed
@dfithian datadog datadog-api-test failed
@rblaze dbus dbus_tests failed
@Gabriel439 dhall-lsp-server doctest failed
@k-bx dns network-tests failed
@karun012 doctest-discover doctests failed
@psibi download test failed
@ryota-ka duration doctest failed
@parsonsmatt esqueleto mysql & postgresql failed
@psibi fb runtests failed
@neongreen fmt tests failed
@Gabriel439 foldl doctest failed
@tdammers ginger tests failed
Tom Murphy gingersnap gingersnap-tests failed
Diogo Biazus hasql-notifications hasql-notifications-test failed
@jfischoff hasql-queue unit-tests failed
@nikita-volkov hasql-transaction conflicts-test failed
@vaclavsvejcar headroom doctest failed
@k-bx hedis hedis-test-cluster failed
@lemmih hgeometry doctests failed
@ndmitchell hie-bios bios-tests failed
@alanz hjsmin test-cli failed
@hpdeifel hledger-iadd spec failed
@philderbeast hpack-dhall golden failed
@myfreeweb hspec-expectations-pretty-diff tests failed
@mchaver hspec-golden-aeson test failed
@snoyberg http-client spec failed
@juhp http-directory test failed
@DougBurke hvega test failed
@vaibhavsagar ihaskell hspec failed
@maoe influxdb doctests failed
@tekul jose-jwt tests failed
@ozataman katip test failed
@Hamsterdam kawhi tests failed
@bubba lsp-test tests failed
@chrisdone lucid test failed
@lehins massiv doctests failed
@donatello minio-hs minio-hs-test failed
@mrkkrp mmark tests failed
@mrkkrp mmark-ext tests failed
@VictorDenisov mongoDB test failed
@nalchevanidze morpheus-graphql morpheus-test failed
@nalchevanidze morpheus-graphql-core morpheus-test failed
@paul-rouse mysql test failed
@paul-rouse mysql-simple test failed
Grandfathered network spec failed
@chrisdone odbc test failed
@tomjaguarpaw opaleye test failed
@maksbotan openapi3 doctests failed
@LaurentRDC pandoc-plot tests failed
@jfischoff pg-transact pg-transact-test failed
@jfischoff port-utils unit-test failed
@jfischoff postgresql-libpq-notify test failed
@dylex postgresql-typed hdbc failed
@robx postgrest spec & spec-querycost failed
@parsonsmatt prairie prairie-test failed
@ndmitchell rattle rattle-test failed
@lemmih reanimate spec failed
@lemmih reanimate-svg w3c-spec failed
@wereHamster rethinkdb-client-driver spec failed
@polarina sdl2 sdl-space failed
@maksbotan servant-openapi3 doctests failed
@koterpillar serverless-haskell tests failed
@ndmitchell shake shake-test failed
Grandfathered skein runtests failed
@igrep skews mock-ws-server-test failed
@rickeyski slack-api tests failed
Grandfathered snap-core testsuite failed
@echatav squeal-postgresql doctest & properties & spec failed
@fizruk swagger2 doctests failed
@bartavelle tar-conduit tests failed
@jfischoff tmp-postgres test failed
@chshersh @vrom911 tomland tomland-test failed
@mbg wai-rate-limit-redis wai-rate-limit-redis-tests failed

@bergmark
Copy link
Member

Throttled pings (please see comment above):
@LaurentRDC
@bartavelle
@chshersh @vrom911
@dylex
@echatav
@fizruk
@igrep
@koterpillar
@mbg
@polarina
@rickeyski
@robx
@wereHamster

@bartavelle
Copy link
Contributor

You might want to also ping @lehins for tar-conduit!

@phadej
Copy link
Contributor

phadej commented Mar 13, 2021

lucid is already fixed.

@mbg
Copy link
Contributor

mbg commented Mar 13, 2021

I am a little confused about wai-rate-limit-redis being included in this list - it is listed under expected-test-failures in any case since it requires a Redis server to run, so that might be a false negative? Sorry if I have missed something else.

I have a daily CI run for this too: https://github.com/mbg/wai-rate-limit/actions/workflows/stackage-nightly.yml which hasn't failed.

@tomjaguarpaw
Copy link
Contributor

Similar to opaleye. Its tests require a Postgres server. You can see it running correctly with hashable == 1.3.1.0 at https://github.com/tomjaguarpaw/haskell-opaleye/runs/2102755933?check_suite_focus=true

(There is one transient failure. These are almost unavoidable in Opaleye tests.)

@LaurentRDC
Copy link
Contributor

For pandoc-plot the tests pass with hashable 1.3.1.0 on my machine. Similar to wai-rate-limit-redis, it is listed under expected test failures, which might be due to plotting toolkits not being installed on the host.

@robx
Copy link
Contributor

robx commented Mar 13, 2021

In the logs, I see a number of lines like

mysql: unexpected test build success
mysql-simple: unexpected test build success

I'm guessing those might be the packages that are marked to be skipped? If so, they should probably be removed here.

(The build log does some weird overwriting, the following seems to work to extract the list of packages with that message: gzcat nightly-build.log-2021-02-20.gz | grep unexpected\ test\ build\ success | col -b | awk -F: '{print $1}')

@bergmark
Copy link
Member

Hmm, perhaps this list includes a bunch of packages with expected-test-failures. I thought I filtered these out but I may have done that incorrectly! No need to do anything if you don't think you need to :-)

@mrkkrp
Copy link
Contributor

mrkkrp commented Mar 13, 2021

The test suites of mmark and mmark-ext pass with hashable-1.3.1.0. mmark depends on hashable directly, so I bumped the lower bound accordingly. mmark-ext doesn't, so I didn't do anything. I published mmark-0.0.7.3 and mmark-ext-0.2.1.3 on Hackage.

@hpdeifel
Copy link
Contributor

The test-suite of the newest hledger-iadd release now passes with hashable-1.3.1.0

@vaclavsvejcar
Copy link
Contributor

I fixed the failing doctest inheadroom so it's now passing with hashable-1.3.1.0, thanks for letting me know!

turboMaCk added a commit to turboMaCk/aeson-combinators that referenced this issue Mar 13, 2021
turboMaCk added a commit to turboMaCk/aeson-combinators that referenced this issue Mar 13, 2021
* make tests compatible with hashable 1.3.1.0

see: commercialhaskell/stackage#5878

* update release info
@turboMaCk
Copy link
Contributor

I've just released aeson-combinators 0.0.5.0 - tests should work with both versions there. I did not update doctest but that one is behind cabal flag which is off by default so I think it should not cause problems. Anyway feel free to ping me if it does.

@mihaimaruseac
Copy link
Contributor

new one:

hashable-1.3.0.0 (changelog) (Grandfathered dependencies, Stackage upper bounds) is out of bounds for:

mihaimaruseac added a commit that referenced this issue Mar 14, 2021
@igrep
Copy link
Contributor

igrep commented Mar 14, 2021

My package skews should be added to expected-failures since iijlab/direct-hs#101.

@wereHamster
Copy link
Contributor

I had a look at the log and I don't see any failure in the rethinkdb-client-driver package, only warnings during compilation. Running the tests requires a RethinkDB database and I doubt it stackage has such a setup (please correct me if I'm wrong). Indeed, the rethinkdb-client-driver package is in expected-test-failures.

@mrkkrp
Copy link
Contributor

mrkkrp commented Mar 14, 2021

@mihaimaruseac I addressed the problem exactly in the way that had been suggested: by bumping the lower bound of hashable. Is there something I need to do in addition to that? I think both mmark and mmark-ext can be safely enabled once hashable-1.3.1.0 is on Stackage.

@phadej
Copy link
Contributor

phadej commented Mar 14, 2021

@chshersh and others, did you made sure that your tests are resilient against further changes in hashable hash functions. You shouldn't rely on it being stable hash implementation. (Nor you cannot depend that unordered-containers doesn't change its use of hash functions either - i.e. print someHashMap is just bad).

I'm (not seriously) considering having different parameters on different platforms, so this would be more in your face. (It kind of is, if you test on 32bit systems too).

@vaclavsvejcar
Copy link
Contributor

vaclavsvejcar commented Mar 14, 2021

@chshersh and others, did you made sure that your tests are resilient against further changes in hashable hash functions. You shouldn't rely on it being stable hash implementation. (Nor you cannot depend that unordered-containers doesn't change its use of hash functions either - i.e. print someHashMap is just bad).

I'm (not seriously) considering having different parameters on different platforms, so this would be more in your face. (It kind of is, if you test on 32bit systems too).

I just released headroom-0.4.1.0 to Hackage aiming this issue. In case of Headroom the issue was in doctest, where you really can't do anything else than comparing strings, so I took different approach that should be more futureproof and I'll keep this in mind for future changes.

@chshersh
Copy link
Contributor

@phadej Yes, we made sure. Specifically, in tomland we use sorting in tests.

@robx
Copy link
Contributor

robx commented Mar 14, 2021

For what it's worth, postgrest tests did break, and are fixed in PostgREST/postgrest#1770, so thanks for the heads up!

This shouldn't affect stackage though, since the tests are skipped due to the PostgreSQL dependency.

@phadej
Copy link
Contributor

phadej commented Mar 14, 2021

@chshersh great! (I was confused why you then bumped the lowerbound?)

@mihaimaruseac
Copy link
Contributor

Unfortunately cannot yet remove the upper bound and test if things got fixed since

hashable-1.3.1.0 (changelog) (Grandfathered dependencies) is out of bounds for:

mihaimaruseac added a commit that referenced this issue Mar 14, 2021
@turboMaCk
Copy link
Contributor

@chshersh and others, did you made sure that your tests are resilient against further changes in hashable hash functions. You shouldn't rely on it being stable hash implementation.

yes. I want to support ghcjs in (all versions of) my lib. Luckily if your code is stupid all you need is stone and sticks turboMaCk/aeson-combinators@a0bf729

@k-bx
Copy link
Contributor

k-bx commented Mar 14, 2021

Both dns and hedis work fine, probably some error due to test setup.

@ludat
Copy link
Contributor

ludat commented Mar 14, 2021

I just released conferer-aeson 1.1.0.1 that fixes this issue.

and the library wasn't really broken before, I just had to tighten some semantics that weren't very well defined previously.

Also, thanks for letting everyone know, this would have definitely taken me quite more time without this issue 👏

tekul added a commit to tekul/jose-jwt that referenced this issue Mar 14, 2021
See commercialhaskell/stackage#5878

The problem is that the JWE header is generated internally and since
it is included in the signature, the tests which attempt to reproduce
the JWE values from the spec exactly fail.

Since the header is a simple structure, instead of using Aeson
to encode the encode it we just create a ByteString directly with
the original order (and encode the Alg and Enc values into it).
@tekul
Copy link
Contributor

tekul commented Mar 15, 2021

I released jose-jwt 0.9.1 which should fix this. Thanks for the warning!

@SamProtas
Copy link
Contributor

composable-associations-aeson-0.1.0.1 released on hackage with docs/tests/doctests that no longer rely on HashMap ordering.

@aiya000
Copy link
Contributor

aiya000 commented Mar 18, 2021

@bergmark

Hi :D
Thank you for notifying to me!

I tried reproduce doctest failed 👇

#5878 (comment)

But I cannot reproduce at here - hashable-1.3.1.0...

In above branch, I added hashable-1.3.1.0 into extra-deps of stack.yaml.
I also tried below, but...

$ stack build         # OK
$ stack test :doctest # OK

Please tell me my wrong x(

Thanks!

@bergmark
Copy link
Member

bergmark commented Mar 18, 2021

@aiya000 I don't think this was related to hashable for character-cases, but the tests fail for me locally:

$ stack unpack character-cases
Unpacked character-cases (from Hackage) to /Users/adam.bergmark/repos/3rd/character-cases-0.1.0.6/
$ cd character-cases-0.1.0.6/
$ stack test
$ stack init --resolver nightly
Looking for .cabal or package.yaml files to use to init the project.
Using cabal packages:
- ./

Selected resolver: nightly-2021-03-17
Selected resolver: nightly-2021-03-17
Initialising configuration using resolver: nightly-2021-03-17
Total number of user packages considered: 1
Writing configuration to file: stack.yaml
All done.
adam.bergmark ~/r/3/character-cases-0.1.0.6 /master $ stack test
character-cases> configure (lib + test)
Configuring character-cases-0.1.0.6...
character-cases> build (lib + test)
Preprocessing library for character-cases-0.1.0.6..
Building library for character-cases-0.1.0.6..
[1 of 4] Compiling Cases.Megaparsec
[2 of 4] Compiling Data.Char.Cases
[3 of 4] Compiling Data.String.Cases
[4 of 4] Compiling Paths_character_cases
Preprocessing test suite 'doctest' for character-cases-0.1.0.6..
Building test suite 'doctest' for character-cases-0.1.0.6..
[1 of 5] Compiling Cases.Megaparsec
[2 of 5] Compiling Data.Char.Cases
[3 of 5] Compiling Data.String.Cases
[4 of 5] Compiling Main
[5 of 5] Compiling Paths_character_cases
Linking .stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/doctest/doctest ...
character-cases> copy/register
Installing library in /Users/adam.bergmark/repos/3rd/character-cases-0.1.0.6/.stack-work/install/x86_64-osx/0adf80aebcae3f7ebfe156181adbc51440c2e273742b573987de95632af96cf0/8.10.4/lib/x86_64-osx-ghc-8.10.4/character-cases-0.1.0.6-JjLrB1XY8mY5NJzfl14Sdj
Registering library for character-cases-0.1.0.6..
character-cases> test (suite: doctest)

src/Data/String/Cases.hs:120: failure in expression `[camelQ|camel|]'
expected: "camel"
 but got: Camel (AlphaLower C_) [AlphaNumAlpha (AlphaLower A_),AlphaNumAlpha (AlphaLower M_),AlphaNumAlpha (AlphaLower E_),AlphaNumAlpha (AlphaLower L_)]
          ^

Examples: 37  Tried: 36  Errors: 0  Failures: 1

character-cases> Test suite doctest failed
Completed 2 action(s).
Test suite failure for package character-cases-0.1.0.6
    doctest:  exited with: ExitFailure 1
Logs printed to console

@mrkkrp
Copy link
Contributor

mrkkrp commented Apr 27, 2021

When can we expect the new version of hashable to be included in nightlies?

@vaibhavsagar
Copy link
Contributor

I just ran the IHaskell test suite with hashable-1.3.1.0 and it passed successfully, I think this was a false positive since the test setup for IHaskell is a little involved (i.e. it requires the ihaskell executable to be in the $PATH). You can reproduce this with Nix by running nix-build --check release-9.0.nix -A passthru.haskellPackages.ihaskell from this branch and you can use the ihaskell Cachix cache to download the dependencies if you like.

@DougBurke
Copy link
Contributor

I have finally got around to fixing the tests in hvega - I've just pushed version 0.11.0.1 and am about 80.927% sure it works...

@mrkkrp
Copy link
Contributor

mrkkrp commented Jun 3, 2021

When can we expect the new version of hashable to be included in nightlies?

@bergmark
Copy link
Member

bergmark commented Jun 3, 2021

When can we expect the new version of hashable to be included in nightlies?

When we switch nightlies to GHC 9, hopefully next week

@aiya000
Copy link
Contributor

aiya000 commented Jun 3, 2021

Hi!
Removing my library from stackage is OK :D

Thank you!

@bergmark
Copy link
Member

Closing this due to the GHC 9 upgrade. Packages mentioned here may have been disabled, please see #6072 for more information.

fendor pushed a commit to fendor/aeson-combinators that referenced this issue Mar 19, 2022
* make tests compatible with hashable 1.3.1.0

see: commercialhaskell/stackage#5878

* update release info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests