testsuite: Add some unit tests for #9466 #9467 #9468

mpickering · 2023-11-24T12:38:54Z

These commits add some tests I wrote when thinking about #9466 and #9467.

aleeusgr · 2023-12-18T05:35:21Z

@Mikolaj Looking into this PR.

cabal-install/cabal-install.cabal

aleeusgr

Does it make sense to open an Issue to refactor some of the tests to have more descriptive names?

I'm puzzled that people are so passionate about test names. I consider them the least important part of a test. A name isn't irrelevant, but I find the test code more important. The code is an executable specification.

https://blog.ploeh.dk/2022/06/13/some-thoughts-on-naming-tests/

cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs

grayjay

@mpickering Thanks for writing these unit tests! I reviewed them based on our discussion in #9466 and #9467.

grayjay · 2024-01-04T18:27:41Z

cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs

+-- With the user constraint syntax (fails)
+setupStanzaTest1 :: SolverTest
+setupStanzaTest1 = userConstraints ["B test"] $ mkTest dbSetupStanza "setupStanzaTest1" ["A"] (solverSuccess [("A", 1), ("B", 1)])


Why does this test specify the constraint as a string? I would expect it to be equivalent to ExStanzaConstraint (scopeToplevel "B") [TestStanzas] in the test below.

The point of this test is testing the user constraint syntax and how UserConstraintSources influence the solver (7e44f76), and is otherwise equal to setupStanaTest2 below.

So I'll remove this test as well

cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs

grayjay · 2024-01-04T21:44:41Z

cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs

+      parseConstraint :: String -> LabeledPackageConstraint
+      parseConstraint str =
+        case explicitEitherParsec parsec str of
+          Left err -> error ("Bad user constraint: " ++ err)
+          Right user_constraint -> toLpc (userToPackageConstraint user_constraint)


I think that we should add all types of constraints to the solver DSL rather than parsing strings.

Using strings for this particular case seems like an added benefit to me: the ability to easily add user constraints to the solver testsuite in the way that they are typically written, which doubles as a validation of the syntax parser. (unlike e.g. specifying (installed) packages and their dependencies for which the DSL is amazing to write concise tests, I don't think we'd win much by using a different syntax for user constraints which are already their own "canonical" concise way of being specified)

I don't think that we should test UserConstraints or parsing in this test suite, since that functionality is part of the cabal-install package, and these unit tests are for the dependency solver. These tests should eventually be moved to the cabal-install-solver package as a followup to #7358. I also think that it is confusing to have two ways to specify the same constraints.

I see how that is reasonable. I'm going to drop the UserConstraint syntax testsuite support and change the tests to use PackageConstraints directly.

alt-romes · 2024-03-11T15:29:24Z

@grayjay I have addressed your review, could you check again? I think this is ready to merge.

alt-romes · 2024-03-14T09:05:25Z

Very good, I've checked that our follow-up PR implementing private dependencies does fix this test that is marked expected broken: https://github.com/haskell/cabal/actions/runs/8270104380/job/22626918468?pr=9743

@grayjay a light ping

cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs

grayjay · 2024-03-19T07:17:06Z

cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs

+      parseConstraint :: String -> LabeledPackageConstraint
+      parseConstraint str =
+        case explicitEitherParsec parsec str of
+          Left err -> error ("Bad user constraint: " ++ err)
+          Right user_constraint -> toLpc (userToPackageConstraint user_constraint)


I don't think that we should test UserConstraints or parsing in this test suite, since that functionality is part of the cabal-install package, and these unit tests are for the dependency solver. These tests should eventually be moved to the cabal-install-solver package as a followup to #7358. I also think that it is confusing to have two ways to specify the same constraints.

alt-romes · 2024-04-04T09:53:29Z

@grayjay ready to go. I've addressed your review fully.

grayjay

Thanks!

…eraction) This adds two tests for issue haskell#9467

These tests check how constraints interact with the --independent-goals flag.

…anza flags These tests check how the setup qualified scope interacts with the stanza flags (specified on the top-level and with the any qualifier)

Kleidukos · 2024-05-16T14:20:17Z

@Mergifyio backport 3.12

mergify · 2024-05-16T14:20:42Z

backport 3.12

✅ Backports have been created

#10020 testsuite: Add some unit tests for #9466 #9467 (backport #9468) has been created for branch 3.12

* testsuite: Add tests for #9467 (base shim, setup qualifier interaction) This adds two tests for issue #9467 (cherry picked from commit 3851043) * testsuite: Add two tests for independent goals (#9466) These tests check how constraints interact with the --independent-goals flag. (cherry picked from commit b169cd4) * testsuite: Add some tests for setup component scope interacts with stanza flags These tests check how the setup qualified scope interacts with the stanza flags (specified on the top-level and with the any qualifier) (cherry picked from commit 573c15d) --------- Co-authored-by: Matthew Pickering <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

