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

Explicitly list testTargets #617

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TeofilC
Copy link

@TeofilC TeofilC commented Apr 19, 2024

testTarget is a space separated list of test-suites.

We explicitly list all test suites.
This allows us to implement logic in the nixpkgs builder to run test suites in separate passthru.test derivations. See: NixOS/nixpkgs#305958

Additionally it makes it easier to filter out test suites we don't want.

I've chosen to avoid changing the type of testTarget from a String to a [String] in order to avoid a breaking change. I think this would be a good change to make but I'm not sure how it would work in practice? Maybe we could add a new testTargets field in the generic builder and use that to transition?

@TeofilC TeofilC force-pushed the wip/list-testTargets branch from 7e57521 to c44a20e Compare April 19, 2024 13:39
`testTarget` is a space separated list of test-suites.

We explicitly list all test suites.
This allows us to implement logic in the nixpkgs builder to run test
suites in separate passthru.test derivations.
@TeofilC TeofilC force-pushed the wip/list-testTargets branch from c44a20e to 58341dd Compare April 19, 2024 13:40
@TeofilC TeofilC marked this pull request as draft April 19, 2024 16:26
@sternenseemann
Copy link
Member

If we're going to do this, I'd add testTargets to the nixpkgs builder and some extra logic to keep backwards compatibility with the old interface. I've been meaning to do this anyways.

W.r.t. this change, I'm not sure if this is the best course of action yet. It seems good in principle, however it is going to massively increase the size of hackage-packages.nix which is a bit of a problem.

Skipping tests could in principle be implemented in the builder without having data about the names of all test suites at eval time, so I'm curious about the second use case you mention. Can you maybe share the code for a proof of concept for that?

@maralorn
Copy link
Member

If we're going to do this, I'd add testTargets to the nixpkgs builder and some extra logic to keep backwards compatibility with the old interface. I've been meaning to do this anyways.

W.r.t. this change, I'm not sure if this is the best course of action yet. It seems good in principle, however it is going to massively increase the size of hackage-packages.nix which is a bit of a problem.

Skipping tests could in principle be implemented in the builder without having data about the names of all test suites at eval time, so I'm curious about the second use case you mention. Can you maybe share the code for a proof of concept for that?

I share the concern that hard coding the list of tests makes the derivations bigger and less flexible. (e.g. sometimes it is useful to use the same derivation with a different source, the more specific we make the derivation the less flexible it will be.)

Maybe we can introduce a list of disabled tests in the builder where we query cabal for the list of available tests and filter out disabled ones at build time and not at eval time?

@TeofilC
Copy link
Author

TeofilC commented Apr 22, 2024

Thanks for taking a look folks!

If we're going to do this, I'd add testTargets to the nixpkgs builder and some extra logic to keep backwards compatibility with the old interface. I've been meaning to do this anyways.

Cool I might make an MR to do this refactoring.

I share the concern that hard coding the list of tests makes the derivations bigger and less flexible

This is a very good point! The disabled test stuff is just a nice extra. My main aim with this is to enable passthru.tests style test suites. Take a look at NixOS/nixpkgs#305958. I think that, in its current form, this requires us to have the list of test suites at eval time. That's because it creates a passthru.tests._ derivation for each test suite.

I think there's two ways to proceed:

  1. put this behind a flag, so nixpkgs isn't burdened by this but end-users can enable it.
  2. Change the nixpkgs PR so that it creates just 1 derivation for ALL the test suites, so we don't require knowing at eval time the names of the test suites.
  3. Only emit this if there's more than 1 test suite. I feel like most packages just have 1, so this might be a nice compromise to avoid exploding the size while still having separate derivations for each?

I think we could also mix (1) and (2), where if testTargets is empty we just create one derivation or something like that.

What do you all think?

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.

3 participants