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

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.

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.

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)

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)