nixpkgs-docs: when to prefer passthru.tests over installCheckPhase #119731

raboof · 2021-04-17T17:05:16Z

Motivation for this change

Make it easier to add tests by documenting better how.

Things done

lukegb · 2021-04-17T17:33:36Z

doc/stdenv/stdenv.chapter.md

+Because this adds overhead to every build, it is usually recommended to add the test to
+<xref linkend="var-meta-tests"/> instead.


My 2c is that I think installCheck is primarily useful for applications that have test-suites in tree (that maybe rely on build system configuration artifacts), that aren't then shipped as part of the package build.

If you as a package maintainer are defining a test, then it probably does make sense to add it as a separate passthru.tests test.

that's a great distinction, updated to make that more clear

nixos-discourse · 2021-04-18T08:37:44Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/contributor-retention/12412/74

danieldk

Thanks for improving the passthru.tests documentation. I only have a comment about the example.

danieldk · 2021-04-18T09:06:30Z

doc/stdenv/meta.chapter.md

+{ /* ... */ }:
+let
+  key = mkDerivation {
+    # ...
+    passthru.tests = {
+      version-check = runCommand "key-help" {} ''
+        ${key}/bin/KeY --help | grep -Fw ${version}
+        touch $out
+      '';
+    }
+  }
+};
+in key


I am not sure if we should present the example like this, we might end up with a lot of people submitting PRs with ugly let ... in ... wrapped derivations, which do not look very canonical. I think it's really much cleaner to just callPackage a separate derivation file with tests. Or if it must really be inline pass pkgs as an argument (both work because pkgs is a fixed-point).

Also, running a command seems quite lightweight. So, if it is not checked by the regular tests, I think it is easier to just add them to the postInstallCheck phase. I think passthru.tests is particularly useful for cases where you really want to test use as a separate derivation, e.g. checking whether you can compile against a library and the library works as expected.

ugly let ... in ... wrapped derivations, which do not look very canonical

I agree the let ... in ... is rather awkward (I saw it in another passthru).

I think it's really much cleaner to just callPackage a separate derivation file with tests. Or if it must really be inline pass pkgs as an argument (both work because pkgs is a fixed-point).

Using callPackage (or passing pkgs - or perhaps even the package itself?) looks neater, but is a bit dangerous: if you add any overrides to the package, the tests exposed by the package-with-overrides will still test the package-without-overrides. If that's a limitation we're happy to live with then that works for me ;).

So perhaps the guidance we should document is: if it's a check that is expected to be quick and can be easily expressed inline, add it to the installCheckPhase, otherwise move it to its own file and add it to passthru.tests?

I'd personally kind of prefer having the let ... in ... trick, even though it's not yet canonical, for the reason mentioned by @raboof of actually testing what we think we're testing. As for embedding wrapped derivations inline, if people think it looks bad (which makes sense) one solution would be to have a separate file that looks like:

# .../tests.nix { runCommand }: key: version: { version-check = runCommand "key-help" {} '' ${key}/bin/KeY --help | grep -Fw ${version} touch $out ''; }

Which would then be called with:

{ /* ... */ }: let key = mkDerivation { # ... passthru.tests = callPackage ./tests.nix {} key version; }; in key

I agree that not running tests for overrides is unexpected. But let self = ...; in self always felt like accidental complexity that makes derivations much harder to read unless you have in-dept Nix knowledge.

I am wondering if there isn't a more elegant solution. E.g. allowing tests to be self-taking functions and having mkDerivation pass self?

I had that in mind at some point too, but never actually got to it, mostly because anything that touches mkDerivation itself I'm trying not to touch — but also because passthru is expected to be passed through, so if mkDerivation started changing that it would technically be a breaking change.

Overall, I'd be all for such a change! But I don't have the time or energy to see it through myself these days, so… :/

As for the current PR, given that the manual that will be online for the next 6 months is currently being defined (and it's too late to land changes to mkDerivation), I'd think that having this non-elegant workaround in the manual would make sense so people at least know to make tests, and hopefully by the time the next release freezes we will have introduced the necessary mkDerivation machinery?

As for the current PR, given that the manual that will be online for the next 6 months is currently being defined (and it's too late to land changes to mkDerivation), I'd think that having this non-elegant workaround in the manual would make sense so people at least know to make tests, and hopefully by the time the next release freezes we will have introduced the necessary mkDerivation machinery?

Sounds fair 👍

I am wondering if there isn't a more elegant solution. E.g. allowing tests to be self-taking functions and having mkDerivation pass self?

With this in mind I think I'd like to show the approach without let, since that'll likely be easier to refactor to the new approach later.

FWIW, I'm not sure we'll actually manage to properly do the self-taking approach in a reasonable timeframe (I'm pretty sure I personally won't do it anytime soon, and don't know whether other people are planning to); and there may be issues that make it harder than expected (eg. programs that are split between a common.nix and vX-Y.nix, or files that are being callPackage'd multiple times from the top-level with various overrides).

So I wouldn't be too optimistic and would assume that it never actually happens in practice for the documentation we write today, though I'd love to be proven wrong 😅

So I wouldn't be too optimistic and would assume that it never actually happens in practice for the documentation we write today

I agree in general - here it just seemed like a nice tie-breaker ;)

This does the trick: #119942

and it's too late to land changes to mkDerivation

I would argue that this is an addition, not a change. It also causes no rebuilds and it's easy to revert.

zowoq · 2021-04-18T09:14:03Z

NixOS/ofborg#524

ofborg eval doesn't check passthru, we probably want to resolve that first before we recommend it in the manual.

raboof · 2021-04-18T09:42:41Z

NixOS/ofborg#524

ofborg eval doesn't check passthru, we probably want to resolve that first before we recommend it in the manual.

we should definitely double-check that - my impression is that ofborg will run the tests for derivations that it builds. That issue seems to say ofbord usually checks derivations it doesn't build at least evaluate correctly, but that that doesn't work for passthru derivations. Not sure though.

raboof · 2021-04-18T10:22:07Z

ofborg eval doesn't check passthru

we should definitely double-check that

It does run it for packages it builds, see for example #119781 . That seems sufficient to me, WDYT?

zowoq · 2021-04-18T10:33:28Z

... ofborg will run the tests for derivations that it builds. That issue seems to say ofbord usually checks derivations it doesn't build at least evaluate correctly, but that that doesn't work for passthru derivations.

Yes.

It's easy it overlook and people would assume a ✅ from ofborg means that it's okay and nobody notices until it fails on Hydra.

Ekleog

Looks great to me, thank you! Just have two small comments below :)

doc/stdenv/meta.chapter.md

abathur

Just some phrasing tweaks; mostly:

trimming syllables/words/clauses/sentences
more active phrasing

doc/stdenv/meta.chapter.md

doc/stdenv/stdenv.chapter.md

doc/stdenv/meta.chapter.md

abathur · 2021-04-18T23:56:04Z

On discourse I said:

the nixpkgs manual ... is thin on how you’d write a test from scratch or what would be meaningful.

This PR is already a well-scoped stepwise improvement that bites off a big part of how you'd write a test from scratch, so I decided to sketch out how I think about what would be meaningful in case it helps clarify whether anything is missing or should be shaded differently? I don't want to force a broader scope or gum the PR up, so I'm posting it in the discourse thread: https://discourse.nixos.org/t/contributor-retention/12412/75

The main place I think it intersects with the how-to documentation already in this PR:

Without a resource covering what to test, I imagine some readers will see the help/version example here as a straightforward example of a test that is good/desirable or at least acceptable. It might be good to drop some breadcrumbs about where it sits on the various scales (Is it just a trivial example that was once acceptable but would get laughed out of PR review these days? Is it the minimum-meaningful test, or all we should aspire until everything is at this level?)
- If it is trivial, saying so is probably enough. If it is meaningful, a quick high-level note on ~why it's meaningful might be a good stop-gap for orienting people towards appropriate values?

doc/stdenv/meta.chapter.md

nixos-discourse · 2021-04-19T16:18:06Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/contributor-retention/12412/80

abathur

LGTM.

I highlighted something I mentioned earlier and didn't see a comment on just in case you know the answer, but feel free to ignore it if not.

abathur · 2021-04-20T18:28:03Z

doc/stdenv/meta.chapter.md

+#### Regular tests
+
+Tests that are part of the source package are often executed in the `installCheckPhase`. Prefer `passthru.tests` for tests that are introduced in nixpkgs because `installCheckPhase` adds overhead to each build and we can run `passthru.tests` independently.
+
+```nix
+{ /* ... */, key }:
+
+mkDerivation rec {
+  # ...
+  passthru.tests = {
+    version-check = runCommand "key-help" {} ''
+      ${key}/bin/KeY --help | grep -Fw ${version}
+      touch $out
+    '';
+  };
+}
+```
+


I mentioned this out in the main thread, but I guess I'll attach it here since you pinged me :)

Without a resource covering what to test, I imagine some readers will see the help/version example here as a straightforward example of a test that is good/desirable or at least acceptable. It might be good to drop some breadcrumbs about where it sits on the various scales (Is it just a trivial example that was once acceptable but would get laughed out of PR review these days? Is it the minimum-meaningful test, or all we should aspire until everything is at this level?)

If it is trivial, saying so is probably enough. If it is meaningful, a quick high-level note on ~why it's meaningful might be a good stop-gap for orienting people towards appropriate values?

I think it would be good to have a little context on this before or after the example, but I don't see it as a blocker and realize you might not know the answer.

but I don't see it as a blocker and realize you might not know the answer.

Correct, I don't know if we have enough experience/consensus to be able to put into words authoritative advice on this yet, so I left it out for now.

doc/stdenv/meta.chapter.md

davidak · 2021-04-24T18:36:15Z

This is great improvement of the reference in the manual.

I finally came to document the best practices of how to create package test today: #120534

When that is merged, you should just add a minimal example that runs tests.nix and update the terms accordingly "package tests" instead of "Regular tests".

SuperSandro2000 · 2021-04-26T00:06:25Z

doc/stdenv/meta.chapter.md

+* `installCheckPhase` adds overhead to each build
+
+```nix
+{ /* ... */, key }:


Suggested change

{ /* ... */, key }:

{ ..., key }:

I disagree. /* ... */ is clearly intended as meta-syntax, whereas ... is actual syntax.

@SuperSandro2000 can you clarify why you wanted /* ... */ to be replaced by ..., so we can perhaps find a third alternative that is better than the 2 competing ones here?

(I moved this example to doc/contributing/coding-conventions.chapter.md and changed this to … (UTF-8 ellipsis) to be consistent with the other already-existing example there)

SuperSandro2000 · 2021-04-26T00:06:37Z

doc/stdenv/meta.chapter.md

+{ /* ... */, key }:
+
+mkDerivation rec {
+  # ...


Suggested change

# ...

...

I disagree. # ... is clearly intended as meta-syntax, whereas ... is close to actual syntax. It's a valid token in the language.

@SuperSandro2000 can you clarify why you wanted # ... to be replaced by ..., so we can perhaps find a third alternative that is better than the 2 competing ones here?

(I moved this example to doc/contributing/coding-conventions.chapter.md and changed this to … (UTF-8 ellipsis) to be consistent with the other already-existing example there)

And mention you can have either lightweight 'package' or more heavyweight 'NixOS' (module) tests. This was suggested at nix-community/nixpkgs-update#260 (comment) and discussed further at NixOS#119731

nixos-discourse · 2024-05-21T21:04:54Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/improve-the-jdk-infrastructure-on-nixpkgs/45678/20

nixos-discourse · 2024-05-27T14:27:28Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/improve-the-jdk-infrastructure-on-nixpkgs/45678/33

github-actions bot added the 8.has: documentation This PR adds or changes documentation label Apr 17, 2021

raboof mentioned this pull request Apr 17, 2021

Are binary checks the responsibility of nixpkgs-update? nix-community/nixpkgs-update#260

Closed

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 17, 2021

lukegb reviewed Apr 17, 2021

View reviewed changes

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from fb4f37d to 5f39e96 Compare April 18, 2021 07:45

raboof requested a review from lukegb April 18, 2021 07:47

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 5f39e96 to 467f558 Compare April 18, 2021 07:52

danieldk reviewed Apr 18, 2021

View reviewed changes

raboof marked this pull request as draft April 18, 2021 09:42

raboof mentioned this pull request Apr 18, 2021

seaweedfs: add version check to passthru.tests #119636

Merged

10 tasks

Ekleog approved these changes Apr 18, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

abathur reviewed Apr 18, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

abathur reviewed Apr 18, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

fricklerhandwerk reviewed Apr 19, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

fricklerhandwerk reviewed Apr 19, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 467f558 to 519f9e7 Compare April 19, 2021 20:14

raboof marked this pull request as ready for review April 19, 2021 20:17

raboof requested review from danieldk and abathur April 20, 2021 06:46

abathur approved these changes Apr 20, 2021

View reviewed changes

danieldk suggested changes Apr 21, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 519f9e7 to 82a912d Compare April 21, 2021 08:37

raboof requested a review from danieldk April 21, 2021 08:37

danieldk approved these changes Apr 21, 2021

View reviewed changes

roberth mentioned this pull request Apr 21, 2021

stdenv.mkDerivation: overlay style overridable recursive attributes #119942

Merged

10 tasks

roberth reviewed Apr 21, 2021

View reviewed changes

doc/stdenv/meta.chapter.md Show resolved Hide resolved

roberth mentioned this pull request Apr 24, 2021

doc: add instructions for creating package tests #120534

Merged

10 tasks

SuperSandro2000 reviewed Apr 26, 2021

View reviewed changes

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 82a912d to 7cd0ff5 Compare April 26, 2021 06:44

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 7cd0ff5 to 306ea76 Compare May 6, 2021 11:11

raboof requested review from roberth and SuperSandro2000 May 6, 2021 11:13

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 306ea76 to 0518c62 Compare May 6, 2021 11:14

raboof changed the title ~~nixpkgs-docs: prefer passthru.tests over installCheckPhase~~ nixpkgs-docs: when to prefer passthru.tests over installCheckPhase Aug 14, 2021

nixpkgs-docs: when to prefer passthru.tests over installCheckPhase

d09e0be

And mention you can have either lightweight 'package' or more heavyweight 'NixOS' (module) tests. This was suggested at nix-community/nixpkgs-update#260 (comment) and discussed further at NixOS#119731

raboof force-pushed the document-preferring-passthru-tests-over-installChecks branch from 0518c62 to d09e0be Compare August 14, 2021 07:47

raboof merged commit c580b69 into NixOS:master Aug 14, 2021

lilyinstarlight mentioned this pull request Nov 10, 2023

freecad: switch back to isolated config with env vars turned on #266498

Merged

13 tasks

doronbehar mentioned this pull request May 29, 2024

doc/meta: Mention --version as a good usecase for installCheckPhase #315616

Merged

13 tasks

		Because this adds overhead to every build, it is usually recommended to add the test to
		<xref linkend="var-meta-tests"/> instead.

nixpkgs-docs: when to prefer passthru.tests over installCheckPhase #119731

nixpkgs-docs: when to prefer passthru.tests over installCheckPhase #119731

Conversation

raboof commented Apr 17, 2021

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented Apr 18, 2021

danieldk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raboof Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

Ekleog Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

danieldk Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zowoq commented Apr 18, 2021

raboof commented Apr 18, 2021

raboof commented Apr 18, 2021

zowoq commented Apr 18, 2021

Ekleog left a comment

Choose a reason for hiding this comment

abathur left a comment

Choose a reason for hiding this comment

abathur commented Apr 18, 2021

nixos-discourse commented Apr 19, 2021

abathur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidak commented Apr 24, 2021

SuperSandro2000 Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixos-discourse commented May 21, 2024

nixos-discourse commented May 27, 2024

raboof Apr 18, 2021 •

edited

Loading

Ekleog Apr 18, 2021 •

edited

Loading

danieldk Apr 18, 2021 •

edited

Loading

SuperSandro2000 Apr 26, 2021 •

edited

Loading