-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
pythonPackages.unittestCheckHook: init #185430
Conversation
8f60b9e
to
8c7e02c
Compare
Wonder why Edit, hours later: but it did for the same type of problem just now...? |
echo "Executing unittestCheckPhase" | ||
runHook preCheck | ||
|
||
eval "@pythonCheckInterpreter@ -m unittest discover $unittestFlags" |
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.
I'm not sure if this is the best way to do things.
The issue that I ran into was that if I did python -m unittest discover "${unittestFlags[@]}"
, a flags value of [ "-s" "tests" ]
would expand to a single argument, -s tests
, which argparse
(which unittest
uses) would parse as the value for the -s
argument being tests
(with a space).
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.
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.
That's basically doing what I'm doing here, as far as I can tell -- I believe it has to do that specifically so it can add its own arguments if needed.
I don't like that OfBorg added the clean up label, though I don't think it gives specifics as to why. 😕 As far as I know, I fixed every issue that caused packages to not eval, so I'm not sure what else it would be adding that for (even if it should fail for that)... |
8c7e02c
to
e4026f7
Compare
e4026f7
to
92ba123
Compare
don't worry about it. I am not sure what triggered it but maybe the treewide change. |
As it is now, the unittest hook should be added to |
92ba123
to
3c9d334
Compare
Fixed. |
pkgs/development/interpreters/python/hooks/unittest-check-hook.sh
Outdated
Show resolved
Hide resolved
be6acc7
to
26ca2a6
Compare
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.
Looks good to me, one more nit.
26ca2a6
to
19adc33
Compare
This broke tests in |
Yes, someone already noted that -- there's a quick fix but also a more proper one that should be upstreamed. See https://matrix.to/#/!kjdutkOsheZdjqYmqp%3Anixos.org/%24_WKyurbQoNXb49piMvNidpULeGz7zFJh-LFM4HoAttE |
In PR #187531 |
Other regressions probably won't have significant impact, but let me post them here anyway, in case someone's interested: |
I made #187778 to fix regressions (drafted until latest eval completes). |
In NixOS#185430 the `unittestCheckHook` argument was introduced, causing the derivation to fail building since the `unittestCheckHook` is not passed. However the original PR already used the `unittestCheckHook` from `python3Packages`.
Description of changes
This adds a new hook,
unittestCheckHook
, that runspython -m unittest discover
, as well as migrating the majority of packages that useunittest
to it.Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes