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

doc: add instructions for creating package tests #120534

Merged
merged 1 commit into from
May 1, 2021

Conversation

davidak
Copy link
Member

@davidak davidak commented Apr 24, 2021

Motivation for this change

This is a big milestone in the effort to add tests to packages that was started by @Profpatsch in 2017 (see NixCon talk ). With documentation, people can actually create tests that can be used to ensure quality and help reviews and automatic updates!

Closes #73076
Closes #27604
Closes #46632

This is not perfect, but better than not having it documented.

I would like to link to better examples, but we don't have them yet. That changes hopefully with this documentation.

This introduces the term "Nix package tests" in addition to "NixOS module tests", or "package tests", "nixos tests", "module tests" for short.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Apr 24, 2021
@ofborg 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 24, 2021
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few comments.

doc/contributing/submitting-changes.chapter.md Outdated Show resolved Hide resolved
doc/contributing/submitting-changes.chapter.md Outdated Show resolved Hide resolved
doc/contributing/submitting-changes.chapter.md Outdated Show resolved Hide resolved
doc/contributing/submitting-changes.chapter.md Outdated Show resolved Hide resolved
@davidak davidak force-pushed the package-tests branch 2 times, most recently from 2194f30 to 1c35fb8 Compare April 24, 2021 16:18

This is an example using the `phoronix-test-suite` package with the current best practices.

Add the tests in `passthru.tests` to the package definition like this:
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 would like to link checkPhase and passthru.tests inside the document, but i don't know how and if its worth adding now when the build pipeline changes soon and it might brake

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, current level of interlinking in the manual leaves a lot to be desired.

If you just use the anchor name, it will work now and we will migrate it:

Suggested change
Add the tests in `passthru.tests` to the package definition like this:
Add the tests in [`passthru.tests`](#var-meta-tests) to the package definition like this:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rember the first time I saw passthru, I went like: "?!?!!?!? (It's not even an english word)"

Maybe one small phrase to introduce that concept here would be in order

By convention, we use the passthru attribute which allows us to store arbitrary data, ...

(If that makes sense)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blaggacao yeah, that's true. there is reference documentation at https://nixos.org/manual/nixpkgs/stable/#sec-standard-meta-attributes

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great thanks.


Tests are important to ensure quality and make reviews and automatic updates easy.

Nix package tests are a lightweight alternative to [NixOS module tests](https://nixos.org/manual/nixos/stable/#sec-nixos-tests). They can be used to create simple integration tests for packages while the module tests are used to test services or programs with a graphical user interface on a NixOS VM. Unittests that are included in the source code of a package should be executed in the `checkPhase`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module tests are used to test services or programs with a graphical user interface on a NixOS VM

We can handle graphical test just fine using Xvfb. The NixOS tests are primarily integration tests that depend on other services or to test NixOS modules themselves. passthru.tests should really be preferred whenever possible since the NixOS tests really are heavy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invocation is

xvfb-run -s '-screen 0 800x600x24' dbus-run-session \
--config-file=${dbus.daemon}/share/dbus-1/session.conf \
meson test --print-errorlogs

Sometimes the dbus session part can be omitted.

can it replace https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/shattered-pixel-dungeon.nix ?

Yeah, that should not be NixOS test.

But xvfb-run just creates a virtual frame buffer so that you can start graphical programs – you will still need to write your own tests or use upstream ones. We should probably create some helpers for passthru tests so that we can use tesseract like in the NixOS test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be installCheckPhase is you want to e.g. test the final result? (IIRC that was wan of the ideas behind the NixCon 2017 talk as well).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarification on that is added with #119731.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtojnar thanks!

We should probably create some helpers for passthru tests so that we can use tesseract like in the NixOS test.

that's probably what #36842 tried, but it was rejected because it duplicated nixos tests... maybe it can be generalized somehow so it works for both

@teto
Copy link
Member

teto commented Apr 24, 2021

nice improvement. There are also tests in pkgs/tests, should these be moved on to their respective software directory ?

@davidak
Copy link
Member Author

davidak commented Apr 24, 2021

@teto thanks!

There are also tests in pkgs/tests

oh, i see that for the first time (it exists since 2004 😮 )

should these be moved on to their respective software directory ?

if possible, yes.

i would like to have a general test-version.nix, that would fit there well

or if a test includes multiple packages, like many nixosTests

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/improve-a-pkgs-tests-for-nixpkgs-update/9530/17


Tests are important to ensure quality and make reviews and automatic updates easy.

Nix package tests are a lightweight alternative to [NixOS module tests](https://nixos.org/manual/nixos/stable/#sec-nixos-tests). They can be used to create simple integration tests for packages while the module tests are used to test services or programs with a graphical user interface on a NixOS VM. Unittests that are included in the source code of a package should be executed in the `checkPhase`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be installCheckPhase is you want to e.g. test the final result? (IIRC that was wan of the ideas behind the NixCon 2017 talk as well).

- [Lobster compile test](https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/compilers/lobster/test-can-run-hello-world.nix)
- [Spacy annotation test](https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/spacy/annotation-test/default.nix)
- [Libtorch test](https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/science/math/libtorch/test/default.nix)
- [Multiple tests for nanopb](https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/nanopb/default.nix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to link to a specific git revision here. Otherwise we'll risk ending up with a bunch of dead links.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense for the long-term perspective where someone reads this manual version in 20 years.

All examples are terrible and will be replaced tho.

@roberth
Copy link
Member

roberth commented Apr 24, 2021

This seems a bit more elaborate than #119731 but perhaps you could 'steal' a line or two.

Also check out #119942, which creates a better way to reference the package, supporting overrides.

@abathur
Copy link
Member

abathur commented Apr 24, 2021

@davidak A few days ago in the contributor-retention discourse thread I sketched out some questions I could imagine gaining perspective on via "what is meaningful to test" documentation. trying to keep my brain focused on something today so I haven't read this yet, but I wanted to go ahead and link it in case it has any utility for you here: https://discourse.nixos.org/t/contributor-retention/12412/75

Apologies if it's mostly overlap.

@davidak
Copy link
Member Author

davidak commented Apr 24, 2021

@abathur thanks. this just documents the best practices as of nov. 2019. when it's merged, we can extend the scope

we need documentation on what to test and how

@blaggacao
Copy link
Contributor

blaggacao commented Apr 24, 2021

I want to crosslink to my WIP effort to provide a nixpkgs-devshell. My vision is that we can provide templates per language subsytem and that those templates would include a test skeleton, as well.

Very early stage, nothing works, but from looking at the code, one might be able to get the general idea:

https://github.com/blaggacao/nixpkgs-devshell#readme

(... in search for capable contributors to submit those templated best practices)

@lukegb
Copy link
Contributor

lukegb commented May 1, 2021

Discussion on this seems to have slowed down: @davidak @jtojnar are we ready to 🚢 this?

@davidak
Copy link
Member Author

davidak commented May 1, 2021

@lukegb thanks for noticing. i'm also afraid this will stall. from my perspective it is ready to merge. more reviews would be nice

@lukegb lukegb merged commit 147c701 into NixOS:master May 1, 2021
@lukegb
Copy link
Contributor

lukegb commented May 1, 2021

:shipit:

@davidak davidak deleted the package-tests branch May 1, 2021 21:38
@knedlsepp
Copy link
Member

Are such passthru.tests being built on hydra?

@roberth
Copy link
Member

roberth commented Jul 9, 2021

They're run by ofborg, but not by hydra, as far as I know.

This means problems in changed dependencies can go unnoticed until someone/something changes the package with a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: documentation This PR adds or changes documentation 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how to test packages automatically Test all nix expressions where applicable Package tests
10 participants