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

Replace the last of unittest with pytest #2467

Merged
merged 16 commits into from
Oct 6, 2018
Merged

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Oct 6, 2018

This replaces (by-and-large) any remaining unittest implementations with pytest implementations. This allows us to use fixtures on any test; the existing implementation does not let us use them on TestCase instances. It also directs future contributors away from using the legacy methods.

Would be great to merge this fairly soon, as there's some chance of merge conflicts (which git may not catch - e.g. someone writing self.assertEqual). I'll try and chase down any test breaks as soon as possible.

@pep8speaks
Copy link

pep8speaks commented Oct 6, 2018

Hello @max-sixty! Thanks for updating the PR.

Line 2436:38: E741 ambiguous variable name 'l'
Line 2436:58: E741 ambiguous variable name 'l'

Line 444:9: E741 ambiguous variable name 'I'

Line 27:9: E741 ambiguous variable name 'I'
Line 137:9: E741 ambiguous variable name 'I'
Line 157:9: E741 ambiguous variable name 'I'
Line 214:9: E741 ambiguous variable name 'I'
Line 526:5: E741 ambiguous variable name 'I'

Comment last updated on October 06, 2018 at 15:15 Hours UTC

@max-sixty
Copy link
Collaborator Author

Is @pep8speaks respecting # noqa directions? It's still complaining about ambiguous variable names here: https://github.com/max-sixty/xarray/blob/pytest/xarray/tests/test_indexing.py#L27

@@ -1532,14 +1529,14 @@ def test_encoding_chunksizes(self):


@requires_zarr
class ZarrDictStoreTest(BaseZarrTest, TestCase):
class ZarrDictStoreTest(BaseZarrTest, object):
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to include object as a second base class for these classes :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some find/replaces going awry! Corrected

@@ -65,7 +70,7 @@ def easy_array(shape, start=0, stop=1):


@requires_matplotlib
class PlotTestCase(TestCase):
class PlotTestCase(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were replacing the last of unittest here?

No worries on saving this for later, though :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this one was tougher because of all the setup methods. But I found a reasonable way to do it systematically, so now that's done

@@ -626,35 +626,35 @@ def test_unsigned_roundtrip_mask_and_scale(self):
encoded = create_encoded_unsigned_masked_scaled_data()
with self.roundtrip(decoded) as actual:
for k in decoded.variables:
self.assertEqual(decoded.variables[k].dtype,
actual.variables[k].dtype)
assert decoded.variables[k].dtype == \
Copy link
Member

Choose a reason for hiding this comment

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

I prefer using parentheses to explicit line continuations (per PEP8), but I'm going to try to avoid being picky about it until we have automated tools that complain about it :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree on the principle. I corrected a bunch of these where I could systematically, but still a few scattered around the repo.

@shoyer
Copy link
Member

shoyer commented Oct 6, 2018

we can probably go ahead and safely merge this in its current state -- none of my comments are deal breakers

@max-sixty
Copy link
Collaborator Author

I added a commit to clear up some of the issues that @shoyer found.

Assuming tests pass, I'll merge shortly given risk of conflicts - please lmk any final comments. I'm also happy to do any flups after merge this weekend.

@max-sixty max-sixty mentioned this pull request Oct 6, 2018
1 task
@max-sixty max-sixty merged commit bb87a94 into pydata:master Oct 6, 2018
@max-sixty max-sixty deleted the pytest branch October 6, 2018 17:09
@shoyer
Copy link
Member

shoyer commented Oct 6, 2018 via email

dcherian pushed a commit to dcherian/xarray that referenced this pull request Oct 7, 2018
* master:
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  switch travis language to generic (pydata#2432)
dcherian pushed a commit to maahn/xarray that referenced this pull request Oct 10, 2018
* master: (51 commits)
  xarray.backends refactor (pydata#2261)
  Fix indexing error for data loaded with open_rasterio (pydata#2456)
  Properly support user-provided norm. (pydata#2443)
  pep8speaks (pydata#2462)
  isort (pydata#2469)
  tests shoudn't need to pass for a PR (pydata#2471)
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Update whats-new.rst (pydata#2466)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  ...
dcherian pushed a commit to dcherian/xarray that referenced this pull request Oct 10, 2018
* master: (21 commits)
  xarray.backends refactor (pydata#2261)
  Fix indexing error for data loaded with open_rasterio (pydata#2456)
  Properly support user-provided norm. (pydata#2443)
  pep8speaks (pydata#2462)
  isort (pydata#2469)
  tests shoudn't need to pass for a PR (pydata#2471)
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Update whats-new.rst (pydata#2466)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  ...
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.

3 participants