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

testsuite: Introduce Cabal-tests library for common testsuite functions #9454

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

mpickering
Copy link
Collaborator

I noticed that Distribution.Utils.TempTestDir was only used in the testsuite but defined in the Cabal library. Rather than expose this in the public interface of the Cabal library, it is cleaner to refactor it into a separate library (Cabal-tests) which can be used by any testsuite component.

Also, in future it gives a clearer place to put utility functions which need to be shared across the testsuite but not exposed in Cabal. Cabal-tests can also freely add dependencies (such as exceptions) which we might want to avoid adding to the Cabal library.

Fixes #9453

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this module to Cabal initially, after discussing the options with @gbaz . We thought that a separate library is a little bit of an overkill, but I agree that it's a cleaner solution. And certainly more sustainable going forward.

Thanks for putting the effort!

@michaelpj
Copy link
Collaborator

This could also be an opportunity to dogfood multiple public libraries in Cabal itself by having a Cabal-testlib public library in Cabal with test utilities?

@mpickering
Copy link
Collaborator Author

This could also be an opportunity to dogfood multiple public libraries in Cabal itself by having a Cabal-testlib public library in Cabal with test utilities?

I don't see why users of Cabal library should use these functions, they seem at home to me in Cabal-tests package away from distribution.

@mpickering
Copy link
Collaborator Author

Can we have another review please so I can merge this?

@ulysses4ever
Copy link
Collaborator

I put a needs-review label in the hope that it'll help to find other reviewers. Maybe we should have done it earlier.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 25, 2023

@mpickering: and you can set the merge label ahead of time as well. It also needs a rebase, because GHA settings have changed and the CI in the PR is not in sync with that.

@mpickering mpickering added the merge me Tell Mergify Bot to merge label Nov 27, 2023
@mpickering
Copy link
Collaborator Author

I rebased, and now I have to wait (another) two days before this gets merged?

@Mikolaj
Copy link
Member

Mikolaj commented Nov 27, 2023

I hope a rebase does not reset the counter, but a comment does. :)

@ulysses4ever
Copy link
Collaborator

I hope a rebase does not reset the counter, but a comment does

I think pretty much everything does (Mergify just uses the GitHub API endpoint called updated). So, this is a good case for manual intervention, perhaps.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 27, 2023

@mpickering: shall I manually intervene?

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 29, 2023
I noticed that Distribution.Utils.TempTestDir was only used in the
testsuite but defined in the Cabal library. Rather than expose this in
the public interface of the `Cabal` library, it is cleaner to refactor
it into a separate library (`Cabal-tests`) which can be used by any
testsuite component.

Also, in future it gives a clearer place to put utility functions which
need to be shared across the testsuite but not exposed in Cabal.
Cabal-tests can also freely add dependencies (such as exceptions) which
we might want to avoid adding to the Cabal library.

Fixes haskell#9453
@mergify mergify bot merged commit 39d42e6 into haskell:master Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribution.Utils.TempTestDir is defined in Cabal but only used in testsuite
6 participants