mpickering force-pushed the wip/unit-tests branch 4 times, most recently from 70bf844 to 935383c Compare November 24, 2023 15:54

grayjay added the cabal-install: solver label Nov 24, 2023

Mikolaj added the attention: needs-review label Nov 25, 2023

aleeusgr reviewed Dec 18, 2023

View reviewed changes

cabal-install/cabal-install.cabal Outdated Show resolved Hide resolved

aleeusgr reviewed Dec 18, 2023

View reviewed changes

cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs Outdated Show resolved Hide resolved

Mikolaj requested review from Kleidukos and alt-romes December 21, 2023 18:52

grayjay self-requested a review January 4, 2024 08:58

grayjay reviewed Jan 5, 2024

View reviewed changes

grayjay removed the attention: needs-review label Jan 19, 2024

alt-romes self-assigned this Mar 5, 2024

alt-romes force-pushed the wip/unit-tests branch 2 times, most recently from 8ee7347 to 7e503eb Compare March 11, 2024 14:46

alt-romes added the attention: needs-review label Mar 11, 2024

geekosaur approved these changes Mar 14, 2024

View reviewed changes

grayjay reviewed Mar 19, 2024

View reviewed changes

alt-romes force-pushed the wip/unit-tests branch 3 times, most recently from baf4564 to f3f1e11 Compare April 3, 2024 10:15

grayjay approved these changes Apr 5, 2024

View reviewed changes

alt-romes added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Apr 5, 2024

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Apr 7, 2024

mpickering added 3 commits April 7, 2024 20:12

testsuite: Add tests for haskell#9467 (base shim, setup qualifier int…

3851043

…eraction) This adds two tests for issue haskell#9467

testsuite: Add two tests for independent goals (haskell#9466)

b169cd4

These tests check how constraints interact with the --independent-goals flag.

testsuite: Add some tests for setup component scope interacts with st…

573c15d

…anza flags These tests check how the setup qualified scope interacts with the stanza flags (specified on the top-level and with the any qualifier)

Mikolaj force-pushed the wip/unit-tests branch from f3f1e11 to 573c15d Compare April 7, 2024 20:12

mergify bot merged commit cd9b1fd into haskell:master Apr 7, 2024
55 checks passed

Kleidukos added the attention: needs-backport 3.12 label May 16, 2024

mergify bot mentioned this pull request May 16, 2024

testsuite: Add some unit tests for #9466 #9467 (backport #9468) #10020

Merged

geekosaur removed the attention: needs-backport 3.12 label Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testsuite: Add some unit tests for #9466 #9467 #9468

testsuite: Add some unit tests for #9466 #9467 #9468

mpickering commented Nov 24, 2023

aleeusgr commented Dec 18, 2023

aleeusgr left a comment •

edited

Loading

grayjay left a comment

grayjay Jan 4, 2024

alt-romes Mar 11, 2024 •

edited

Loading

alt-romes Apr 3, 2024

grayjay Jan 4, 2024

alt-romes Mar 11, 2024 •

edited

Loading

grayjay Mar 19, 2024

alt-romes Apr 3, 2024

alt-romes commented Mar 11, 2024

alt-romes commented Mar 14, 2024

grayjay Mar 19, 2024

alt-romes commented Apr 4, 2024

grayjay left a comment

Kleidukos commented May 16, 2024

mergify bot commented May 16, 2024 •

edited

Loading

testsuite: Add some unit tests for #9466 #9467 #9468

testsuite: Add some unit tests for #9466 #9467 #9468

Conversation

mpickering commented Nov 24, 2023

aleeusgr commented Dec 18, 2023

aleeusgr left a comment • edited Loading

Choose a reason for hiding this comment

grayjay left a comment

Choose a reason for hiding this comment

grayjay Jan 4, 2024

Choose a reason for hiding this comment

alt-romes Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

alt-romes Apr 3, 2024

Choose a reason for hiding this comment

grayjay Jan 4, 2024

Choose a reason for hiding this comment

alt-romes Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

grayjay Mar 19, 2024

Choose a reason for hiding this comment

alt-romes Apr 3, 2024

Choose a reason for hiding this comment

alt-romes commented Mar 11, 2024

alt-romes commented Mar 14, 2024

grayjay Mar 19, 2024

Choose a reason for hiding this comment

alt-romes commented Apr 4, 2024

grayjay left a comment

Choose a reason for hiding this comment

Kleidukos commented May 16, 2024

mergify bot commented May 16, 2024 • edited Loading

✅ Backports have been created

aleeusgr left a comment •

edited

Loading

alt-romes Mar 11, 2024 •

edited

Loading

alt-romes Mar 11, 2024 •

edited

Loading

mergify bot commented May 16, 2024 •

edited

Loading