-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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
[RFC] Use meta.tests
to link from packages to the tests that test them
#44439
Conversation
Any trick in mind to make it more palatable than a path full of |
Hmm… From what I understand, it'd require a refactoring from |
I think such a PR should include some addition to documentation and to validation scripts in Nixpkgs tree (I don't remember the precise location of this code) — unknown meta entries get reported as typos by various tools. |
@@ -74,5 +74,8 @@ stdenv.mkDerivation rec { | |||
description = "Open source IMAP and POP3 email server written with security primarily in mind"; | |||
maintainers = with stdenv.lib.maintainers; [ peti rickynils fpletz ]; | |||
platforms = stdenv.lib.platforms.unix; | |||
tests = [ | |||
(import ../../../../nixos/tests/opensmtpd.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.
There should also be a link to nixos/tests/dovecot.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.
Indeed, I had only picked the opensmtpd
test I had written as an example for the purpose of the RFC, all tests should be added to all relevant packages, I think so too :) There appear to be ~200 of them, though, so I'm not sure all of them could reasonably go into a single PR without risking to have bitrotten before even being completed?
I'm not quite sure whether this really should belong together, because the NixOS VM tests are more tailured towards the NixOS service, so tests should ideally be linked to the NixOS module (possibly in a way so you can easily say "run all tests for the current configuration", prototype here) rather than to the package. If the test is really a test for the package itself (like unit tests etc.), then something like this sounds more appropriate. If this is only for running tests on pull requests, this sounds better, but I also don't like the |
Using attribute paths btw. also has the benefit that |
@aszlig if some package is disproportionately likely to break a NixOS test, we might want to schedule that test anyway. If the test is executed in a background batch together with some rebuilds, and the test is already debugged for other reasons, VM overhead is less of an issue. |
We (though I don’t remember who besides me) already had a type cobbled together that these tests should have: In plain words, |
@7c6f434c I think the second commit of this PR (added after I noticed the ofborg check failed) should do the “updating the validation scripts” part :) @aszlig To me, the point of I think a scheme, like you put forward, of tying tests to a NixOS configuration, would be useful too, but I'm not sure it'd fit the same aim, as your scheme appears to be more oriented towards individuals accepting (or not) a channel bump as an upgrade :) Basically, here I'm using a NixOS test because that's all I have to smoke-test that OpenSMTPD / dovecot appear to be working relatively consistently (ie. are able to talk to each over on something that at least look like resp. SMTP and binary-MDA). It's not particularly tied to NixOS in my opinion, but NixOS is (currently) the only testing framework that we have :) @Profpatsch Your proposal appears much more complete than mine! would you rather I close this PR and you open one about it, or I include your changes into mine? Also, there are still one point I'm not fully comfortable with with your There are two types of test I can think of off the top of my head that could be used right now:
And potentially in the future, there could be a testing scheme that doesn't require a full-blown NixOS system, like the one @7c6f434c proposed (test category 3). I think the filtering could be done along two axes:
I'm not completely sure about case 2 (it could be done by directly including the test case for What do you think about this? :) I've added the still-to-do points to the top post, will try to get some done tomorrow :) |
I think we should basically have an open record of properties for tests.
These options are then used by the test runner to decide which tests to run and how long the run can last until it is aborted. More options might be added later. (note: I’m using my |
8d582fb
to
7bb814b
Compare
Success on aarch64-linux (full log) Attempted: dovecot, opensmtpd Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: dovecot, opensmtpd Partial log (click to expand)
|
I think all the points that were requiring fixing are now fixed :) @Profpatsch I have re-used the pre-existing |
Success on aarch64-linux (full log) Attempted: dovecot, opensmtpd Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: dovecot, opensmtpd Partial log (click to expand)
|
Success on x86_64-linux (full log) Attempted: dovecot, opensmtpd Partial log (click to expand)
|
Success on aarch64-linux (full log) Attempted: dovecot, opensmtpd Partial log (click to expand)
|
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.
Not sure, but I think this needs to use some cross-stuff.
|
||
nixosTests = | ||
let | ||
system = builtins.currentSystem; |
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 doesn’t work, I think. cc @Ericson2314
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 don't really know: NixOS test derivations are supposed to be built on the local system, which I'd guess to be builtins.currentSystem
? Not sure at all though, so cc @Ericson2314 indeed :) I.must find it hard to imagine what's the cross-derivation story of NIxOS 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.
@Ericson2314 ping :) (2 months sounds like a reasonable waiting time :p)
I wonder if we ever find a way to specify timeouts properly (to express a linear combination of some calibration build, maybe a minimal VM boot time, wall-clock seconds and maybe something more) without losing our minds. But of course it is not a blocker here. |
It should be worth a look to see how bazel does it, and also other build tools that support tests. |
Sorry I diddn't see this. |
It's not letting me comment inline for some reason, but |
@Ericson2314 I've opened #50028 to (hopefully) handle your first comment. The second appears like a much larger refactoring. Do you think we should go back on this change until we get it exactly right, or is it “good enough” for now and can it be improved upon once we manage to do the refactoring? |
Why is the evaluator complaining:
|
@matthewbauer Yes, that's the idea, along with having ofborg automatically build these :) Do you think here would be a good place to add this @FRidh Where are you seeing that? I've noticed ofborg complain about missing attributes (for tests that don't exist in the system, though I don't know whether we actually have those in practice and I hit this only when trying to fix the cross-compilation story), but can't find where this happens for |
@Ekleog run |
Unknown |
@peti perhaps we should revert this entire PR, then? |
An attribute set with as values tests. A test is a derivation, which | ||
builds successfully when the test passes, and fails to build otherwise. A | ||
derivation that is a test requires some <literal>meta</literal> elements | ||
to be defined: <literal>needsVMSupport</literal> (automatically filled-in |
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 should probably not be created, since this is already covered by the requiredFeatures of the derivation.
@Ericson2314 @FRidh @peti I've opened #50233, that should hopefully handle all the remaining concerns. |
@Ekleog thanks. Please do revert this change and add the revert of the revert to your new change. That way we can review the whole thing again. |
It has already been reverted by @peti, so there is as much time as need be :) (well, to be more precise the Unless you mean reverting the whole PR and doing it again in the fixing PR? I don't think I can manage a clean reviewable history with that, compared to what I have currently made: it'd mean the three atomic “drop meta.needsVMSupport” (+3-19), “rename into passthru.tests” (+20-14, mostly comments) and “use the newly-extracted all-tests.nix” (+5-17) commits would get squashed into a jumbo “add meta.tests” that'd include this PR (+98-4), but I can do if you think it'd be easier for you to review this way. |
Rationale
Currently, tests are hard to discover. For instance, someone updating
dovecot
might not notice that the interaction ofdovecot
withopensmtpd
is handled in theopensmtpd.nix
test.And even for someone updating
opensmtpd
, it requires manual work to gocheck in
nixos/tests
whether there is actually a test, especiallygiven not so many packages in
nixpkgs
have tests and this is thus mostof the time useless.
Finally, for the reviewer, it is much easier to check that the “Tested
via one or more NixOS test(s)” has been checked if the file modified
already includes the list of relevant tests.
Implementation
Currently, this commit only adds the metadata in the package. Each
element of the
meta.tests
attribute is a derivation, that, when itbuilds successfully, means the test has passed (ie. following the same
convention as NixOS tests).
Future Work
In the future, the tools could be made aware of this
meta.tests
attribute, and for instance a
--with-tests
could be added tonix-build
so that it also builds all the tests. Or a--without-tests
to build without all the tests. @Profpatsch described in his NixCon talk
such systems.
Another thing that would help in the future would be the possibility to
reasonably easily have cross-derivation nix tests without the whole
NixOS VM stack. @7c6f434c already proposed such a system.
This RFC currently handles none of these concerns. Only the addition of
meta.tests
as metadata to be used by maintainers to remember to runrelevant tests.
Still to do before merge
../../..
(likely requires a refactoring ofrelease.nix
to put it inside the fix-point)meta.tests
checker like @Profpatsch's code