-
Notifications
You must be signed in to change notification settings - Fork 691
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
Port 'TestFlags' to 'new-test' #5455
Port 'TestFlags' to 'new-test' #5455
Conversation
As it stands, I’m sure this PR is broken in all sorts of small ways. However, I would love if someone with some experience could look over the general approach - I’m not very knowledgeable in cabal, so this whole PR might be fundamentally flawed and it would be great to know that before I start fixing the smaller bugs. This scratches a longstanding itch I’ve had for the Haddock test suite:
|
8bea237
to
1a634ba
Compare
This is ready for review. Assuming everything that passed before I rebased continues to pass, there will be one CI failure. Specifically, EDITLooks like Travis is going to be all green, but now Appveyor is failing where it wasn't before. I'm going to assume this is all due to flakiness in the tests. |
1a634ba
to
0ff2a99
Compare
18004fd
to
d0d3415
Compare
@@ -696,6 +713,9 @@ instance Arbitrary PackageConfig where | |||
instance Arbitrary HaddockTarget where | |||
arbitrary = elements [ForHackage, ForDevelopment] | |||
|
|||
instance Arbitrary TestShowDetails where | |||
arbitrary = elements [Never, Failures, Always, Streaming, Direct] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elements [minBound..maxBound]
in case we add/remove some constructors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you say it, why not arbitraryBoundedEnum :: (Bounded a, Enum a) => Gen a
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Naively threads 'TestFlags' through the 'new-test' argument parsing through to the backend.
This means that test flags can be passed in to 'new-build', 'new-install', 'new-configure', etc. I've mostly just followed the lead of 'HaddockFlags' here.
Paths with escaped characters would, instead of shrinking, grow.
9a42cee
to
b5858c6
Compare
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-7.8.4" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-7.6.3" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-8.2.2" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-8.0.2" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-8.4.4" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-8.6.2" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"linux-8.4.4-fdebug-expensive-assertions" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"osx-7.8.4" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "b5858c66b1ae318e2288a79e531a30a488b89b5c", "tag":"osx-8.0.2" }
cabal-install/changelog
Outdated
@@ -15,6 +15,9 @@ | |||
no longer ignores all arguments except the last one (#5512). | |||
* Add the following option aliases for '-dir'-suffixed options: | |||
'storedir', 'logsdir', 'packagedir', 'sourcedir', 'outputdir' (#5484). | |||
* Ported old-style test options to the new-style commands (#5455). | |||
|
|||
2.4.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a mis-merge here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-7.8.4" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-7.6.3" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-7.10.3" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-8.2.2" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-8.0.2" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-8.4.4" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-8.6.2" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"osx-7.8.4" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"linux-8.4.4-fdebug-expensive-assertions" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"osx-7.10.3" }
"url":"pull/5455", "account":"haskell", "repo":"cabal", "commit": "c71343083987a725f2b393b8f30793eada7325ee", "tag":"osx-8.0.2" }
@quasicomputational Could we merge this soon? I'm getting tired of rebasing and of having to direct people to a workaround for Haddock. I really want to just tell folks to run
instead of explaining why we need
The only CI failure looks network related. |
LGTM. I'd say to go ahead and merge whenever - I can't see any problems and nor has anyone else, evidently. |
Thanks for the review @quasicomputational - I've merged. |
@harpocrates is there a reason for the |
@harendra-kumar Not that I know of. What are you seeing? Does |
@harpocrates I think there is no difference in the two cases, I had the wrong idea about that. However, earlier when I used |
Some caveats: * Cabal version in CI has been bumped to 3.0+ due to need of test flags support: haskell/cabal#5455 * v1-copy was dropped, since it requires v1 build artifacts. There is no v2-copy command AFAICT. * I don't think there is an equivalent for the v1-install command for libraries?
Some caveats: * Cabal version in CI has been bumped to 3.0+ due to need of test flags support: haskell/cabal#5455 * v1-copy was dropped, since it requires v1 build artifacts. There is no v2-copy command AFAICT. * I don't think there is an equivalent for the v1-install command for libraries?
Make all of the old-style
test
flags available tonew-test
/new-build
/new-install
/etc. This should include:--log
(as--test-log
)--machine-log
(as--test-machine-log
)--show-details
(as--test-show-details
)--keep-tix-files
(as--test-keep-tix-files
)--test-options
(as--test-options
)--test-option
(as--test-option
)Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!
Fixes #4803
Fixes #4643
Fixes #4766
Fixes #5416