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

pytest-ification #1828

Merged
merged 41 commits into from
Jan 15, 2018
Merged

pytest-ification #1828

merged 41 commits into from
Jan 15, 2018

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Jan 14, 2018

So far, only the changes that unittest2pytest could fix. Need to work out how to sort through the exisiting xarray ones

EDIT: ready to merge (and keen to get this in before there are divergences from other branches)

Copy link
Member

@fujiisoup fujiisoup left a comment

Choose a reason for hiding this comment

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

Thanks @maxim-lian,
This is a great step to clean up our test suit :)

Are you going to make our tests more pytest-ized (e.g. by adopting more @pytest.mark.parametrize)?
I think the current change is a good size.

There are some test failures (maybe due to mis-import).
After they are fixed, I will review more carefully and merge this so that this change would not be outdated.

@@ -109,12 +109,6 @@ def _importorskip(modname, minversion=None):


class TestCase(unittest.TestCase):
if PY3:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can remove from xarray.core.pycompat import PY3 in line 3.

@max-sixty max-sixty force-pushed the pytest-conv branch 2 times, most recently from b5abf05 to 9fc243b Compare January 14, 2018 20:26
@max-sixty
Copy link
Collaborator Author

I think this is now passing throughout except for Python3.4, which I think is because pytest is on version 3.0.5, which doesn't have the match keyword from pytest 3.3.

Does anyone know why an old version of pytest is being installed there? From what I can see on the pytest home page, 3.4 is fully supported, and the 3.4 ci requirements don't force a lower version of pytest.

I could unravel that but if anyone has insight (and is anyone using 3.4? Though this PR prob not the place to make that decision)

I think we should try and merge this asap, as there are going to be (manageable) merge conflicts with other branches

@max-sixty max-sixty mentioned this pull request Jan 14, 2018
@max-sixty
Copy link
Collaborator Author

Are you going to make our tests more pytest-ized (e.g. by adopting more @pytest.mark.parametrize)?

Not yet, I think that's a bigger exercise, and more difficult to script

fill_value = np.nan
elif np.issubdtype(dtype, int):
elif np.issubdtype(dtype, np.signedinteger):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also apply to unsigned integers, and the resulting dtype should be given as a numpy dtype. Plus, we can make this much more efficient for small integers:

elif np.issubdtype(dtype, np.integer):
    if dtype.itemsize <= 2:
        dtype = np.float32
    else:
        dtype = np.float64
    fill_value = np.nan

@max-sixty
Copy link
Collaborator Author

Tests now pass, including flake8, apart from the pytest.warns.

Given how much code this touches, I'm keen to merge this ASAP, before divergences creep in. What's the best course on 3.4? pandas is in the process of dropping it.

Alternatively I could revert the pytest.warns changes, though there isn't one clean commit to revert.

@jhamman
Copy link
Member

jhamman commented Jan 15, 2018

@maxim-lian - I opened #1829 to discuss dropping Python 3.4.

@fujiisoup
Copy link
Member

Using pip seems to temporary solve this issue.
ci/requirements-py34.yml

name: test_env
dependencies:
  - python=3.4
  - bottleneck
  - flake8
  - pandas
  - pip:
    - coveralls
    - pytest>=3.3
    - pytest-cov

maybe not the best solution, though.

@fujiisoup
Copy link
Member

There are two comment-outed lines using self.assertEqual in test_dataarray and test_dataset.

We are still using some methods of TestCase class, such as assertVariableNotEqual.
Can we add equivalent function to testings.py?

@max-sixty
Copy link
Collaborator Author

There are two comment-outed lines using self.assertEqual in test_dataarray and test_dataset.

That are comment-out as part of this PR? Or prior? If prior, then I think we should leave until we attempt to add those back in

We are still using some methods of TestCase class, such as assertVariableNotEqual.
Can we add equivalent function to testings.py?

I think the best way of handling those is xfailing an assert_equal test, rather than having a extra -not test for each variant. I also think we should do that in a separate PR (generally am keen to get this in before there are divergences)

@max-sixty
Copy link
Collaborator Author

Using pip seems to temporary solve this issue.

Nice, thanks. I've put this in. If we're on the way to deprecate 3.4, from which source we install pytest is flexible IMHO

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for the cleanup.

@max-sixty max-sixty merged commit f3deb2f into pydata:master Jan 15, 2018
@max-sixty max-sixty deleted the pytest-conv branch January 15, 2018 22:25
@max-sixty max-sixty mentioned this pull request Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants