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

Turn off DeterministicTrivial test (see #8356) #8357

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

ffaf1
Copy link
Collaborator

@ffaf1 ffaf1 commented Aug 10, 2022

See #8356


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • no relevant info to users
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

@ffaf1 ffaf1 force-pushed the skip-test branch 2 times, most recently from 49e19de to ceef1ba Compare August 10, 2022 14:14
@ulysses4ever
Copy link
Collaborator

I assume we won't be waiting for two days here?

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Aug 10, 2022

Ambiguous module name ‘Crypto.Hash.SHA256’:
      it was found in multiple packages:
      cryptohash-sha256-0.11.102.1 cryptohash-sha256-0.11.102.1

??? I keep looking at the two cryptohash, they are the same!

@Mikolaj
Copy link
Member

Mikolaj commented Aug 10, 2022

??? I keep looking at the two cryptohash, they are the same!

That's the very test we were trying to disable. Apparently it fails before our disabling kicks in. Perhaps the skipIf needs to be moved to the start of the test.

@ffaf1
Copy link
Collaborator Author

ffaf1 commented Aug 10, 2022

Putting skip... on top still gives me that weird import error.

With this, it works, but it is ugly (had to comment BS16 import) and leaves all the questions on why it fails open.

import Test.Cabal.Prelude
import qualified Data.ByteString as BS
-- import qualified Data.ByteString.Base16 as BS16
import qualified Crypto.Hash.SHA256 as SHA256
import System.FilePath
    ( (</>) )

main = cabalTest $ do

    cabal "v2-sdist" ["deterministic"]
    env <- getTestEnv
    let dir = testCurrentDir env
        knownSdist = dir </> "deterministic-0.tar.gz"
        mySdist = dir </> "dist-newstyle" </> "sdist" </> "deterministic-0.tar.gz"

    -- This helps to understand why this test fails, if it does:
    --
    -- shell "tar" ["-tzvf", knownSdist]
    -- shell "tar" ["-tzvf", mySdist]
    --

    known <- liftIO (BS.readFile knownSdist)
    unknown <- liftIO (BS.readFile mySdist)

    -- Not using `expectBroken` becuase the test succedes locally but
    -- fails in CI.
    skipIf "#8356" True
    -- assertEqual "hashes didn't match for sdist" (BS16.encode $ SHA256.hash known) (BS16.encode $ SHA256.hash unknown)
    assertEqual "hashes didn't match for sdist" True True

@Mikolaj
Copy link
Member

Mikolaj commented Aug 10, 2022

Oh, wow, so it fails in the import? No wonder we can't put skipIf far enough up. Let's comment out the import in that case!

I wonder what uncommenting This helps to understand why this test fails, if it does would show, but that's for another time.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 10, 2022

And actually, it would probably not show anything useful, because the assertion probably doesn't fail, it's the import that fails.

@ffaf1 ffaf1 force-pushed the skip-test branch 2 times, most recently from 7bc6703 to ca8bdbc Compare August 10, 2022 18:35
@ffaf1
Copy link
Collaborator Author

ffaf1 commented Aug 10, 2022

The last iteration:

  • works;
  • clearly states the test is broken in unfathomable ways;
  • marks it as skipped lest we forget about it.

@ffaf1 ffaf1 force-pushed the skip-test branch 2 times, most recently from a0f855f to 22db7a9 Compare August 10, 2022 18:42
@ffaf1
Copy link
Collaborator Author

ffaf1 commented Aug 10, 2022

Notice how on different platforms, we get different imports error (on a linux virtual machine, BS16; on Windows CI, SHA256).

What a weird behaviour.

@ffaf1 ffaf1 marked this pull request as ready for review August 11, 2022 07:12
@Mikolaj
Copy link
Member

Mikolaj commented Aug 11, 2022

Emergency-merging to unblock CI for our devs.

@Mikolaj Mikolaj merged commit 131d672 into haskell:master Aug 11, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Aug 12, 2022

Unfortunately, this needs to be backported, because it blocks CI on branch 3.8 as well.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 12, 2022

@mergify backport 3.8

@mergify
Copy link
Contributor

mergify bot commented Aug 12, 2022

backport 3.8

✅ Backports have been created

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

Successfully merging this pull request may close these issues.

4 participants