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.

Sign up for GitHub

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

Jump to bottom

pythonPackages.astropy: Disable tests #68687

Merged
merged 1 commit into from Sep 24, 2019
Merged

pythonPackages.astropy: Disable tests #68687

merged 1 commit into from Sep 24, 2019

Conversation

JohnAZoidberg
Copy link
Member

Motivation for this change

A ton of tests fail and it's not obvious to me how to fix them.
Adding bleach to checkInputs fixes a tiny number of them, though.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @KentJames

@jonringer
Copy link
Contributor

A lot of them are failing due to pytest5, I took a look at them as well. Generally these are from using pytest.raise(..) as exc: then it will fail because exc is no longer able to coerced into a string... instead it needs to be exc.value.
see mvantellingen/python-zeep#1006 for an example

@JohnAZoidberg
Copy link
Member Author

Thanks, I'll check whether pytest4 fixes them. They're so many that I won't bother fixing them all.

@JohnAZoidberg
Copy link
Member Author

It's not easy to fix. Let's just disable them for now and wait for upstream to fix the tests.

@jonringer
Copy link
Contributor

using pytest4 isn't an option due to having multiple version of a package in the python ecosystem (they all get loaded on to PYTHONPATH, so you can't seperate them)

@JohnAZoidberg JohnAZoidberg mentioned this pull request Sep 14, 2019
Closed
10 tasks
@Ma27
Copy link
Member

Ma27 commented Sep 14, 2019

using pytest4 isn't an option due to having multiple version of a package in the python ecosystem (they all get loaded on to PYTHONPATH, so you can't seperate them)

We also have multiple django and opencv versions in our package set. Also I think that this is perfectly fine if we do this for a test library: pytest either lands in a check phase (and I doubt that any package will require multiple pytest versions) or will be used as propagated build input for any pytest-related library, in that case it's IMHO the task of a maintainer to ensure that the package doesn't depend on multiple pytest versions.

Or did you find a case I'm not aware of? :)

Otherwise I'd suggest to close the PR which reverts the introduction of pytest_4 @JohnAZoidberg

jonringer
jonringer approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried tackling this myself, but disabling seems to the be by far the easiest approach because:

  • This package propagates pytest
  • Another package pytest-astropy needs to have the same version of pytest

I say just re-enable when they support pytest5

@smaret
Copy link
Member

smaret commented Sep 19, 2019 •
edited

Some of the tests in astropy fails with a Deprecation error from numpy-1.17. This has been fixed upstream, but the latest release does not include the fix yet: astropy/astropy#8935

That's another reason to disable the tests for now so we can get astropy and its dependants in 19.9.

smaret
smaret reviewed Sep 19, 2019
View reviewed changes
pkgs/development/python-modules/astropy/default.nix Outdated
@@ -23,7 +24,7 @@ buildPythonPackage rec {

propagatedBuildInputs = [ numpy pytest ]; # yes it really has pytest in install_requires

checkInputs = [ pytest pytest-astropy ];
checkInputs = [ pytest pytest-astropy bleach ];
Copy link
Member

@smaret smaret Sep 19, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'me sure we need to add bleach for the moment, since the tests are disabled anyway.

pkgs/development/python-modules/astropy/default.nix
@@ -39,6 +40,9 @@ buildPythonPackage rec {
pytest
'';

# 368 failed, 10889 passed, 978 skipped, 69 xfailed in 196.24s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to explain why these tests fails, for later reference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few reasons for the failure, however I'm not sure what they all are. That's why I'm unable to fix them.

@d-goldin
Copy link
Contributor

It was mentioned in #68964 that there are upstream patches that could possibly fix this - it might make sense to try those instead of disabling the whole test-suite.

@d-goldin
Copy link
Contributor

It would be probably also good to link this one to ZHF: #68361 for visibility, as the other PR got closed.

@JohnAZoidberg
pythonPackages.astropy: Disable tests
4c714c1 
A ton of tests fail and it's not obvious to me how to fix them.
Adding bleach to checkInputs fixes a tiny number of them, though.
@JohnAZoidberg
Copy link
Member Author

Even with the mentioned patch a lot of tests fail.

@globin globin merged commit d5a5a96 into NixOS:master Sep 24, 2019
@JohnAZoidberg JohnAZoidberg deleted the astropy-errors branch September 24, 2019 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smaret smaret smaret left review comments

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JohnAZoidberg @jonringer @Ma27 @smaret @d-goldin @globin