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

[RFC] Use meta.tests to link from packages to the tests that test them #44439

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Ekleog
Copy link
Member

@Ekleog Ekleog commented Aug 4, 2018

Rationale

Currently, tests are hard to discover. For instance, someone updating
dovecot might not notice that the interaction of dovecot with
opensmtpd is handled in the opensmtpd.nix test.

And even for someone updating opensmtpd, it requires manual work to go
check in nixos/tests whether there is actually a test, especially
given not so many packages in nixpkgs have tests and this is thus most
of 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 it
builds 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 to
nix-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 run
relevant tests.

Still to do before merge

  • Use attribute paths instead of raw imports with ../../.. (likely requires a refactoring of release.nix to put it inside the fix-point)
  • Add documentation
  • Add metadata in the meta.tests checker like @Profpatsch's code

@samueldr
Copy link
Member

samueldr commented Aug 4, 2018

Any trick in mind to make it more palatable than a path full of ../ that will invariably change depending on the depth of the derivation? The tests are already being put in an attrset for release, maybe it could be reused or moved into a more reusable place?

@Ekleog
Copy link
Member Author

Ekleog commented Aug 4, 2018

Hmm… From what I understand, it'd require a refactoring from release.nix, as that's not readily available from the fixed point? If so, maybe doing it once the overall design is more or less consensual would be better, in order for the refactoring not to hide the important parts during discussion :) Thank you for the idea!

@7c6f434c
Copy link
Member

7c6f434c commented Aug 4, 2018

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 {})
Copy link
Member

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.

Copy link
Member Author

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?

@aszlig
Copy link
Member

aszlig commented Aug 4, 2018

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 ../../..., maybe use a list of attribute paths instead?

@aszlig
Copy link
Member

aszlig commented Aug 4, 2018

Using attribute paths btw. also has the benefit that nix-instantiate --eval --strict '<nixpkgs>' -A dovecot.meta will work, IIRC something like this was (or is) used in Hydra (edit: hydra-eval-jobs.cc:37).

@7c6f434c
Copy link
Member

7c6f434c commented Aug 4, 2018

@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.

@Profpatsch
Copy link
Member

Profpatsch commented Aug 4, 2018

We (though I don’t remember who besides me) already had a type cobbled together that these tests should have:
Profpatsch@d74203a#diff-096a3f0fd04e519accb1cde6c483e72bR179

In plain words, tests should be an attribute set in meta that contains tests, where a test is a derivation that has a passthrough boolean field isVmTest (this way test runners are able to filter). That’s just a first idea of course.

@Ekleog
Copy link
Member Author

Ekleog commented Aug 4, 2018

@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 meta.tests is to be run before accepting a package update into nixpkgs, indeed, given unit tests are (I think?) supposed to be run in the checkPhase. The aim is mostly to allow a single test (like here the OpenSMTPD test) to serve for two derivations (here, opensmtpd and dovecot) :)

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 check-meta update (apart from the foo attribute, that I guess is a WIP test element?): isVmTest.

There are two types of test I can think of off the top of my head that could be used right now:

  1. NixOS tests
  2. Dependencies (eg. a library setting a light dependency as a smoke test to check it hasn't completely broken its API)

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:

  1. How long does it take to run the build? (the equivalent of your isVmTest, I guess, but test category 3 could also potentially take long)
  2. Should tests be recursive? (ie. should the tests of the test packages be executed? this could be useful for test category 2, eg. openssl could declare curl as a recursive-test, and curl would have a test that checks it against an https nginx -- this would smoke-test that the API of openssl appears not to have broken enough to break curl)

I'm not completely sure about case 2 (it could be done by directly including the test case for curl against nginx for instance), but I think anyway isVmTest could (should?) be renamed isSlowTest.

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 :)

@Profpatsch
Copy link
Member

Profpatsch commented Aug 4, 2018

What do you think about this? :)

I think we should basically have an open record of properties for tests.

let
  testProperties = productOpt {
    open = true;
    req = {
      # times mostly defined by test runners, comments are proposals
      # test runners might decide to cap after certain time
      runningTime = sum {
        short = unit; # under 1 minute
        medium = unit; # under 5 minutes
        long = unit; # under 15 minutes
        unbound = unit; # more than 15 minutes
        maxMinutes = int;
      };

      needsVMSupport = bool;
    };
  };
in testProperties

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 types-simple module to define these types, which is not in nixpkgs (yet))

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: dovecot, opensmtpd

Partial log (click to expand)

copying path '/nix/store/qksj8mx1cdvhhblqxshjzfri4yc3qrdb-libevent-2.1.8' from 'https://cache.nixos.org'...
copying path '/nix/store/ljs86xdssnqnqvpa6xnyhvkzfv6w4prg-libsodium-1.0.16' from 'https://cache.nixos.org'...
copying path '/nix/store/29j6kf19lxj81hyb6mbzqp42w9s47vx9-lz4-1.8.2' from 'https://cache.nixos.org'...
copying path '/nix/store/gd222hhrhwm2rjzd7c4vi55vfxrrg5zk-cyrus-sasl-2.1.26' from 'https://cache.nixos.org'...
copying path '/nix/store/v2hsgafk6wgrbgzrai05wnncwf9dwkzm-libtool-2.4.6-lib' from 'https://cache.nixos.org'...
copying path '/nix/store/wj214jamqiysbkw5dbvpgyvdnmlpkahr-openldap-2.4.45' from 'https://cache.nixos.org'...
copying path '/nix/store/vi9n4cszz9rqfmf3p5c8chvqv2pviywb-opensmtpd-6.0.3p1' from 'https://cache.nixos.org'...
copying path '/nix/store/8dyq4hg5p6c074kcxbrcl34miwg6n5bf-dovecot-2.3.2.1' from 'https://cache.nixos.org'...
/nix/store/8dyq4hg5p6c074kcxbrcl34miwg6n5bf-dovecot-2.3.2.1
/nix/store/vi9n4cszz9rqfmf3p5c8chvqv2pviywb-opensmtpd-6.0.3p1

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: dovecot, opensmtpd

Partial log (click to expand)

  /nix/store/0v05paacys9bnb9fxvvhwk8j5mqg34lw-dovecot-2.3.2.1
  /nix/store/5h9kpirrh5fw9imblwxm4d1lr8acwsk6-clucene-core-2.3.3.4
  /nix/store/llyvxfx789yrwbljy3pig9q7al64bg5k-libasr-1.0.2
  /nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1
copying path '/nix/store/5h9kpirrh5fw9imblwxm4d1lr8acwsk6-clucene-core-2.3.3.4' from 'https://cache.nixos.org'...
copying path '/nix/store/llyvxfx789yrwbljy3pig9q7al64bg5k-libasr-1.0.2' from 'https://cache.nixos.org'...
copying path '/nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1' from 'https://cache.nixos.org'...
copying path '/nix/store/0v05paacys9bnb9fxvvhwk8j5mqg34lw-dovecot-2.3.2.1' from 'https://cache.nixos.org'...
/nix/store/0v05paacys9bnb9fxvvhwk8j5mqg34lw-dovecot-2.3.2.1
/nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1

@Ekleog
Copy link
Member Author

Ekleog commented Aug 5, 2018

I think all the points that were requiring fixing are now fixed :)

@Profpatsch I have re-used the pre-existing timeout meta value for test timeouts, and put needsVMSupport in meta (from the idea that it may be useful for non-test derivations too). That said, the way I used of enforcing these two attributes are present in test derivations is currently pretty ugly… but until your types-simple gets merged I'm not sure it's possible to do much better :/

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: dovecot, opensmtpd

Partial log (click to expand)

/nix/store/8dyq4hg5p6c074kcxbrcl34miwg6n5bf-dovecot-2.3.2.1
/nix/store/vi9n4cszz9rqfmf3p5c8chvqv2pviywb-opensmtpd-6.0.3p1

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: dovecot, opensmtpd

Partial log (click to expand)

  /nix/store/0v05paacys9bnb9fxvvhwk8j5mqg34lw-dovecot-2.3.2.1
  /nix/store/5h9kpirrh5fw9imblwxm4d1lr8acwsk6-clucene-core-2.3.3.4
  /nix/store/llyvxfx789yrwbljy3pig9q7al64bg5k-libasr-1.0.2
  /nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1
copying path '/nix/store/5h9kpirrh5fw9imblwxm4d1lr8acwsk6-clucene-core-2.3.3.4' from 'https://cache.nixos.org'...
copying path '/nix/store/llyvxfx789yrwbljy3pig9q7al64bg5k-libasr-1.0.2' from 'https://cache.nixos.org'...
copying path '/nix/store/0v05paacys9bnb9fxvvhwk8j5mqg34lw-dovecot-2.3.2.1' from 'https://cache.nixos.org'...
copying path '/nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1' from 'https://cache.nixos.org'...
/nix/store/0v05paacys9bnb9fxvvhwk8j5mqg34lw-dovecot-2.3.2.1
/nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: dovecot, opensmtpd

