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

Attempt at fixing T3436 regression test. #5288

Merged
merged 1 commit into from
May 3, 2018
Merged

Conversation

merijn
Copy link
Collaborator

@merijn merijn commented Apr 26, 2018

The T3436 regression test is responsible for literally every PR being marked as failing, because it fails on GHC 8.4. I don't really understand the test, so someone has to check whether this defeats it's entire purpose (@grayjay ?).

The main problem is that GHC 8.4 isn't officially supported by older Cabal versions, so the test's use of "Cabal >= 2.0" leads it to pick the Cabal 2.2 that comes with GHC and everything breaks. The new version simply ups the Cabal version to 2.4 so that the test will properly compile with GHC 8.4 and hopefully fix everyone's failing tests. Rejoice!

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

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

@23Skidoo
Copy link
Member

Can we set the version to e.g. 3000, so that it wouldn't break with the next GHC release?

@23Skidoo
Copy link
Member

23Skidoo commented Apr 26, 2018

...so it turns out that's how the test was implemented initially, but then had to be changed because of #4909. Perhaps we should add a switch for turning that off and run the test suite with it enabled.

@grayjay
Copy link
Collaborator

grayjay commented Apr 28, 2018

@merijn Is PackageTests/Regression/T3436/cabal.test.hs the only failing test? I think that the main regression test for #3436 is the one that uses cabal sandbox (cabal-testsuite/PackageTests/Regression/T3436/sandbox.test.hs), because the test needs to install a package into the user or global package db, and that is easier to do with a test sandbox. cabal.test.hs is a new-build version of the regression test, but there is a comment saying that it doesn't work:

-- NB: This test doesn't really test #3436, because Cabal-1.2
-- isn't in the system database and thus we can't see if the
-- depsolver incorrectly chooses it. Worth fixing if we figure
-- out how to simulate the "global" database without root.
r <- fails $ cabal' "new-build" ["custom-setup"]

It might be better to just delete cabal.test.hs, and we can migrate it again if necessary when we remove cabal sandbox. It's possible that the regression test won't be relevant then, because new-build can completely ignore the installed packages when choosing the setup Cabal version, once it doesn't need to interact with the old build code path.

@merijn
Copy link
Collaborator Author

merijn commented Apr 30, 2018

I think it's only cabal.test.hs that's failing, yeah. I'm fine with just nuking that one if everyone's okay with it.

@merijn
Copy link
Collaborator Author

merijn commented Apr 30, 2018

Aside from the AppVeyor build (which seems to fail in a completely unrelated test?) this seems to turn everything green. Any objections to just nuking the new-build version of T3436, considering it wasn't really doing anything anyway?

@merijn
Copy link
Collaborator Author

merijn commented Apr 30, 2018

Actually, looks like it's just AppVeyor being difficult, on my branch it's marked as succeeding, so should be fine to merge if people agree with ditching the new-build test.

@merijn
Copy link
Collaborator Author

merijn commented Apr 30, 2018

Ping @ezyang

I was told you knew the test-suite best, any comments?

@merijn
Copy link
Collaborator Author

merijn commented May 2, 2018

Any objections? Else I'm merging this.

@ezyang ezyang merged commit e2ea6a4 into haskell:master May 3, 2018
@merijn merijn deleted the test branch May 3, 2018 08:14
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