On 2022-10-21 23 h 28, Louis-Philippe Véronneau wrote:
Hello,
This is my review of the pystray package you asked the Debian Python
Team to sponsor in the Debian archive.
1. I doubt d/README.source is needed, as this is a team-maintained
package and most of that info is in the Policy :)
2. in d/rules, you are using "override_dh_sphinxdoc" and then calling
dh_sphinxdoc.
You should instead use "execute_before_dh_sphinxdoc".
3. You are not running any upstream tests, neither at build not as
autopkgtests.
Tests are very important. How do you know your package isn't broken, or
has not been broken by a change in the archive? Only tests can tell you
that.
I see you've noted that some tests require user confirmations and it
indeed seems to be the case for most of the ones in icon_tests.py.
* is there a way to bypass this or are those tests simply not relevant
in a automated environment?
* what about menu_descriptor_tests.py? I gave it a very cursory look,
but it doesn't seem to use the confirm() function. Trying to run it
gives me an "Xlib.error.DisplayNameError: Bad display name" error
though, so chances are you'd need to run tests with xvfb to mock an X
session.
Note that it is my personal policy not to sponsor packages that do not
run upstream tests. I make some exceptions for unusual cases (this might
be one?), but rarely.
Some other people might not be as rigid as I am on this, but I thought
you should know.
---
That's it! I've removed your package from the sponsor queue for now, but
feel free to re-add it when you feel like you've dealt with my review.
Apart from the test problem, this is a pretty good package!
Thanks for you contribution to Debian.
Hello,
I've taken another look at pystray.
First of all, next time, please don't force push, it made it harder to
know what had changed since my last review and I had to start from
scratch :(
The autopkgtests are currently failing. I suspect you are not running
those locally when you are building the package? It's fairly easy to do
on sbuild [1] and I would highly recommend you add this to your standard
build process.
In any case, I get the following error:
E Xlib.error.DisplayNameError: Bad display name ""
I see you patched the upstream code to be able to run the tests at build
(kudos). I suspect either dependencies are missing in the autopkgtest,
or you aren't passing your TEST_SKIP_INTERACTIVE var there.
In either case, those tests need to pass, otherwise the package won't
migrate to testing.
Note that:
1. You have duplicate build-dep entries in d/control
2. Your d/salsa-ci.yml file is currently skipping autopkgtests
I would've have fixed those before uploading, but with the failing
autopkgtests it seems we'll need another back-and-forth round anyway.
Cheers,
[1]: Look for "run_autopkgtest = 1" in /etc/sbuild/sbuild.conf
--
⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ Louis-Philippe Véronneau
⢿⡄⠘⠷⠚⠋ po...@debian.org / veronneau.org
⠈⠳⣄
OpenPGP_0xE1E5457C8BAD4113.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
