Nixpkgs: pkgs.nixosTests doesn't make sense

[roberth]

Issue description

While convenient, pkgs.nixosTests has surprising behavior. For example, this should fail, but it doesn't:

(import ./. { overlays = [ (self: super: { nginxStable = null; }) ]; }).nixosTests.nginx

So what is wrong here, is that the NixOS tests do not use the pkgs that they are passed. The NixOS tests assume that they can call Nixpkgs however they want. Although not a requirement for all tests, this means that in general the tests must reinvoke Nixpkgs, thus ignoring the Nixpkgs where .nixosTest was defined.

The documentation doesn't explain this ("Push NixOS tests inside the fixed point", which is misleading at best) and the implementation of nixosTests looks just fine, passing pkgs along as one would expect when reusing Nixpkgs for some tests. However, that is not what happens. pkgs will only be used to get qemu, lib, probably perl and perhaps some other things.

Why this is a problem: someone will use it to test whether NixOS, as built with their company overlay, still works. They will get false positives.

Steps to reproduce

nix-build --expr '(import ./. { overlays = [ (self: super: { nginxStable = null; }) ]; }).nixosTests.nginx'

Watch it succeed. It shouldn't.

Solutions

Ideally, we would pass pkgs to the NixOS machines. This can be done for some tests, but not for all the NixOS tests, because many depend on config.nixpkgs.config, which is unsupported by config.nixpkgs.pkgs (which is how you inject an existing Nixpkgs invocation).

So I don't see a way to make this work as one might predict. I suggest deleting pkgs.nixosTests.

@Ekleog do we really need this? It seems like just a convenience attribute.

[Ekleog]

@roberth This, theoretically, should have been solved in the refactoring at https://github.com/NixOS/nixpkgs/pull/50233. It looks like it didn't actually.

The issue is that it is kind of required for usage in meta.tests (see this comment).

I must say I am not sure what config.nixpkgs.config exactly does, but there is at least a place where the nginx module still has an import to nixpkgs that I missed in the above refactoring. I'll try to have a look at how and why it's there!

[roberth]

This is where NixOS gets its Nixpkgs, unless config.nixpkgs.pkgs is set. That's the code path for how everyone has been building NixOS, although that can change (but probably hasn't) since about a month ago. So that doesn't seem to be something we can change.

I've had another look at the actual tests that would break if we set config.nixpkgs.pkgs. We seem to have the following situations:

1. config.nixpkgs.config.packageOverrides. These can be replaced by overlays since #49256.
2. config.nixpkgs.localSystem. This is needs to be taken from pkgs anyway. See #49765.
3. config.nixpkgs.allowUnfree and config.nixpkgs.allowUnfreePredicate. I'm not sure whether doing this on hydra.nixos.org is even ok with policy. This only applies to quake3.nix and virtualbox.nix.

(1) and (2) can be fixed easily. (3) is a bit trickier. What I'd like to do is make sure that config.nixpkgs.config is not set when config.nixpkgs.pkgs is set, by means of assertions. This will break the (3) tests, but only when invoked via Nixpkgs instead of via NixOS, so that seems ok to me. I don't suppose hydra will build somepkgs.tests through Nixpkgs, but only builds the tests when building NixOS. Is that correct? If not, we can make an exception for quake3 and virtualbox.

Regarding the implementation, we can:

a. Make meta.tests return only paths to tests. This puts the burden on the users, which solves the 'leaky abstraction' by removing the leakiness. Doesn't seem necessary anymore, but is an option.
b. Make meta.tests use pkgs.nixosTest. This should reduce some duplication. If meta.tests requirements turn out to differ from user-side nixosTest requirements, we should duplicate the function such that meta.tests uses the new variation.
c. Make meta.tests set config.nixpkgs.pkgs by itself, right away, not using pkgs.nixosTest.

I'm in favor of (b).

---

Appendix: grep 'nixpkgs\.' in nixos/tests

