-
-
Notifications
You must be signed in to change notification settings - Fork 16.7k
nixos: Add meta.tests #413892
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
base: master
Are you sure you want to change the base?
nixos: Add meta.tests #413892
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea, this looks useful.
A few minor comments, most of which I could address in a followup if preferred. Some of my comments are more questions than feedback.
@@ -39,6 +39,64 @@ let | |||
# { file = "module location"; value = <path/to/doc.xml>; } | |||
merge = loc: defs: defs; | |||
}; | |||
|
|||
/** | |||
Custom type for `meta.tests` option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kinda redundant given the name & context, however many of the other custom types in this file describe how the value is merged; e.g.
# Returns tuples of
# { file = "module location"; value = <path/to/doc.xml>; }
and
# Returns list of
# { "module-file" = [
# "maintainer1 <[email protected]>"
# "maintainer2 <[email protected]>" ];
# }
Something like that may be more useful here 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A complete and accurate description of the input and output needs to be in the user facing documentation anyway, so I prefer to refer to that, rather than copy it to here.
nixos/modules/misc/meta.nix
Outdated
*/ | ||
testsType = lib.mkOptionType { | ||
name = "tests"; | ||
check = value: lib.isFunction value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this intentional?
check = value: lib.isFunction value; | |
check = lib.isFunction; |
An attrset structure of <testName>.<fileName> = derivation; | ||
*/ | ||
transposed = lib.zipAttrsWith (_k: lib.mergeAttrsList) ( | ||
lib.mapAttrsToList (fileName: values: lib.mapAttrs (k: v: { "${fileName}" = v; }) values) relevant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, was this intentional?
lib.mapAttrsToList (fileName: values: lib.mapAttrs (k: v: { "${fileName}" = v; }) values) relevant | |
lib.mapAttrsToList (fileName: lib.mapAttrs (k: v: { "${fileName}" = v; })) relevant |
Not really important, I'm just curious if you have a preference for either code style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are ok. I only have a preference in code that's performance critical, where eta-reduced performs better, producing fewer calls. Shorter error traces can also be nice.
nixos/modules/misc/meta.nix
Outdated
|
||
This is like `filter`, but solves the problem that `nix-build` ignores attributes with `.` in their names. | ||
*/ | ||
filterForNixBuild = paths: lib.concatMap lib.attrValues (lib.attrValues (filter paths)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be more readable as a pipe?
filterForNixBuild = paths: lib.concatMap lib.attrValues (lib.attrValues (filter paths)); | |
filterForNixBuild = paths: lib.pipe paths [ | |
filter | |
builtins.attrValues | |
(builtins.concatMap builtins.attrValues) | |
]; |
Although, more verbose so maybe not worth it.
Alternatively, how about using collect isDerivation
? Maybe this would be less efficient, but it might convey the intention more clearly:
filterForNixBuild = paths: lib.concatMap lib.attrValues (lib.attrValues (filter paths)); | |
filterForNixBuild = paths: lib.collect lib.isDerivation (filter paths); |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it might convey the intention more clearly:
It somewhat blurs the intent, because it's two layers and not something arbitrarily deep.
I'll add:
# Two calls to `attrValues` because we have exactly two layers to flatten.
nixos/modules/misc/meta.nix
Outdated
|
||
This can be used for tooling to figure out which tests are relevant, given a set of changed files. | ||
''; | ||
default = { ... }: { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default = { ... }: { }; | |
default = { ... }: { }; | |
defaultText = lib.literalExpression "{ ... }: { }"; |
or
default = { ... }: { }; | |
default = _: { }; | |
defaultText = lib.literalExpression "{ nixosTests }: { }"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm un-documenting the default because it's niche and un-interesting.
The description
covers it all.
That also means I won't add an example
(not that you were suggesting to), as that would be confusing because the definitions and option value are so different.
nixos/modules/misc/meta.nix
Outdated
|
||
Definitions are tied to the location of the module, so the definition syntax is distinct from the option value format. | ||
|
||
Example definition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example definition: | |
For example, this definition: |
nixos/modules/misc/meta.nix
Outdated
} | ||
``` | ||
|
||
Example option value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example option value: | |
Would produce the following option value: |
nixos/modules/misc/meta.nix
Outdated
```nix | ||
# module.nix | ||
{ | ||
meta.tests = { nixosTests }: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: nixfmt would format like this:
meta.tests = { nixosTests }: | |
meta.tests = | |
{ nixosTests }: |
Should the example be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is supposed to be used:
- manually as a script, it should probably be in
maintainers/scripts/...
. - by CI, then it should be somewhere in
ci/
. - by ofborg only, then it should be in the ofborg repo (?)
I'd say we want this to be used by CI, too. It would be helpful to expose the list of tests which need to be run in the comparison artifact, so that nixpkgs-review
can pick it up and run those tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess computing a list in nix is too expensive (See roberts explanation in the topic). nixpkgs-review can instead implement a feature to call that function with the list of changed files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess computing a list in nix is too expensive
The file header literally says:
Returns a list of tests for a given list of NixOS module files (and/or other files which will be ignored).
I think you assume I meant to eval all tests for difference. I did not. Same approach, just based on changed files. The goal of this PR is clearly to run stuff in any of the CIs - and I think the location of the file is just wrong for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a convenience wrapper really, and I would expect it to be called by CI and maintainers.
maintainers/scripts/
seems like the right place for that. I just forgot to add /scripts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a convenience wrapper really, and I would expect it to be called by CI and maintainers.
maintainers/scripts/
seems like the right place for that. I just forgot to add/scripts
.
I think the script is placed correctly in maintainers/scripts
right now, but I don't think this should be the entry-point for OfBorg. Everything in maintainers/scripts
should be free to change their interfaces easily, to make the scripts more useful for regular use.
This doesn't need to be figured out in this PR, though. But, we should think about this with #272591 in mind, before taking steps on the OfBorg side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not infer the mapping automatically?
Basically the same reasons we have
passthru.tests.nixos
.1. Too costly. We'd have to evaluate all tests to figure out which ones changed. 2. For some modules it is not feasible to run all affected tests, because that set is too large. We need a representative smaller set. 3. This could perhaps be scripted as a one-off or infrequent process that updates the mapping, but then it is _time of all evals_ * *number of files under `nixos/`... And then there's still some risk that it's not a good mapping for some reason. Doesn't solve (2)
This reasoning makes sense to me.
I tested the interface and it works fine. I think it's a good idea in general.
Returns a list of tests for a given list of NixOS module files (and/or other files which will be ignored). | ||
|
||
A more detailed structure than a list is available in the `meta.tests` option value. | ||
|
||
Example: | ||
nix-build maintainers/nixos-tests-for-files.nix --arg files '[ nixos/modules/services/databases/postgresql.nix ]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this - and it doesn't just return a list of tests, but it also builds all of them. Is that intended?
I would have expected to just get back a list of... maybe attributes to build or so? Instead, I just ran 45 VM tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nix-build
builds whatever you provide it with. If the function returns an attrset, it will build all those attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question is whether we should be suggesting nix-instantiate
instead of or alongside nix-build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And/or rephrase the comment, because that's where my expectation came from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to Example usage (to be run from the Nixpkgs root):
I think nix-build
is generally what you want when calling this by hand, and I'm sure we can figure out that instantiate is also ok.
If we really want to make this easy, we should add a shell script wrapper with a --dry-run
or --eval
flag, but let's keep it simple for now.
Note: I might be missing something in the following, because I don't know the module system well enough. I couldn't find a way to get back a list of the files for the tests belonging to a module. This would be helpful for things like maintainer pings, for example. If I see a test file is touched and I can connect it easily to modules that depend on this test, then I can ping those maintainers. See also #413097, where the idea is to get module maintainers from changed files for modules. I think the problem is the indirection through This would kind of move the definition of "treat this file as a test", which currently happens in The alternative, to get that mapping back from tests to their file, would be something like Or maybe a combination of both? TLDR: I'm not sure whether the current interface of |
Instead of defining the test inside the module file via meta, would it make more sense to have them declared outside the file via some convention, for example a directory called Apart from that, I'm wondering how exactly it would look like if we used this to test the minimal modules list. Essentially we would want to test if each module individually evaluates successfully against the minimal list of modules. I guess, we probably don't want to modify each individual file to add this generic test to |
I don't think the files are what you want, especially if you can have the actual tests, which have Also relevant:
Yes, although for nightly CI / build farm duty I think the explicit list may still be more efficient. Other than that, I agree with those thoughts.
It's an attrset argument, so that it can evolve as needed. So yes, changes are possible. My goal here is to establish something that works, and I believe it already works quite well this way. |
Non-zero but cheap. We're not actually instantiating the tests, and the test framework's use of
Currently, you'd might add a This also reminds me that the "plain set of test files" approach does not handle which test entrypoint is to be used. It's also not obvious to me how you'd construct a test matrix except adding a bunch of boilerplate files.
Making this explicit is a good thing. It shows which modules are tested and maintained in a modular way, and which ones are still part of the module soup. |
The files are what I want, because I can match them with the changed files in a PR. Yes, That should make for much better maintainer pings. (Not really relevant for codeowners, because codeowners define the file<->owner relationship directly)
Yeah, I don't see the problem with extensibility, but with the fact that the indirect way through |
Because you mention Now, applying the same concept to NixOS tests, that I mentioned earlier:
With this approach, the current WDYT? |
Got it. You want it to work for test files too, and for that we need #403839. module file <-> test (this PR) Alternatively, you could sidestep the module system and make everything directory-based with CODEOWNERS. |
#403839 knows it even better; it knows the whole imports closure, not just the top, and it can see through anonymous modules. Sgtm, but also no problem to pivot to something else, e.g. directory based, if at some point that seems to be better. |
OK, cool. If that works, I'm happy.
Yeah - I have different ideas for codeowners. I ideally want less of them, not more. |
054710c
to
7b2a135
Compare
b66ddf9
to
bc73e5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a module-magician, so can't comment much on those parts. The concept is great, my concerns about the interface resolved.
The obvious way to use this is for OfBorg to build NixOS tests. But we should also think about how to integrate this with nixpkgs-review and/or GitHub Actions CI.
Since nixpkgs-review already pulls eval results from GHA, it could also pull a list of nixosTests
attributes to build. Alternatively, if not expensive to evaluate, nixpkgs-review could do that itself as well, ofc.
In CI, we could make use of the data, to maybe add labels. I don't think "rebuilds tests" labels with numbers make sense, because we don't really know how many tests/modules would be affected on top of those linked. But maybe something like "has required tests" or something, which gives a quick indication of "Are there any tests associated to the module, that I should watch out for?".
Maintainer pings have already been discussed.
Anything else this could be used for?
nixos/modules/misc/meta.nix
Outdated
@@ -39,6 +39,73 @@ let | |||
# { file = "module location"; value = <path/to/doc.xml>; } | |||
merge = loc: defs: defs; | |||
}; | |||
|
|||
# TODO: add to lib? | |||
resolveDefaultNix = p: if lib.pathType p == "directory" then p + "/default.nix" else p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To potentially help the decision on the TODO: This is done elsewhere, too:
Line 294 in 2147c83
toString fn + optionalString (pathIsDirectory fn) "/default.nix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd removed the TODO while fixing my definition, but actually that made them equivalent, so yes. This should be done, and it should be done in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whichever merges first, I'll adjust the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a convenience wrapper really, and I would expect it to be called by CI and maintainers.
maintainers/scripts/
seems like the right place for that. I just forgot to add/scripts
.
I think the script is placed correctly in maintainers/scripts
right now, but I don't think this should be the entry-point for OfBorg. Everything in maintainers/scripts
should be free to change their interfaces easily, to make the scripts more useful for regular use.
This doesn't need to be figured out in this PR, though. But, we should think about this with #272591 in mind, before taking steps on the OfBorg side.
Discovery and producing the mapping should be pretty quick, based on
That is after applying a trivial fix ( The real cost is in the instantiation of the actual test derivations, which needs to happen locally on the nixpkgs-review machine anyway. I don't know much about how I can't estimate the cost of the |
bdb60df
to
4aaa2ba
Compare
This provides the needed infrastructure to make tooling like ofborg build the relevant tests, given a set of changed files. Having mandatory CI for modules is not just crucial for DX, but also prerequisite to test-driven expansion of NixOS use cases, such as alternate base module sets. (minimal module list and similar ideas)
This makes it clear that it's a path value literal. We generally prefix ./ even if it's not strictly necessary, because it helps the reader. Co-authored-by: Matt Sturgeon <[email protected]>
Not super quick, but faster than the postgresql case.
1b58dbf
to
27d8ee3
Compare
_file can be set to a string that does not represent a file path. This is a useful and accepted practice, so it should just work.
Co-authored-by: Wolfgang Walther <[email protected]>
This provides the needed infrastructure to make tooling like ofborg build the relevant tests, given a set of changed files.
Having mandatory CI for modules is not just crucial for DX, but also prerequisite to test-driven expansion of NixOS use cases, such as alternate base module sets. (minimal module list and similar ideas)
The option is documented.
We can provide additional instructional docs elsewhere when this is wired into ofborg.
Why not infer the mapping automatically?
Basically the same reasons we have
passthru.tests.nixos
.nixos/
... And then there's still some risk that it's not a good mapping for some reason. Doesn't solve (2)Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.