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

phoronix-test-suite: add tests #75117

Merged
merged 1 commit into from Apr 24, 2020
Merged

Conversation

@davidak
Copy link
Member

@davidak davidak commented Dec 6, 2019

Motivation for this change

Add tests to make reviews easier and the package more stable.

This is my attempt to implement package testing like discussed in #73076 (comment).

Please provide feedback!

Some aspects might be controversial like with phoronix-test-suite;. It allows to simply use "${version}" in the tests, but using with was discouraged for some reason.

Also what about naming?

  passthru.tests = {
    tests = callPackage ./tests.nix { };
  };

tests tests tests don't read nicely. should we call it "simple" or "basic"?

When it is approved i would like to add tests to more packages and document it.

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 nix-review --run "nix-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.
Notify maintainers

cc @jtojnar @worldofpeace @Profpatsch @7c6f434c

@davidak davidak marked this pull request as ready for review Dec 6, 2019
Copy link
Contributor

@jtojnar jtojnar left a comment

To be honest, I do not see much value in these trivial execution tests. I would say that the application builds but does not execute after update is pretty small. And the reviewer still needs to at least run the program and try some action so it will be already covered by this.

Even a trivial actions would be enough for this approach to be worth in my eyes (e.g. testing that calculator returns 4 for 2+2). But perhaps this is a rather hard program to test, as it probably relies on real hardware.

pkgs/tools/misc/phoronix-test-suite/tests.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/phoronix-test-suite/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/phoronix-test-suite/tests.nix Outdated Show resolved Hide resolved
@davidak davidak force-pushed the davidak:phoronix-test-suite-tests branch from cbaccdb to c2cf859 Dec 7, 2019
@davidak
Copy link
Member Author

@davidak davidak commented Dec 7, 2019

@jtojnar thanks for your feedback!

To be honest, I do not see much value in these trivial execution tests. I would say that the application builds but does not execute after update is pretty small. And the reviewer still needs to at least run the program and try some action so it will be already covered by this.

My plan is to automate this manual tests to check this boxes:

  • Built on platform(s) NixOS
  • Tested execution of all binary files (usually in ./result/bin/)

This is especially useful for automated updates with nixpkgs-update, but also for easier review with ofborg and nixpkgs-review.

Even a trivial actions would be enough for this approach to be worth in my eyes (e.g. testing that calculator returns 4 for 2+2). But perhaps this is a rather hard program to test, as it probably relies on real hardware.

Yes, this specific program downloads tests from the internet (about 1 MB or more) and run a benchmark which uses 100% CPU on one core for several minutes. Nothing you want to run on a CI.

So i have choosen this dummy command to test if basic functionality is given.

For other programs i would test at least one common task.

Do you see a difference in testing with this automated tests and testing manual in a real terminal?

@davidak davidak force-pushed the davidak:phoronix-test-suite-tests branch from c2cf859 to 972d999 Dec 7, 2019
@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Dec 8, 2019

I would say that the application builds but does not execute after update is pretty small.

In general, when packaging, I think «builds and fails to load a library on start» is more typical than «builds and runs but doesn't work even on simplest examples»…

@davidak davidak force-pushed the davidak:phoronix-test-suite-tests branch from 972d999 to e9007b3 Dec 10, 2019
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jan 25, 2020

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/108

@davidak davidak changed the title [WIP] phoronix-test-suite: add tests phoronix-test-suite: add tests Mar 7, 2020
@davidak
Copy link
Member Author

@davidak davidak commented Mar 7, 2020

All remarks are resolved so this is ready to merge.

There were no further comments, so this seems to be fine.

I already reviewed 2 updates where this would have been helpful.

@garbas
garbas approved these changes Apr 24, 2020
@garbas garbas merged commit 0dfdfc2 into NixOS:master Apr 24, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@davidak davidak deleted the davidak:phoronix-test-suite-tests branch Apr 24, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Aug 17, 2020

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

https://discourse.nixos.org/t/how-many-people-are-paid-to-work-on-nix-nixpkgs/8307/51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.