ceph.nix:      nixpkgs.config.packageOverrides = super: {
common/letsencrypt/common.nix:  nixpkgs.overlays = lib.singleton (self: super: {
containers-imperative.nix:          inherit (config.nixpkgs.localSystem) system;
hocker-fetchdocker/machine.nix:{ nixpkgs.config.packageOverrides = pkgs': {
quake3.nix:      nixpkgs.config.packageOverrides = overrides;
quake3.nix:      nixpkgs.config.allowUnfreePredicate = unfreePredicate;
quake3.nix:          nixpkgs.config.packageOverrides = overrides;
quake3.nix:          nixpkgs.config.allowUnfreePredicate = unfreePredicate;
radicale.nix:        nixpkgs.overlays = [
subversion.nix:          nixpkgs.config.packageOverrides = overrides;
subversion.nix:          nixpkgs.config.packageOverrides = overrides;
virtualbox.nix:      nixpkgs.config.allowUnfree = useExtensionPack;

[Ekleog]

Thank you for your investigation!

allowUnfree in tests

For (3), I'd think neither hydra nor ofborg should build them. Currently we're kind of OK given none of them defaults to building them, but it does mean that it's currently possible to trigger some ofborg builds for unfree stuff by relaying through NixOS tests (cc @LnL7 in the absence of Graham, even though I don't really think it's that urgent an issue as ofborg never redistributes build products anyway).

That said, this question raises the proper question of what license these tests should be considered as having. Maybe it'd make sense to have them set meta.license to unfree in this case? This would both allow them to stay in meta.tests for easy human test, and avoid that either ofborg or hydra build them.

Now, if we want to do that, it will mean having a way to override nixpkgs with a custom allowUnfreePredicate before passing it to the test. I would hope it wouldn't be that hard to do, but… nixpkgs always finds a way to surprise me in this domain.

Fixing tests

I must say I'm not sure exactly what your point (b) entails. If I understand correctly, it is basically a replacement for make-test.nix, right? As such, it would require fixing all tests. This sounds like the best path forward to me too.

Ideally it would even handle the allowUnfree case as proposed above by propagating a non-false allowUnfree to setting meta.license to unfree in the “test” derivation.

Now, do I read correctly that this proposal is mostly changes to the tests themselves, and that it would fix all-tests.nix by making it actually respect its expected interface, ie. use its pkgs argument as the package set to use instead of relying on re-importing it?

nixosTests

If my analysis above is correct, I think it's compatible with nixosTests, which is basically just importing said all-tests.nix with the outer pkgs so that packages could easily depend on it for setting their meta.tests. As you didn't mention nixosTests in your latest message I'm not sure what your ideas for it currently are, sorry if that's already what you meant :)

[roberth]

allowUnfree in tests

So we can disable the 'unfree' tests, with the method of disabling depending on the outcome of your question what license these tests should be considered as having.
To run those tests manually, the user can simply pass allowUnfree into the Nixpkgs invocation that produces the pkgs in pkgs.nixosTests.
So (3) seems can be solved.

Fixing tests

The solutions for fixing the actual machines in the tests are provided by (1), (2) and the paragraph above.

Now, do I read correctly that this proposal is mostly changes to the tests themselves, and that it would fix all-tests.nix by making it actually respect its expected interface, ie. use its pkgs argument as the package set to use instead of relying on re-importing it?

The test definitions themselves (such as tests/nginx.nix) need to be changed slightly to allow injecting the test runner instead of always calling make-test.nix. make-test.nix will be the default, so you can just nix-build these tests.
I don't expect any problems replacing make-test.nix by pkgs.nixosTest after fixing the machines, but I haven't tried it yet.

nixosTests

Your analysis seems correct and what you suggest is roughly what needs to be done in all-tests.nix and related files.

Plans

I'm a little busy, but I'll try to make a PR for this soon.

If you feel like improving the "top-level" Nixpkgs and NixOS interfaces, have a look at #49765 in the meantime ;)

Future use in NixOS jobs?

(Skip this for now)
In theory, NixOS could reuse pkgs in pkgs.nixosTest to achieve an evaluation speedup, but I'd rather try not to change the way NixOS calls all-tests.nix for now. I also think we should have some tests that exercise the 'normal' modules/misc/nixpkgs.nix functionality, so perhaps all-tests.nix should then have access to two runners: an efficient one and a 'standalone' one.

[LnL7]

I didn't even know this was a thing, and I think nix-build nixos/tests/virtualbox.nix --arg enableUnfree true makes much more sense than passing an already imported package set. This way each test can define what parameters are available and what actually makes sense.

As for ofborg, we could provide builds and tests for unfree software since no caches are redistributed. However there are some ideas on doing that for debugging purposes so for now it's best not to diverge too much from hydra in terms of what we build and how we evaluate IMHO.

[Ekleog]

@roberth Sounds great, thank you!

About the way NixOS calls all-tests.nix, as I split all-tests.nix out of release.nix only a few days ago I think there's no better time than now to change it if it needs to :)
But I also think that if pkgs.nixosTest means that the tests will actually use their pkgs argument without re-importing nixpkgs, then the evaluation-time gains should come in immediately: the refactor of release.nix to all-tests.nix already included passing the same imported nixpkgs as the pkgs argument (see commit message https://github.com/NixOS/nixpkgs/commit/83b27f60ceff23967e477c90bef8e78cc96d50a2, though I was wrong in that it actually only went from including nixpkgs twice per test to including it once per test plus twice globally). So the gain should come for free! Leading to actually including nixpkgs twice globally, and not once more per test.

@LnL7

I think we totally agree that nix-build nixos/tests/virtualbox.nix --arg enableUnfree true makes the most sense for manually running the tests. The question is more for ofborg (which currently runs approx. nix-build nixos/release.nix -A tests.virtualbox), hydra (same thing but via release-{small,combined}.nix) and meta.tests, all of which “need to” take defaults and can't pass additional arguments.

For all these, re-using the already-existing nixpkgs would be a gain both in purity (nixpkgs importing does quite some impure stuff, and even though restrict-eval prevents most or all of it most users won't run local builds as restrict-eval, so it impacts meta.tests) and evaluation time (importing nixpkgs was ~3s on my computer last I checked, multiplied by the number of tests that need to be run).

But this doesn't prevent keeping the dual-interface all-tests.nix / direct nix-build, with tests taking in either a pkgs argument or fetching it themselves with the arguments they get provided :)

[LnL7]

I'm not familiar with how it works now. But not having the module system import nixpkgs means what you're testing is not what actually happens, so what are you testing at that point? That said I do agree evaluation time is important, given how expensive this has become over time.

[Ekleog]

So the idea is: you need to test what actually happens, but with which nixpkgs?

The objective is:

• When you invoke nix-build nixos/tests/foo.nix, then it'll default to importing ../...
• When you invoke nix-build nixos/release.nix -A tests.foo, then it'll default to, in nixos/release.nix, importing ./.. and using it in tests.foo (and using the same import for all tests if you -A tests, even though that's likely to fail because lots of tests are broken).
• When you invoke nix-build -E '(import <nixpkgs> { overlays = [ ./overlay.nix ]; }).foo.tests, it'll defer to nixosTests.foo (from the same nixpkgs), which will call the test with the same overlay'd nixpkgs.

Before https://github.com/NixOS/nixpkgs/commit/83b27f60ceff23967e477c90bef8e78cc96d50a2 there were two imports of nixpkgs per test, independently from how they were invoked: one for the tools used by make-test.nix and one for the test itself. After that commit, there's only the nixpkgs import used by the test itself remaining. Once this issue will be fixed, there should remain no nixpkgs import, except if no pkgs argument is passed to the test in which case it'll default to importing ../...

Does that sound good to you? :)

[matthewbauer]

Why do we even have nixosTests to begin with? It seems like it violates the separation between nixos and nixpkgs. If you want to have a reference from nixpkgs to the nixos test, why not just use a string? So you could do:

meta.tests = [ "nixos/tests/acme.nix" ];

The current architecture is misguided IMO. Nixpkgs shouldn't import NixOS at all.

[Ekleog]

@matthewbauer I totally agree with you that Nixpkgs shouldn't need to import NixOS at all. But it's all we have until something like https://github.com/NixOS/nixpkgs/pull/36842 lands, to be able to have at least a way to link from packages to some tests that show they're non-broken.

The end-goal of passthru.tests is to have ofborg automatically run them, and using strings for doing that would be a complete non-starter: passthru.tests is defined as a set of derivations that have as semantics that the test passes iff the derivation builds. These semantics are unrelated to nixosTests and I'm pretty sure we definitely do not want these to be replaced by a list of strings that need to be imported instead, among other things because we may want to say “to test gcc, I want to build coreutils, firefox and emacs” (to check a wide range of hacks that may not be supported by more recent gcc). We wouldn't want to give string paths to these derivations, would we? Some derivations don't even have files for themselves but a file that only defines a function that's later called in all-packages.nix.

(Note: you can see an example of passthru.tests being used without nixosTests in pkgs/development/tools/build-managers/bazel/default.nix)

And then nixosTests comes into play, because we just don't have any better way of specifying integration tests for packages for the time being: we need a way to propagate the few tests we actually have to nixpkgs so that they're automatically run by the passthru.tests infrastructure.

Also, BTW, your example doesn't tie Nixpkgs less to NixOS: nix is a lazy language, and a string that refers to a path is not particularly less bad that an import of that path, so long as the import is not used… and if it's used, then the string would have been imported anyway, so it's just as much of an import.

TL;DR: I totally agree we shouldn't need to import nixos into nixpkgs, but it's the least bad option we've got to be able to automatically run tests for the time being.

[roberth]

Closing in favor of #55798