Fix meta.tests system #50233
Fix meta.tests system #50233
Conversation
This way, the package set will be possible to pass without re-importing all the time
This will make the list much easier to re-use, eg. for `nixosTests` The drawback is that this approaches makes the ``` nix-build release.nix -A tests.opensmtpd.x86_64-linux ``` command about twice as slow (3s to 6s): it now has to evaluate `nixpkgs` once for each architecture, instead of just having the hardcoded list of tests that allowed to say “ok just evaluate for x86_64-linux”. On the other hand, complete evaluation of `release.nix` should be much faster because we no longer import `nixpkgs` for each test: testing with the following command went from 30s to 18s, and that's just for a few tests. ``` time nix-instantiate --eval --strict nixos/release.nix -A tests.nat ``` I initially wanted to test on the whole `release.nix`, but there are too many broken tests and it takes too long to eval them all, especially compared to the fact that the current implementation breaks some setup. Given developers can just `nix-build nixos/tests/my-test.nix`, it sounds like an overall win.
Its job is already handled by `requiredSystemFeatures`
Nix currently rejects derivations in `meta` values. This works around that limitation by using `passthru` instead. Closes NixOS#50230
Ekleog
requested review from
Ericson2314,
infinisil,
matthewbauer and
thoughtpolice
as code owners
GrahamcOfBorg
added
6.topic: nixos
6.topic: stdenv
Ekleog
force-pushed
the
fix-test-system
branch
from
9ca74e5
to
6aedf6a
Compare
|
Any reason to do this all in one PR? I think we could merge the relatively urgent |
|
@timokau The Then I must say I'm currently basically thinking “well, this already 3 months, can wait 1 more”, though I'm aware this will merge-conflict as soon as someone adds a test. Anyway, I can split the “drop meta.needsVMSupport” and “rename to passthru.tests” commits in a separate PR if you'd rather, I was just thinking it'd be better to have nothing than a broken setup, in part because it encourages people to actually review it instead of thinking something along the lines of “well, it works good enough, there are more pressing PRs” :) BTW let's check whether it actually works with ofborg. @GrahamcOfBorg build opensmtpd.tests |
|
Success on x86_64-linux (full log) Attempted: opensmtpd.tests Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: opensmtpd.tests Partial log (click to expand)
|
Sure, whatever way you think works best. I know it can be very frustrating when you spend time improving something and then it goes stale forever. Bonus stress points if there are issues after merge. That happens and is hard to avoid without better tooling. I appreciate your efforts to improve the testing infrastructure :) |
|
I understand the impatience, and can't see how anyone wouldn't think this is at least a step in the right direction, so I'll merge. |
|
\o/ Thank you! :D |
This currently stops ofborg from testing tests, and I would hazard a guess it will also fail on Hydra. |
|
A first attempt at figuring out this is at #50186 (comment) |
The current behavior, breaking restrict-eval=true, had been introduced in 209f8b1. See also [1]. This change uses a hack to identify whether we're evaluating in restrict-eval, keeping the optimization when not and reverting to the old behavior when evaluating in restrict-eval. This should fix the issues that appeared at [2] and [3], that followed the change making us actually use this `nixpkgs` argument for tests since 83b27f6 [4]. CC: @roberth [1] NixOS@209f8b1acd4c#commitcomment-28419462 [2] NixOS#50233 (comment) [3] NixOS#50186 [4] NixOS#50233
|
nixops breakage documented here: #50419 |
This will be easier reviewed commit-by-commit, I tried to add in useful commit messages.
I think this should handle the concerns raised at #44439 and #50230.
For those who want to read only the commit messages, here they are:
tests: refactor to carry the package set as an argument
This way, the package set will be possible to pass without re-importing
all the time
tests: split into a separate
all-tests.nixfileThis will make the list much easier to re-use, eg. for
nixosTestsThe drawback is that this approaches makes the
command about twice as slow (3s to 6s): it now has to evaluate
nixpkgsonce for each architecture, instead of just having the hardcoded list of
tests that allowed to say “ok just evaluate for x86_64-linux”.
On the other hand, complete evaluation of
release.nixshould be muchfaster because we no longer import
nixpkgsfor each test: testing withthe following command went from 30s to 18s, and that's just for a few
tests.
I initially wanted to test on the whole
release.nix, but there are toomany broken tests and it takes too long to eval them all, especially
compared to the fact that the current implementation breaks some setup.
Given developers can just
nix-build nixos/tests/my-test.nix, it soundslike an overall win.
tests: disable some broken tests and/or restrict to x86_64
meta.tests: drop
meta.needsVMSupportIts job is already handled by
requiredSystemFeaturesmeta.tests: rename into passthru.tests
Nix currently rejects derivations in
metavalues. This works aroundthat limitation by using
passthruinstead.Closes #50230
nixosTests: use the newly-extracted all-tests.nix