From 1be8a1c7398b16fec4afb9c44d9fd1608cb2516a Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Wed, 3 Nov 2021 16:50:00 -0700 Subject: [PATCH 01/14] Formalize tests for nixpkgs packages --- rfcs/0119-testing-conventions.md | 114 +++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 rfcs/0119-testing-conventions.md diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md new file mode 100644 index 000000000..7deaea1b5 --- /dev/null +++ b/rfcs/0119-testing-conventions.md @@ -0,0 +1,114 @@ +--- +feature: Defined conventions around testing of official Nixpkgs packages. +start-date: 2021-09-03 +author: Jonathan Ringer +co-authors: +shepherd-team: +shepherd-leader: +related-issues: + - [RFC 0088 - Nixpkgs Breaking Change Policy](https://github.com/NixOS/rfcs/pull/88) +--- + +# Summary +[summary]: #summary + +When updating or modifying packages, several conventions for testing regressions +have been adopted. However, these practices are not standard, and generally it's not well +defined how each testing method should be implemented. It would be beneficial to have +an unambiguous way to say that a given package, and all downstream dependencies, have +had as many automated test ran possible. This will give a high degree of certainty that +a given change is less likely to manifest regressions once introduced on a release +channel. + +Another desire of this rfc is also to have a way for various review tools +(e.g. ofborg, hydra, nixpkgs-review) to have a standard way to determine if a +package has additional tests which can help verify it's correctness. + +# Motivation +[motivation]: #motivation + +Breakages are a constant painpoint for nixpkgs. It is a very poor user experience to +have a configuration broken because one or more packages fail to build. Often when +these breakages occur, it is because the change had a large impact on the entirity +of nixpkgs; and unless there's a dedicated hydra jobset for the pull request, it's +infeasible to expect pull request author(s) to verify every package that's affected +by a change they are proposing. However, it is feasible to specify packages that +are very likely to be affected by changes in another package, and use this information +to help mitigate regressions from appearing in release channels. + +# Detailed design +[design]: #detailed-design + +Standardize `passthru.tests.` and `passthru.nixosTests.` as a mechanism of +more expensive but automatic testing for nixpkgs. As well as encourage the usage of +`checkPhase` or `installCheckPhase` when packaging within nixpkgs. + +Usage for `passthru.nixosTests.` +- Reserved for tests utilitizing the nixosTest utilties. + - Generally these are more resource intensive, and may require additional system features + such as kvm + +Usage for `passthru.tests.`: +- Running tests which include downstream dependencies. + - This avoids cyclic dependency issues for test suites. +- Running lengthy or more resource expensive tests. + - There should be a priority on making package builds as short as possible. + - This reduces the amount of compute required for everyone reviewing, building, or iterating on packages. +- Referencing downstream dependencies which are most likely to experience regressions. + - Most applicable to [RFC 0088 - Nixpkgs Breaking Change Policy](https://github.com/NixOS/rfcs/pull/88), +as this will help define what breakages a pull request author should take ownership. + +Usage for mkDerivation's `checkPhase`: +- Quick "cheap" tests, which run units tests and maybe some addtional scenarios. +- Since this contributes to the "build time" of a package, there should be some +emphasis on ensuring this phase isn't bloated. + +Usage for mkDerivations `installCheckPhase`: +- A quick trivial example (e.g. ` --help`) to demonstrate that one (or more) +of the programs were linked correctly. +- Assert behavior post installation (e.g. python's native extensions only get installed +and are not present in a build directory) + +# Examples and Interactions +[examples-and-interactions]: #examples-and-interactions + +This section illustrates the detailed design. This section should clarify all +confusion the reader has from the previous sections. It is especially important +to counterbalance the desired terseness of the detailed design; if you feel +your detailed design is rudely short, consider making this section longer +instead. + +# Drawbacks +[drawbacks]: #drawbacks + +None? This is opt-in behavior for package maintainers. + +# Alternatives +[alternatives]: #alternatives + +Continue to use current ad-hoc conventions. + +# Unresolved questions +[unresolved]: #unresolved-questions + +How far should testing go? +- What consistitutes that "enough testing" was done to a package before a change was merged? + +Should `.passthru.tests` be flat? +For packages which have extremes in resource usage when testing (e.g. pytorch), it may +be beneficial to have additional structure for the tests to denote expectations of resources +and ownership of testing for upstream packages. + +# Future work +[future]: #future-work + +Problem with onboarding more test to aspects of nixpkgs CI and processes is the increased +need of compute, storage, and ram resources. Therefore, consideration of future work should +take into consideration how much testing is feasible for a given change. + +Onboarding of CI tools to support testing paradigms: +- ofborg + - Testing of .passthru.tests is already done. + - Testing of downstream dependencies and their tests when minimal (e.g. <10 rebuilds?) +- hydra + - Allow for derivations exposed to hydraJobs to also probe for `.passthru.tests`? From 4f5c46d02d4625d425a050ebd23772264a85f24a Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Mon, 10 Jan 2022 00:20:55 -0800 Subject: [PATCH 02/14] Update rfcs/0119-testing-conventions.md Co-authored-by: Kevin Cox --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 7deaea1b5..a2cb8c2e2 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -31,7 +31,7 @@ Breakages are a constant painpoint for nixpkgs. It is a very poor user experienc have a configuration broken because one or more packages fail to build. Often when these breakages occur, it is because the change had a large impact on the entirity of nixpkgs; and unless there's a dedicated hydra jobset for the pull request, it's -infeasible to expect pull request author(s) to verify every package that's affected +infeasible to expect pull request authors to verify every package affected by a change they are proposing. However, it is feasible to specify packages that are very likely to be affected by changes in another package, and use this information to help mitigate regressions from appearing in release channels. From 38be7244c55d7dc656b59ba94a3871346f167d13 Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Thu, 3 Feb 2022 16:26:50 -0800 Subject: [PATCH 03/14] Update rfcs/0119-testing-conventions.md --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index a2cb8c2e2..9221efea8 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -1,6 +1,6 @@ --- feature: Defined conventions around testing of official Nixpkgs packages. -start-date: 2021-09-03 +start-date: 2021-12-29 author: Jonathan Ringer co-authors: shepherd-team: From 501c10f083b733747ff85e11e8ad62234f4b28db Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Mon, 14 Mar 2022 08:21:17 -0700 Subject: [PATCH 04/14] Update rfcs/0119-testing-conventions.md Co-authored-by: Anderson Torres --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 9221efea8..bbd71441e 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -29,7 +29,7 @@ package has additional tests which can help verify it's correctness. Breakages are a constant painpoint for nixpkgs. It is a very poor user experience to have a configuration broken because one or more packages fail to build. Often when -these breakages occur, it is because the change had a large impact on the entirity +these breakages occur, it is because the change had a large impact on the entirety of nixpkgs; and unless there's a dedicated hydra jobset for the pull request, it's infeasible to expect pull request authors to verify every package affected by a change they are proposing. However, it is feasible to specify packages that From 7adec2563edad7e184e3fad352875ff7c7308c06 Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Mon, 14 Mar 2022 08:21:29 -0700 Subject: [PATCH 05/14] Update rfcs/0119-testing-conventions.md Co-authored-by: Anderson Torres --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index bbd71441e..08741e526 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -22,7 +22,7 @@ channel. Another desire of this rfc is also to have a way for various review tools (e.g. ofborg, hydra, nixpkgs-review) to have a standard way to determine if a -package has additional tests which can help verify it's correctness. +package has additional tests which can help verify its correctness. # Motivation [motivation]: #motivation From 21ecc190ceb79fcd7d3eceebb74af1558d70740e Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Fri, 25 Mar 2022 10:03:11 -0700 Subject: [PATCH 06/14] Update rfcs/0119-testing-conventions.md --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 08741e526..503bcf79d 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -108,7 +108,7 @@ take into consideration how much testing is feasible for a given change. Onboarding of CI tools to support testing paradigms: - ofborg - - Testing of .passthru.tests is already done. + - Testing of `.passthru.tests` is already done. - Testing of downstream dependencies and their tests when minimal (e.g. <10 rebuilds?) - hydra - Allow for derivations exposed to hydraJobs to also probe for `.passthru.tests`? From 3ae0279d3d3af1a948ab93992f3c9665906793cd Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Fri, 25 Mar 2022 10:03:19 -0700 Subject: [PATCH 07/14] Update rfcs/0119-testing-conventions.md --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 503bcf79d..4972341f3 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -94,7 +94,7 @@ Continue to use current ad-hoc conventions. How far should testing go? - What consistitutes that "enough testing" was done to a package before a change was merged? -Should `.passthru.tests` be flat? +Should `.passthru.tests` be flat? For packages which have extremes in resource usage when testing (e.g. pytorch), it may be beneficial to have additional structure for the tests to denote expectations of resources and ownership of testing for upstream packages. From 6870a94220624f7008336036872e4e603327598f Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Fri, 25 Mar 2022 10:03:34 -0700 Subject: [PATCH 08/14] Update rfcs/0119-testing-conventions.md --- rfcs/0119-testing-conventions.md | 9 --------- 1 file changed, 9 deletions(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 4972341f3..9276f6f1d 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -69,15 +69,6 @@ of the programs were linked correctly. - Assert behavior post installation (e.g. python's native extensions only get installed and are not present in a build directory) -# Examples and Interactions -[examples-and-interactions]: #examples-and-interactions - -This section illustrates the detailed design. This section should clarify all -confusion the reader has from the previous sections. It is especially important -to counterbalance the desired terseness of the detailed design; if you feel -your detailed design is rudely short, consider making this section longer -instead. - # Drawbacks [drawbacks]: #drawbacks From 51579885f6f54126c1d2c90f4f9213c0bb8e5b9a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 6 Apr 2022 17:36:33 +0200 Subject: [PATCH 09/14] Add shepherds --- rfcs/0119-testing-conventions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 9276f6f1d..33b78b4a3 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -3,8 +3,8 @@ feature: Defined conventions around testing of official Nixpkgs packages. start-date: 2021-12-29 author: Jonathan Ringer co-authors: -shepherd-team: -shepherd-leader: +shepherd-team: @Mic92 @Artturin @kevincox +shepherd-leader: @Mic92 related-issues: - [RFC 0088 - Nixpkgs Breaking Change Policy](https://github.com/NixOS/rfcs/pull/88) --- From 1de8d71a4e14d90426ac327e74153682175118aa Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Sat, 30 Apr 2022 10:42:44 -0700 Subject: [PATCH 10/14] Update RFC post 30th Apr meeting --- rfcs/0119-testing-conventions.md | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 33b78b4a3..3659bbf36 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -39,16 +39,11 @@ to help mitigate regressions from appearing in release channels. # Detailed design [design]: #detailed-design -Standardize `passthru.tests.` and `passthru.nixosTests.` as a mechanism of +Standardize `passthru.tests.` as a mechanism of more expensive but automatic testing for nixpkgs. As well as encourage the usage of `checkPhase` or `installCheckPhase` when packaging within nixpkgs. -Usage for `passthru.nixosTests.` -- Reserved for tests utilitizing the nixosTest utilties. - - Generally these are more resource intensive, and may require additional system features - such as kvm - -Usage for `passthru.tests.`: +Criteria for `passthru.tests.`: - Running tests which include downstream dependencies. - This avoids cyclic dependency issues for test suites. - Running lengthy or more resource expensive tests. @@ -57,6 +52,9 @@ Usage for `passthru.tests.`: - Referencing downstream dependencies which are most likely to experience regressions. - Most applicable to [RFC 0088 - Nixpkgs Breaking Change Policy](https://github.com/NixOS/rfcs/pull/88), as this will help define what breakages a pull request author should take ownership. +- Running integration tests (E.g. nixosTests) + - Tests which have heavy usage or platform requirements should add the appropriate systemFeature + - E.g. `nixos-test` `kvm` `big-parallel` Usage for mkDerivation's `checkPhase`: - Quick "cheap" tests, which run units tests and maybe some addtional scenarios. @@ -85,21 +83,23 @@ Continue to use current ad-hoc conventions. How far should testing go? - What consistitutes that "enough testing" was done to a package before a change was merged? -Should `.passthru.tests` be flat? -For packages which have extremes in resource usage when testing (e.g. pytorch), it may -be beneficial to have additional structure for the tests to denote expectations of resources -and ownership of testing for upstream packages. +Hydra: How would this look for hydra adoption and hydraChecks? # Future work [future]: #future-work -Problem with onboarding more test to aspects of nixpkgs CI and processes is the increased +One problem with onboarding more tests to the current nixpkgs CI and processes is the increased need of compute, storage, and ram resources. Therefore, consideration of future work should take into consideration how much testing is feasible for a given change. Onboarding of CI tools to support testing paradigms: +- nixpkgs-review + - Run `passthru.tests` on affected packages + - Allow for filtering based upon requiredSystemFeatures - ofborg - Testing of `.passthru.tests` is already done. - - Testing of downstream dependencies and their tests when minimal (e.g. <10 rebuilds?) -- hydra - - Allow for derivations exposed to hydraJobs to also probe for `.passthru.tests`? + +Nixpkgs: +- Add existing nixosTests to related packages +- Updated testing clause on PR template +- Update contributing documentation From 12dec978c91b686aa31225ea2624c79e7ea5a076 Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Sat, 30 Apr 2022 10:55:44 -0700 Subject: [PATCH 11/14] Update rfcs/0119-testing-conventions.md --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 3659bbf36..02500e04f 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -101,5 +101,5 @@ Onboarding of CI tools to support testing paradigms: Nixpkgs: - Add existing nixosTests to related packages -- Updated testing clause on PR template +- Update testing clause on PR template - Update contributing documentation From 8111905fd805d4891f3ea5c2a31848f3d4023b1d Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Wed, 15 Jun 2022 10:27:07 -0700 Subject: [PATCH 12/14] passthru.tests -> tests --- rfcs/0119-testing-conventions.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 02500e04f..241fc7146 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -39,11 +39,11 @@ to help mitigate regressions from appearing in release channels. # Detailed design [design]: #detailed-design -Standardize `passthru.tests.` as a mechanism of +Standardize `tests.` (previously `passthru.tests.` as a mechanism of more expensive but automatic testing for nixpkgs. As well as encourage the usage of `checkPhase` or `installCheckPhase` when packaging within nixpkgs. -Criteria for `passthru.tests.`: +Criteria for `tests.`: - Running tests which include downstream dependencies. - This avoids cyclic dependency issues for test suites. - Running lengthy or more resource expensive tests. @@ -94,10 +94,10 @@ take into consideration how much testing is feasible for a given change. Onboarding of CI tools to support testing paradigms: - nixpkgs-review - - Run `passthru.tests` on affected packages + - Run `.tests` on affected packages - Allow for filtering based upon requiredSystemFeatures - ofborg - - Testing of `.passthru.tests` is already done. + - Testing of `.tests` is already done. Nixpkgs: - Add existing nixosTests to related packages From c0c66c6648a60e08808f12941068b75ac1dc6227 Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Wed, 15 Jun 2022 10:30:05 -0700 Subject: [PATCH 13/14] typo --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 241fc7146..337dbdb3f 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -39,7 +39,7 @@ to help mitigate regressions from appearing in release channels. # Detailed design [design]: #detailed-design -Standardize `tests.` (previously `passthru.tests.` as a mechanism of +Standardize `tests.` (previously known as `passthru.tests.`) as a mechanism of more expensive but automatic testing for nixpkgs. As well as encourage the usage of `checkPhase` or `installCheckPhase` when packaging within nixpkgs. From 929669e801ee78e69c2b322c4e68e8458edb4187 Mon Sep 17 00:00:00 2001 From: Jonathan Ringer Date: Thu, 16 Jun 2022 16:10:17 -0700 Subject: [PATCH 14/14] Update rfcs/0119-testing-conventions.md Co-authored-by: Robert Hensing --- rfcs/0119-testing-conventions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0119-testing-conventions.md b/rfcs/0119-testing-conventions.md index 337dbdb3f..a8fa95d67 100644 --- a/rfcs/0119-testing-conventions.md +++ b/rfcs/0119-testing-conventions.md @@ -39,7 +39,7 @@ to help mitigate regressions from appearing in release channels. # Detailed design [design]: #detailed-design -Standardize `tests.` (previously known as `passthru.tests.`) as a mechanism of +Standardize `tests.` (commonly set with the `passthru.tests.` argument to `mkDerivation`) as a mechanism of more expensive but automatic testing for nixpkgs. As well as encourage the usage of `checkPhase` or `installCheckPhase` when packaging within nixpkgs.