[RFC 0119] Formalize testing for nixpkgs packages #119
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,105 @@ | ||||||
| --- | ||||||
| feature: Defined conventions around testing of official Nixpkgs packages. | ||||||
| start-date: 2021-12-29 | ||||||
| author: Jonathan Ringer | ||||||
| co-authors: | ||||||
| shepherd-team: @Mic92 @Artturin @kevincox | ||||||
| shepherd-leader: @Mic92 | ||||||
| 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 its 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 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 | ||||||
| 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 | ||||||
|
|
||||||
|
There was a problem hiding this comment. Recently people have started adding sensitive downstream dependencies passthru.tests Cc @risicle I propose adding a new section called There was a problem hiding this comment. Think this technique was @jonringer 's idea in the first place 😉 The main thing I'm slightly uncomfortable with is adding the reverse dependencies as arguments - I imagine it causing wierdness for people who do a lot of overriding. But I can't think of a sensible way it could be avoided. There was a problem hiding this comment. Ideally the benefit is for nixpkgs testing. For the overriding case, it should be that an override would not trigger a downstream dependency to influence the build as passthru gets pruned before instantiation. I agree it's odd that a package is aware of it's downstream dependencies, but I'm not sure of another way of having a package be aware of them. There was a problem hiding this comment.
A potential mitigation is to source these from a There was a problem hiding this comment. usage of There was a problem hiding this comment. nixpkgs-review builds all reverse dependencies. could we just use this mechanism in a cc @Mic92 |
||||||
| Standardize `passthru.tests.<name>` and `passthru.nixosTests.<name>` 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. | ||||||
|
There was a problem hiding this comment. I'm confused where to draw the line between what should be done in cc @thiagokokada so that you can add your thoughts There was a problem hiding this comment. IMO,
So yeah, between this and the fact that to use There was a problem hiding this comment. Also, IMO, VM-less But if the idea is test only the current package, There was a problem hiding this comment. I think ideally we want expensive build steps spread among different derivations, but that's hard. Expensive tests are sometimes feasible to split, so splitting them is good when it doesn't create a mess. (Cheap checks with no new deps do not follow this logic, of course) Also, separated tests make it more practical to support packages with different level of «works» on different platforms, because we do not need to touch the main build causing expensive rebuild on the «happy» platforms to add partial support (and its tests) on the more exotic ones. There is no single «package is broken», in the black-and-white view we have never had a non-broken LibreOffice packages, for example. There was a problem hiding this comment. Separated tests also make it easier to debug failures, since you can easily access the package (which may build successfully but then fail the test). I think we should follow something like a "principle of least privilege", where it is best practice to always use the test that makes the fewest assumptions about its environment.
As a result, I'd suggest to pick There was a problem hiding this comment. I don't have much ~ecosystem perspective on this, but I do have a few scattered thoughts from authoring and maintaining a small number of packages (broadly, my thinking has been aligning with timokau over time):
There was a problem hiding this comment. I've avoided |
||||||
|
|
||||||
| Usage for `passthru.nixosTests.<name>` | ||||||
| - Reserved for tests utilitizing the nixosTest utilties. | ||||||
|
There was a problem hiding this comment. This doesn't seem a meaningful distinction to me. Maybe we should flip this.
Of course this makes the name less meaningful, but we can bikeshed that later. There was a problem hiding this comment. I was trying to keep it consistent in plurality with the already used There was a problem hiding this comment. The separate name makes testing everything slightly harder. Today, we can do This also has a runtime cost, whereas standardizing on |
||||||
| - Generally these are more resource intensive, and may require additional system features | ||||||
| such as kvm | ||||||
|
There was a problem hiding this comment. We could have an automatic attribute like There was a problem hiding this comment. We decided in the meeting to use It re-uses existing conventions, and largely achieves the same goal. The burden of filtering the tests will be on the tooling side, not the maintainer side. There was a problem hiding this comment. Some in-nixpkgs provisions may still be good, as it can shrink the interface between the tooling and nixpkgs. |
||||||
|
|
||||||
| Usage for `passthru.tests.<name>`: | ||||||
| - 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. | ||||||
|
There was a problem hiding this comment. I think I originally wrote this about sage, and sageWithTests. Which took even longer :) There was a problem hiding this comment. (looks sadly at NixOS/nix#3600 being unmerged) although for a given situation with a package, speeding up separate tests on their own would also not hurt |
||||||
| - 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. | ||||||
|
There was a problem hiding this comment. Is this the only place corresponding to «tests have to pass»? (BTW what about a test removal procedure? Explicit maintainer approval?) There was a problem hiding this comment. In the future work, there's mentions to add it to the PR template, and it's already part of ofborg. There was a problem hiding this comment. I think all of this is vague enough that it remains unclear whether the RFC establishes a norm that tests should be run completely, and have to pass (hmmm, what about platform discrepancies…) There was a problem hiding this comment. This is more meant to set the expectation. There's already somewhat of a convention, and I would like for that to be more explicit and expected.
There was a problem hiding this comment. So basically you do not want to implement something with the full extent of the point from the meeting notes?
(Here my question can be treated as procedural: for the text «updated according to the meeting notes» I am fine both with the text that clearly claims what the point in the meeting notes claims, or with an explicit comment in the discussion that the meeting note point as written is too far-reaching, maybe because it is too brief of a summary of an agreed position; I just want clarity what interpretation of RFC gets covered by the eventual decision) There was a problem hiding this comment.
ofborg already runs
This is a non-goal for the RFC. Personally, I believe this should be left up to the maintainer(s) to decide. The additional tests should be providing value to the review process, if they don't then they should probably be removed, but this can be decided by the maintainers.
Also a non-goal. Just concerned with the ability to test a PR, which may aide in the decision making process to merge a PR. There was a problem hiding this comment. Well, do we ask that tests are run or that tests pass fully at least on one platform (maybe not the first try etc.…)? There was a problem hiding this comment.
I think the tests should be ran; and if any fail, it should be an indicator that the package is unhealthy in context of the PR There was a problem hiding this comment. I agree with the position, and I think it could be made less vague in the RFC. Maybe put that statement that «tests should be run and a failing test is an indicator that the package is unhealthy in the context of the PR.» as a top-level statement in the detailed design? |
||||||
|
|
||||||
| Usage for mkDerivation's `checkPhase`: | ||||||
| - Quick "cheap" tests, which run units tests and maybe some addtional scenarios. | ||||||
|
There was a problem hiding this comment. Not all test suites are quick or cheap, but running them should be a priority over quickness. If we can make running them in a separate derivation easy, that's worth considering, but it seems that the human overhead would not be worth it in the general case.
Suggested change
|
||||||
| - 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. `<command> --help`) to demonstrate that one (or more) | ||||||
| of the programs were linked correctly. | ||||||
|
Comment on lines
+65
to
+66
There was a problem hiding this comment. There is also There was a problem hiding this comment. Related: NixOS/nixpkgs#144503 |
||||||
| - Assert behavior post installation (e.g. python's native extensions only get installed | ||||||
|
There was a problem hiding this comment. The "only" in this sentence is confusing me. There was a problem hiding this comment. Native extensions only get installed. However, most test suites will consume the code in the build directory. So tests will fail because the compiled extensions will not be present. I'm not sure how to word this better. There was a problem hiding this comment. Maybe start with the phrase about the build directory? |
||||||
| and are not present in a build directory) | ||||||
|
|
||||||
|
There was a problem hiding this comment. some programs are hard to test automatically so how about creating a new meta attribute like There was a problem hiding this comment. I strongly oppose this idea. IIUC the goal of this RFC is to make it easier for changes to upstream packages to be tested. The end goal is that we have automatic tooling that can test packages, notify maintainers of breakages and eventually mark as broken if the maintainers are unable to fix the package in time. Adding required manual testing puts unacceptable levels of burden on core package maintainers (that are depended on by hundreds or thousands of other packages). I think a TL;DR I don't want to require people to manually test loads of packages, if you want your package not to break due to changes in dependencies you need automated tests. There was a problem hiding this comment. I guess There was a problem hiding this comment. Perhaps we could record merge conditions instead? There was a problem hiding this comment. I would like to have a |
||||||
| # Drawbacks | ||||||
| [drawbacks]: #drawbacks | ||||||
|
|
||||||
| None? This is opt-in behavior for package maintainers. | ||||||
|
|
||||||
| # Alternatives | ||||||
| [alternatives]: #alternatives | ||||||
|
There was a problem hiding this comment. One alternative is that we consider all dependent packages as tests. We can have dependent packages that are just tests for example a Then a PR author would responsible that all dependents build (aka pass) or are marked broken. The obvious issue here is that for packages with lots of dependents it becomes infeasible for the average author to run a tool that builds everything and marks failures as broken. I think it is worth mentioning this alternative because this RFC demonstrates a clean way to define an appropriate sample size. Then it is expected that nixpkgs-provided build resources can be used for the full build + mark as broken. There was a problem hiding this comment. Yes, and this is the compromise with For example, some packages may make use of many of systemd's features, however, other packages only really use libudev, which is much more stable. We could probably forego the libudev packages, and just list the packages which are using systemd's more advanced features. |
||||||
|
|
||||||
| Continue to use current ad-hoc conventions. | ||||||
|
There was a problem hiding this comment. |
||||||
|
|
||||||
| # 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 `<package>.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. | ||||||
|
There was a problem hiding this comment. I think this differentiation can be solved by |
||||||
|
|
||||||
| # 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 `<package>.passthru.tests` is already done. | ||||||
|
There was a problem hiding this comment. The technical goal of There was a problem hiding this comment. I'm not super familiar with 92, but it looks like it's the future |
||||||
| - Testing of downstream dependencies and their tests when minimal (e.g. <10 rebuilds?) | ||||||
| - hydra | ||||||
| - Allow for derivations exposed to hydraJobs to also probe for `<drv>.passthru.tests`? | ||||||
|
There was a problem hiding this comment. This could be implemented in There was a problem hiding this comment. I removed this line, and added it to unresolved section, as it's more of a hydra concern than anything. Also, the ultimate goal of hydra is to provide channel updates for nixpkgs, in which these tests are more "nice to have" than anything else. Also, I'm not sure if the official hydra should be building all of the tests, if the tests are only meant to influence whether a change should be merged. The official hydra could be populating the cache with a lot of never-to-be-used-again builds. |
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe expand on what are the intended changes to the status quo?
passthru.testsis a name documented inside the manual, howevernixosTestsare recommended to be also put there.(also, if sorting by resource consumption, maybe this split is not needed?)
Are we encouraged to compromise on something in the name of more test coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I wasn't sure what the status quo should be. My current thoughts are, "here is some addtional metadata you can add to ensure that people know how your package may break. Or add your package to the tests of other packages to ensure it's not broken."