Partial log (click to expand)

shrinking /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/sbin/dovecot
gzipping man pages under /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/share/man/
strip is /nix/store/1hi76hr87bd1y1q1qjk0lv8nmcjip1c8-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/lib  /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/libexec  /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/bin  /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/sbin
patching script interpreter paths in /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1
/nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/libexec/dovecot/decode2text.sh: interpreter directive changed from "/bin/sh" to "/nix/store/dkh7l9a4sx7zqh8riqbj3z21sz25p8xy-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1...
moving /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/sbin/* to /nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1/bin
/nix/store/zjmkciznnc2n4vvkpnq7zqvwv25iry41-dovecot-2.3.2.1
/nix/store/lmjpbp2vh6k6via7lyb92zxjfb3gpw7j-opensmtpd-6.0.3p1

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: dovecot, opensmtpd

Partial log (click to expand)

shrinking /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/bin/doveadm
gzipping man pages under /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/share/man/
strip is /nix/store/zrs21zqcchgyabjf4xfimncdq16njizc-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/lib  /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/libexec  /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/bin  /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/sbin
patching script interpreter paths in /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1
/nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/libexec/dovecot/decode2text.sh: interpreter directive changed from "/bin/sh" to "/nix/store/6vqgi9d49smsbr2qxra52yhipg0yxf9f-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1...
moving /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/sbin/* to /nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1/bin
/nix/store/z17hyaagddgxlpncxmqk6a70hn2kh27d-dovecot-2.3.2.1
/nix/store/vi9n4cszz9rqfmf3p5c8chvqv2pviywb-opensmtpd-6.0.3p1

Copy link
Member

@Profpatsch Profpatsch left a 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;
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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)

@7c6f434c
Copy link
Member

7c6f434c commented Nov 4, 2018

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.

@timokau timokau merged commit 6141939 into NixOS:master Nov 6, 2018
@Profpatsch
Copy link
Member

Profpatsch commented Nov 7, 2018

I wonder if we ever find a way to specify timeouts properly

It should be worth a look to see how bazel does it, and also other build tools that support tests.

@Ericson2314
Copy link
Member

Sorry I diddn't see this. builtins.currentSystem is very incorrect, both for cross and for hydra which evaluates different native builds.

@Ericson2314
Copy link
Member

It's not letting me comment inline for some reason, but nixpkgs = pkgs is also wrong. nixpkgs is supposed to be unimported nixpkgs, which release.nix will them import various ways. Really all relating to the NixOS tests needs some refactoring, both for correctness, and for performance in not evaluating nixpkgs too many times. The best thing to do is separate the tests from release.nix

@Ekleog
Copy link
Member Author

Ekleog commented Nov 9, 2018

@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?

@FRidh
Copy link
Member

FRidh commented Nov 11, 2018

Why is the evaluator complaining:

derivation 'dovecot-2.3.3' has invalid meta attribute 'tests'
derivation 'opensmtpd-6.4.0p2' has invalid meta attribute 'tests'

@Ekleog
Copy link
Member Author

Ekleog commented Nov 11, 2018

@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 nix-build command?

@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 'tests', and it should be taken care of by this part of the PR :/

@FRidh
Copy link
Member

FRidh commented Nov 11, 2018

@Ekleog run exec nix-env -f . -qaP --meta --json --drv-path --show-trace >/dev/null on master or staging-next.

@peti
Copy link
Member

peti commented Nov 11, 2018

unknown meta entries get reported as typos by various tools

Unknown meta attributes are reported as a warning by Nix itself. See #50230.

@grahamc
Copy link
Member

grahamc commented Nov 11, 2018

@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
Copy link
Member

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.

@Ekleog
Copy link
Member Author

Ekleog commented Nov 11, 2018

@Ericson2314 @FRidh @peti I've opened #50233, that should hopefully handle all the remaining concerns.

@FRidh
Copy link
Member

FRidh commented Nov 11, 2018

@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.

@Ekleog
Copy link
Member Author

Ekleog commented Nov 11, 2018

It has already been reverted by @peti, so there is as much time as need be :) (well, to be more precise the meta.tests lines have been commented out)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet