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

PULL_REQUEST_TEMPLATE: generalize binary test checkbox #36786

Closed

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 11, 2018

Motivation for this change

Many derivations don't produce a binary, but several libraries or
scripts. Therefore the bullet point about testing shouldn't focus on
testing binaries only, but on everything which lives in ./result

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Many derivations don't produce a binary, but several libraries or
scripts. Therefore the bullet point about testing shouldn't focus on
testing binaries only, but on everything which lives in `./result`
@FRidh
Copy link
Member

FRidh commented Mar 11, 2018

Many derivations don't produce a binary, but several libraries or
scripts. Therefore the bullet point about testing shouldn't focus on
testing binaries only, but on everything which lives in ./result

Yes and no. Testing libraries may be much harder, so the scope really is /bin. In that case, replace "binaries" with "executables"?

@7c6f434c
Copy link
Member

A longer term observation: for many libraries we have a primary executable reverse dependency in Nixpkgs, though. And testing just the main use case is definitely faster than a full nox-review wip. Should we open an issue to collect ideas about codification of that and the related guidelines? (Maybe after the release is officially announced).

@Ma27
Copy link
Member Author

Ma27 commented Mar 11, 2018

Not sure if I made my point clear enough, hence another try:

I've been bothered by this item on the checklist for some time now, whenever I patch the derivation of a library (lately several python packages), I also check this item when I loaded the module into a REPL and confirmed that the main API is still functional and no executable in ./result/bin was available. I submitted a simple patch to show what I'd like to change and start a discussion about this to see whether or not I'm the only one.

Testing libraries may be much harder, so the scope really is /bin. In that case, replace "binaries" with "executables"?

ACK. My main point was that the phrasing might be confusing in case of a lib-only derivation.
How do you fell with adding to separate points (tested executables, tested build outputs (e.g. libraries, properly built man pages, ...))?

Should we open an issue to collect ideas about codification of that and the related guidelines

👍 I think finding a suitable solution everyone is happy with might even reduce the amount of breackage because of insufficient testing :-)

@Ma27
Copy link
Member Author

Ma27 commented Mar 23, 2018

I just talked to @fpletz to get further opinions some days ago.
Despite the (absolutely valid argument regarding the effort to test single libraries I already answered too) he doesn't see the need as the problem can/will be solved thanks to the PR bot which is able to check possible rebuilds as well.

As three core maintainers basically have the same position I think that it's absolutely fine to close it for now and continue the discussion in case more people are interested...

@Ma27 Ma27 closed this Mar 23, 2018
@Ma27 Ma27 deleted the generalize-testing-checkbox-in-pr-template branch March 23, 2018 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants