python3Packages.buildPythonPackage: default enabledTestPaths = [ "." ] by ShamrockLee · Pull Request #425191 · NixOS/nixpkgs

ShamrockLee · 2025-07-14T19:46:00Z

Description

Summary

This is a proposal to explicitly provide a default value for the argument enabledTestPaths as the current working directory (enabledTestPaths = [ "." ]) to simplify overriding (making enabledTestPaths always specified as a non-empty list) while preserving pytest's default behaviour of discovering tests in the current working directory.

Background

pytestCheckHook's current test-selection interface consists of the following attributes:

enabledTestPaths and disabledTestPaths
enabledTestMarks and disabledTestMarks
enabledTests and disabledTests

Where enabledTests and the current implementation of (enabled|disabled)(TestMarks|Tests) is provided in PR #386513

To avoid semantic confusion, the enabled-attributes are not allow to take an empty list ([ ]). They can either be unspecified, null, or a non-empty list. This results in a rather complex overriding interface -- The overriding function need to handle the edge case where the previous state of the attribute might be unspecified or null before starting list operation, and set it to null and optionally doCheck to false when the processed list might otherwise be empty.

After the proposed change, enabledTestPaths should always be specified as a non-empty list, which greatly simplifies the overriding.

Drawbacks

Global (as in buildPythonPackage) namespace pollution
- Reason: specifying an attribute specific to pytestCheckHook globally even when the setup hook is not used seems unclean and could be confusing.
- Refutation: pytest is the de facto standard for writing Python tests, which somehow justifies its space in the buildPythonPackage-level.
Inconsistency between other two enabled(Tests|TestMarks) attributes
- Reason: After this change, the way to override enabledTestPaths will differ from that of enabledTestMarks and enabledTests.
- Refutation: The logic behind enabledTestPaths and the other two enabled-attributes are quite different, and the former occurs much more often than the latter.

Things done

Add a 👍 reaction to pull requests you find important.

wolfgangwalther · 2025-07-15T10:06:53Z

Just to be clear the "complication" in overriding is enabledTestPaths = prev.enabledTestPaths or [] ++ [ my stuff ] instead of enabledTestPaths = prev.enabledTestPaths ++ [ my stuff ], correct?

If that's the case.. this is such a common overriding pattern, I really don't think we need to fix this here. So I'm 👎 on this change.

But maybe I'm missing something.

ShamrockLee · 2025-07-15T10:41:18Z

Just to be clear the "complication" in overriding is enabledTestPaths = prev.enabledTestPaths or [] ++ [ my stuff ] instead of enabledTestPaths = prev.enabledTestPaths ++ [ my stuff ], correct?

It would be

enabledTestPaths = (if prev.enabledTestPaths or null == null then [ "." ] else prev.enabledTestPaths) ++ [ "my stuff" ]

without this change.

wolfgangwalther · 2025-07-15T10:50:06Z

Ah, I see the problem now. I went through the discussions in #386513 twice, but I couldn't find anything discussing the choice of allowing null. We discussed whether to allow the empty list or not, but it doesn't seem like we considered the case of null.

Why do we allow null here at all? Can we just forbid this, too?

wolfgangwalther · 2025-07-15T10:52:49Z

But I see that even if we solve the null problem, this would still not solve the "extend the array with new items takes other items away problem", because of the implicit default value involved. So if we remove null, it would still need to be:

enabledTestPaths = prev.enabledTestPaths or [ "." ] ++ [ my stuff ]

Which might not be too bad, but I'm not sure about the practicability. It's certainly unintuitive.

wolfgangwalther

The change as is makes sense.

I'd say we could still consider disallowing null from being allowed, but not sure. Might not be needed, but we should at least remove it from the list of "OK values" in the error message.

doc/languages-frameworks/python.section.md

ShamrockLee · 2025-07-15T11:57:41Z

But I see that even if we solve the null problem, this would still not solve the "extend the array with new items takes other items away problem", because of the implicit default value involved. So if we remove null, it would still need to be:
enabledTestPaths = prev.enabledTestPaths or [ "." ] ++ [ my stuff ] 
Which might not be too bad, but I'm not sure about the practicability. It's certainly unintuitive.

Argh! <pkg>.overridePythonAttrs is hard to cope with. We'll need prev.enabledTestPaths or [ "." ] unless we add such hack to the already-hacky makeOverridablePythonPackage.

