Add tests for active-repositories feature#11684
Conversation
|
I asked Claude to give me an idea of the test coverage. Its response:
Where:
|
8a79072 to
335c948
Compare
andreabedini
left a comment
There was a problem hiding this comment.
I left a couple of comments but otherwise LGTM.
Not a deal breaker but any chance you can add a simple test for this edge case behaviour?
-- Note: currently if 'ActiveRepoRest' is provided more than once,
-- rest-repositories will be multiple times in the output.
| @?= sort [foo1, bar1] | ||
| , testCase "Merge+Override: override repo replaces all versions of overlapping package" $ | ||
| -- repoFoo12 has foo-1.0 and foo-1.1; repoFoo2 has only foo-2.0. | ||
| -- Override means repoFoo2 wins the entire 'foo' bucket. |
There was a problem hiding this comment.
This comments seems wrong. Below we have
repoFoo12 = PackageIndex.fromList [foo1, foo2]
So repoFoo12 has foo-1.0 and foo-2.0. I am not sure what was intended.
There was a problem hiding this comment.
The full test case is:
pkgs [(repoFoo12, CombineStrategyMerge), (repoFoo2, CombineStrategyOverride)] @?= [foo2]
So the test is to ensure that CombineStrategyOverride does in fact completely override CombineStrategyMerge.
Am I missing something?
There was a problem hiding this comment.
I was just referring to the comment repoFoo12 has foo-1.0 and foo-1.1. It seems to me that repoFoo12 has foo-1.0 and foo-2.0. Maybe I misunderstood.
cabal-install/tests/UnitTests/Distribution/Client/IndexUtils.hs
Outdated
Show resolved
Hide resolved
Merge Queue Status
This pull request spent 1 hour 5 minutes 23 seconds in the queue, including 3 seconds running CI. Required conditions to merge
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
Merge Queue Status
This pull request spent 11 minutes 12 seconds in the queue, including 44 seconds running CI. Required conditions to merge
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
|
A changelog file is missing, here is a suggestion: |
Merge Queue Status
This pull request spent 10 minutes 45 seconds in the queue, including 4 seconds running CI. Required conditions to merge
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
Merge Queue Status
This pull request spent 11 minutes 20 seconds in the queue, including 26 seconds running CI. Required conditions to merge
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
Merge Queue Status
This pull request spent 1 hour 34 minutes 43 seconds in the queue, including 3 seconds running CI. Required conditions to merge
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
Merge Queue Status
This pull request spent 4 minutes 26 seconds in the queue, with no time running CI. ReasonThe pull request #11684 has been manually updated HintIf you want to requeue this pull request, you can post a |
Merge Queue Status
This pull request spent 10 minutes 20 seconds in the queue, including 3 seconds running CI. Required conditions to merge
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
andreasabel
left a comment
There was a problem hiding this comment.
I am afraid I do not even know what the active-repositories feature is, so I cannot judge this PR.
However, when it comes to tests: the more, the merrier!
These tests were generated by Claude code, but manually reviewed.
|
The documentation for this feature is at: https://cabal.readthedocs.io/en/latest/cabal-project-description-file.html#cfg-field-active-repositories |
Merge Queue Status
This pull request spent 1 hour 54 minutes 13 seconds in the queue, including 3 seconds running CI. Waiting for:
All conditions
ReasonPull request #11684 has been dequeued merge conditions no longer match:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
This PR only contains tests, which were generated by Claude code, and then manually reviewed. Unfortunately I have not worked much on Cabal so my manual review may not be up-to-scratch.
This PR was submitted in preparation for taking over the work in #8997 and getting it merged. One of the things mentioned in that PR was that need for tests for the
active-repositoriesfeature.Include the following checklist in your PR: