Briefly tested it with python 2.7 and 3.7 on NixOS and seems fine in both cases. Maybe it would be nice to briefly comment in the derivation why tests are disabled.

@d-goldin Can do - they're currently disabled bc they require networking - is it preferable to keep them disabled or is there a way to allow networking during the checkPhase?

@endocrimes: Yeah, I tried it before and arrived at the same conclusion. Just thought it might make sense to leave a remark in the code so it can be patched in the future or just inform.

To my knowledge wrt. network access - while there might be some ways to hack around the sandbox, we shouldn't go there, as it would potentially sacrifice purity reproducibility. But also better to not keep hitting external resources from the build-infra.

So often such tests are either selectively disabled/removed or patched to not rely on external resources (ideally upstream of course).

@d-goldin 👍 added a comment

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/52

is there a way to allow networking during the checkPhase?

No, the nix language still doesn't have a good way for that AFAIK. Well, you could cheat by creating a separate fixed-output derivation that produces something trivial on success (e.g. an empty file) and doing tests within that, but... there are also good arguments to try avoiding tests that require internet access. I believe that localhost networking should work fine already (even in the sandbox).

@vcunat I’m well aware of the downsides of running tests that require network access 😅 - in this case I’m packaging a depending that I am not a maintainer of, so I have limited influence over that. In this case disabling checks is probably fine though.

If these are just exceptions, it might be worth trying to disable just those that need internet.

I'm not sure if the disabled tests are holding this PR back, but in case it helps, here is a commit that more selectively disables tests by using a pre-existing decorator: d-goldin@f796e1e

I took your patch with my nitpicks as b199e30. At a quick look, it might be worth submitting upstream, if you like.

@vcunat: Why doCheck alone was insufficient, is because it runs ${python.interpreter} setup.py test per default, but for that a test_suite should be defined in setup.py. What is done in the upstream repo instead is that they use toxto kick-off the tests and run for different "environments" (https://github.com/sethmlarson/rfc6555/blob/master/tox.ini#L8). For our purposes here trying to use tox would have been more complicated than just adjusting the phase.

Thanks.

Here I did checkPhase -> postCheck, so that we only extend the logic instead of overriding it. Long-term experience says it's much better that way.