nixos/testing: fix testScript eval for functions without elipsis#501599
Merged
Mic92 merged 1 commit intoNixOS:masterfrom Mar 21, 2026
Merged
nixos/testing: fix testScript eval for functions without elipsis#501599Mic92 merged 1 commit intoNixOS:masterfrom
Mic92 merged 1 commit intoNixOS:masterfrom
Conversation
13 tasks
Mic92
changed the title
...
Mic92
commented
Mar 20, 2026
| v: | ||
| if lib.isFunction v then | ||
| # Only pass args the testScript function expects. | ||
| args: v (builtins.intersectAttrs (lib.functionArgs v) args) |
Member
Author
There was a problem hiding this comment.
We could think later about making the old form a warning, but for now we should probably fix evaluation in staging asap.
Member
Author
Member
Author
|
Mainly tested eval. |
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
Member
|
Shouldn't this target nixpkgs master? The commit being fixed by this is in there already. |
jfly
approved these changes
Mar 20, 2026
Contributor
jfly
left a comment
jfly
left a comment
There was a problem hiding this comment.
Thanks for the quickfix! One nit inline that hopefully doesn't matter IRL (I haven't checked).
| v: | ||
| if lib.isFunction v then | ||
| # Only pass args the testScript function expects. | ||
| args: v (builtins.intersectAttrs (lib.functionArgs v) args) |
Contributor
There was a problem hiding this comment.
Non blocking: This wouldn't work for functions that only take a positional parameter:
nix-repl> lib.functionArgs (args: 42)
{ }
nix-repl> lib.functionArgs ({}: 42)
{ }
Does nix provide some mechanism to detect such a difference?
Member
There was a problem hiding this comment.
Got stuck due to indecision two years ago
Member
There was a problem hiding this comment.
What's more likely to happen, testScript = { ... }: '' '' or testScript = args: '' ''?
AFAIU we can't fix both.
nixpkgs-ci
bot
added
12.approvals: 1
Ma27
reviewed
Mar 20, 2026
Ma27
reviewed
Mar 20, 2026
Member
Ma27
left a comment
Ma27
left a comment
There was a problem hiding this comment.
If we do this, I'd much prefer an eval warning for deprecation and a fix in-tree for all affected tests, it's not that many after all:
nixos/tests/spire.nix
61: testScript =
62: { nodes }:
nixos/tests/nixops/default.nix
59: testScript =
60: { nodes }:
nixos/tests/drbd.nix
57: testScript =
58: { nodes }:
nixos/tests/rspamd-trainer.nix
139: testScript =
140: { nodes }:
nixos/tests/parsedmarc/default.nix
93: testScript =
94: { nodes }:
204: testScript =
205: { nodes }:
nixos/tests/munge.nix
14: testScript =
15: { nodes }:
nixos/tests/discourse.nix
167: testScript =
168: { nodes }:
nixos/tests/web-apps/snipe-it.nix
46: testScript =
47: { nodes }:
nixos/tests/web-apps/peering-manager.nix
20: testScript =
21: { nodes }:
Member
|
We can't discern Fixing 9 tests and letting out of tree users fix it for themselves without any hint whatsoever is the only other option, it seems. I don't think that's preferable. |
Ma27
added a commit
to Ma27/nixpkgs
that referenced
this pull request
Mar 21, 2026
The addition of the nspawn backend introduced a second argument passed to the attribute-set passed to `testScript`, i.e. containers[1]. Note: this is by no means intended to replace the compat fix from NixOS#501599, I believe we shouldn't have deprecated invocations in-tree at all, hence the change on this end. This is intended to only fix address the fallout from NixOS#478109, `nixosTests.parsedmarc` fails due to some postfix module changes to evaluate and the test of `peering-manasger` still seems broken. [1] NixOS#478109
13 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Mic92
pushed a commit
to Ma27/nixpkgs
that referenced
this pull request
Mar 21, 2026
The addition of the nspawn backend introduced a second argument passed to the attribute-set passed to `testScript`, i.e. containers[1]. Note: this is by no means intended to replace the compat fix from NixOS#501599, I believe we shouldn't have deprecated invocations in-tree at all, hence the change on this end. This is intended to only fix address the fallout from NixOS#478109, `nixosTests.parsedmarc` fails due to some postfix module changes to evaluate and the test of `peering-manasger` still seems broken. [1] NixOS#478109
The nspawn container support (23f1e63) added a `containers` argument to the testScript caller. This breaks tests whose testScript uses strict pattern matching without ellipsis, e.g. `{ nodes }:`, since Nix rejects unexpected arguments. Use `builtins.intersectAttrs` to only pass arguments the function actually expects, making the caller forward-compatible with future argument additions. Fixes: 23f1e63 ("nixos/test-driver: add support for nspawn containers")
Mic92
force-pushed
the
fix-testscript-containers-arg
branch
from
95a43fd to
00f5ae0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The nspawn container support (23f1e63) added a
containersargument to the testScript caller. This breaks tests whose testScript uses strict pattern matching without ellipsis, e.g.{ nodes }:, since Nix rejects unexpected arguments.Use
builtins.intersectAttrsto only pass arguments the function actually expects, making the caller forward-compatible with future argument additions.Fixes: 23f1e63 ("nixos/test-driver: add support for nspawn containers")
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.