<pkg>.overrideAttrs should support prev.enabledTestPaths ++ [ "my stuff" ] after this change. Maybe I should add some test cases in tests.overriding.

MattSturgeon · 2025-07-15T12:49:17Z

So if we remove null, it would still need to be:
enabledTestPaths = prev.enabledTestPaths or [ "." ] ++ [ my stuff ] 
Which might not be too bad, but I'm not sure about the practicability. It's certainly unintuitive.

To me, personally, I actually find it more intuitive that prev would not contain an enabledTestPaths attr of one was not specified when creating the package.

I usually conceptualise prev as "the previous arguments to mkDerivation (or whatever builder, buildPythonPackage in this case). Therefore if that argument was previously not specified, I wouldn't expect it to magically exist in prev.

That said, I can see how the attr being missing having a default behaviour (being equivalent to ["."]) would be confusing...

One issue with this proposal is it makes it impossible for the override to explicitly not include "." in the list.

Would it make sense to have a separate attr like enableCwdTestPath that defaults to true when missing?

That way . is always included no matter what enabledTestPaths is set to, but can be separately disabled using enableCwdTestPath (or whatever it ends up being named).

wolfgangwalther · 2025-07-15T12:53:11Z

One issue with this proposal is it makes it impossible for the override to explicitly not include "." in the list.

I thought about this, too, but consider:

you can always set enableTestPaths = [ something ] without refering to prev for a full override without considering previous layers.
if you actually want to disable some stuff, the various disableXX attrs are probably much better. It's rarely a good idea to say "I only want to build X" in an overlay... because it suffers from the same problems as "not considering previous overlay". What if the source changes and adds some new tests - you'd silently ignore those, too. By making more explicit what you ignore, you'd be composing with the evolution of the source over time better, I'd say.

MattSturgeon · 2025-07-15T13:48:23Z

One issue with this proposal is it makes it impossible for the override to explicitly not include "." in the list.

I thought about this, too, but consider:

you can always set enableTestPaths = [ something ] without refering to prev for a full override without considering previous layers.

I was wrong to say "impossible"... Somehow I momentarily forgot that the prev value must be explicitly included in the new value, using ++.

So if one wanted part of the old value, they could use standard list manipulation tools like lib.remove.

The only confusing thing they'd need to know is that "." is implicitly added to the actual list, if it was missing or empty.

mweinelt · 2025-07-15T17:02:31Z

This should be lib.mkDefault, so that any explicit configuration overwrites it.

wolfgangwalther · 2025-07-15T17:07:35Z

Is lib.mkDefault a thing for pkgs / mkDerivation args?

MattSturgeon · 2025-07-15T18:51:01Z

Is lib.mkDefault a thing for pkgs / mkDerivation args?

No. That's only available when using the module system.

MattSturgeon

👍

doc/release-notes/rl-2511.section.md

doc/languages-frameworks/python.section.md

ShamrockLee · 2025-08-02T05:34:09Z

we should at least remove it from the list of "OK values" in the error message.

Addressed.

I'd say we could still consider disallowing null from being allowed, but not sure.

I also think we should enforce this, but not sure about how to implement for a smooth transition experience.

Should we backport the backward-compatible part of this change, and enforce the new rule in a separate commit?

wolfgangwalther · 2025-08-03T15:35:07Z

Should we backport the backward-compatible part of this change, and enforce the new rule in a separate commit?

Is there a backwards-compatible part of this change at all? Not having a default causes a certain behavior when overriding, and this behavior is changed now.

Personally, I'd be more concerned about that default-value-change than a change about disallowing null. Thus, I think we can either backport both or none of these. I don't think we necessarily need to split them into separate commits.

ShamrockLee · 2025-08-07T22:59:40Z

Personally, I'd be more concerned about that default-value-change than a change about disallowing null. Thus, I think we can either backport both or none of these. I don't think we necessarily need to split them into separate commits.

I see. The non-empty-list assertion is now enforced.

I adjusted the release note to reflect the changes. Please take a look!

wolfgangwalther

LGTM reading through, but didn't run any code.

ShamrockLee · 2025-08-09T05:29:09Z

@ofborg build python3Packages.pytestCheckHook.tests
pypck's source module directory is defined under the project root instead of src/.
@ofborg build python3Packages.pypck

Explicitly provide a default value for the argument `enabledTestPaths` as the current working directory (`enabledTestPaths = [ "." ]`) to simplify overriding (making `enabledTestPaths` always specified as a non-empty list) while preserving `pytest`'s default behaviour of discovering tests in the current working directory. Assert the specified `enabledTestPaths` value to be a non-empty list.

ShamrockLee · 2025-08-17T20:44:05Z

Both python3Packages.pytestCheckHook.tests and python3Packages.pypck build. Sounds like a good news.

wolfgangwalther · 2025-08-19T12:39:07Z

while preserving pytest's default behaviour of discovering tests in the current working directory.

It seems like this default behavior is not as simple as assumed, see #434974 (comment).

I assume this affects potentially a lot of packages, so I think we should revert this?

ShamrockLee · 2025-08-19T12:45:51Z

while preserving pytest's default behaviour of discovering tests in the current working directory.

It seems like this default behavior is not as simple as assumed, see #434974 (comment).

I assume this affects potentially a lot of packages, so I think we should revert this?

Same vibe. Let's revert.

ShamrockLee mentioned this pull request Jul 14, 2025

pytestCheckHook: support inclusion and exclusion of path globs, test items, keywords, and markers #386513

Merged

13 tasks

wolfgangwalther approved these changes Jul 15, 2025

View reviewed changes

doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved

nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jul 15, 2025

ShamrockLee marked this pull request as ready for review July 15, 2025 11:51

nix-owners bot requested review from mweinelt and natsukium July 15, 2025 11:53

MattSturgeon reviewed Jul 15, 2025

View reviewed changes

doc/release-notes/rl-2511.section.md Outdated Show resolved Hide resolved

doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved

ShamrockLee force-pushed the enabledTestPaths-default branch from 3a56748 to 75e3953 Compare August 2, 2025 05:27

ShamrockLee force-pushed the enabledTestPaths-default branch from 75e3953 to 4f646ec Compare August 2, 2025 05:59

ShamrockLee force-pushed the enabledTestPaths-default branch from 4f646ec to e3372dc Compare August 7, 2025 22:56

ShamrockLee force-pushed the enabledTestPaths-default branch from e3372dc to a9ad573 Compare August 7, 2025 23:02

wolfgangwalther approved these changes Aug 8, 2025

View reviewed changes

nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 14, 2025

ShamrockLee force-pushed the enabledTestPaths-default branch from a9ad573 to 5bf25df Compare August 17, 2025 20:43

ShamrockLee requested a review from MattSturgeon August 17, 2025 20:49

nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 17, 2025

wolfgangwalther merged commit 746cd1f into NixOS:staging Aug 18, 2025
26 of 28 checks passed

me-and mentioned this pull request Aug 19, 2025

Build failure: python3Packages.cssselect #434974

Closed

3 tasks

wolfgangwalther mentioned this pull request Aug 19, 2025

Revert "python3Packages.buildPythonPackage: default enabledTestPaths = [ "." ]" #434978

Merged

Uh oh!

Conversation

ShamrockLee commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary

Background

Drawbacks

Things done

Uh oh!

wolfgangwalther commented Jul 15, 2025

Uh oh!

ShamrockLee commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jul 15, 2025

Uh oh!

wolfgangwalther commented Jul 15, 2025

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShamrockLee commented Jul 15, 2025

Uh oh!

MattSturgeon commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jul 15, 2025

Uh oh!

MattSturgeon commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mweinelt commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Jul 15, 2025

Uh oh!

MattSturgeon commented Jul 15, 2025

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ShamrockLee commented Aug 2, 2025

Uh oh!

wolfgangwalther commented Aug 3, 2025

Uh oh!

ShamrockLee commented Aug 7, 2025

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

ShamrockLee commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShamrockLee commented Aug 17, 2025

Uh oh!

Uh oh!

wolfgangwalther commented Aug 19, 2025

Uh oh!

ShamrockLee commented Aug 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ShamrockLee commented Jul 14, 2025 •

edited

Loading

ShamrockLee commented Jul 15, 2025 •

edited

Loading

MattSturgeon commented Jul 15, 2025 •

edited

Loading

MattSturgeon commented Jul 15, 2025 •

edited

Loading

mweinelt commented Jul 15, 2025 •

edited

Loading

ShamrockLee commented Aug 9, 2025 •

edited

Loading