Nixpkgs pull request: ask for instructions to test

Thanks to the great Nix’s system, many people can contribute to nixpkgs… the problem is that Nix is victim of its popularity as many Pull Request takes ages to be reviewed and merged, up to the point of the creation of this thread.

I have the feeling that this process could be speed-up with a simple addition: we should semi-automatically give instructions on how to test a PR. This way:

  • testing most PR is just a simple copy/paste
  • it is easier to see which PR are easy to review and which need more effort

Some people/scripts already do it greatly. For instance nixpkgs-update does provide nice instructions as seen here. But most people don’t (I just picked random PR) : even simpler PR that just introduce a new package like here) do not provide any instructions while they are trivial to test with nixpkgs-review… and we can’t really blame anyone as we don’t provide any template to make it easy. For some more complex PR like here, it’s not clear how simple testing the PR is… adding instructions would clarify this point.

Of course, it’s hard to automatically generate instructions for any PR, but at least adding a section in the template with ‘how to test this PR’, pre-filled with some generic code would help a lot, forcing the author of the PR to think about it (maybe github provides a way to automatically insert the url of the current PR to make it even easier to integrate with nixpkgs-review? Or we can use a bot otherwise that edits the original message.).

What do you think?

3 Likes

Yes, fully agreed.

Would you mind adding a PR to adapt the PR template? A concrete proposal is always easier to evaluate than questions or opinions, and much more likely to produce a tangible outcome.

A minimally invasive change would be to provide a section at the end:

# How to test this change?

<!-- provide instructions to reviewers -->

The PR thread is an abomination, and in my opinion quite pointless: a PR should be opened (or put out of draft mode) if and only if it is ready for review. Picking PRs for review should not be based on recency, but some measure of importance (as proposed here: Improving Nix developer experience). Following up on changes should follow a well-defined process that can already be managed entirely through GitHub via notifications.

The PR templates are also way too noisy and would benefit from some refactoring. But that’s just my two cents of ranting about problems I will probably never get around to address myself.

Sure, I can give it a try this evening. Just, does anyone knows if we have a procedure to automatically replace, say, {{CURRENT_PR_URL}} with the URL of the current PR? This url is needed for nixpkgs-review but does not exist yet when you create the PR. I can maybe use github actions for that, I need to check it it has enough rights to edit a PR.

For a while it was quite trendy to execute the PR testing instructions that the PR template gives (nixpkgs-review in mass on open PRs), some people even had a bot running that did this automatically on PRs. Quite frankly this was a disaster and lead to a lot of pointless noise.

This is because testing if things still build is a bit error prone (for large changes you need to check for false negatives as well usually) and it is not the only condition for checking if something is mergeable. I would argue that the code review and judgement of the potential impact must come first and then build testing should be done to verify one’s assumptions, the problem being of course, that not everyone is necessarily familiar enough with the matter to determine this.

We have automatic testing infrastructure, i.e. ofborg, which I’d argue is much more important. By filling passthru.tests with appropriate cases and reverse dependencies it can also give a decent indication of impact. The big advantage of this is trust and, crucially, platform coverage. Whether it works on x86_64-linux is not a hard question to answer for most people and thus semi-automatic testing of changes mostly contributes to this answer of the question.

2 Likes

Okay, agreed. Skimming a couple of recent pull requests, none of them have tests. I suppose this is because writing tests for Nixpkgs and NixOS feels kind of hard and inscrutable. We could address this by adding better documentation and providing guidance e.g. with tutorials. Just thinking out loud, I’m not actually going to do this any time soon. But something to keep in mind.

Also, NixOS tests have a ton of overhead around booting the heavy VM. Not the best debugging experience, and that’s while tests are even more finicky than builds… And hacky non-NixOS-based solutions for GUI testing were and probably will be rejected for duplicating functionality.

This might get better after Nix PR 3600 gets merged (it talks about not using nixbldN users but as a minor side-effect it allows running NixOS-in-container in a Nix build with much much better «boot» times)

For non-leaf packages just inheriting some reverse deps into passthru.tests (like harfbuzz has), is already really valuable. It doesn’t need to be anything fancy.

passthru.tests can also be simple derivations which gets you quite far usually.

2 Likes
Hosted by Flying Circus.