Skip to content

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Jun 11, 2021

🚀 Pull Request

Description

I tried pointing pytest at Iris's tests and got 3 errors and 3 failures. The errors were easy to fix (see comment on test_merge.py). The failures are genuine, and it seems that test_PartialDateTime has not been running under setuptools or nox. I've added an __init__.py file to get those tests running. Now we need to decide whether PartialDateTime needs fixing or its tests need changing. The failures are:

  • PartialDateTime() == 1 now returns False but a TypeError is expected.
  • PartialDateTime() != 1 now returns True but a TypeError is expected.

As far as I can tell, PartialDateTime.__eq__ is returning NotImplemented in the above.

  • PartialDateTime(month=3, microsecond=2) == cftime.datetime(year=2013, month=3, day=20, second=2) now returns False but True is expected.

The microsecond is ignored if other doesn't have that attribute. Did cftime.datetime gain a microsecond attribute at some point?

Anyone have an opinion on what the desired behaviour should be for the above cases?


Consult Iris pull request check list



class TestMixin:
class MergeMixin:
Copy link
Member Author

Choose a reason for hiding this comment

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

pytest tried to run this as a test class, so errored all three tests when it couldn't find _data_path. Removing "Test" from the class name stops pytest trying to do that.

@rcomer rcomer marked this pull request as draft June 11, 2021 16:16
@rcomer rcomer mentioned this pull request Jun 11, 2021
@rcomer
Copy link
Member Author

rcomer commented Jun 15, 2021

Found and added another missing __init__.py.

@rcomer
Copy link
Member Author

rcomer commented Jun 15, 2021

Now setuptools and nox are running two more tests than pytest.

@rcomer
Copy link
Member Author

rcomer commented Jun 15, 2021

Looks like it was the system tests that pytest wasn't discovering. Because the test case names started with system instead of test.

@rcomer
Copy link
Member Author

rcomer commented Jun 16, 2021

pytest, setuptools and nox are now all producing the same numbers of passes, fails and skips. I did some pattern matching on the pytest and setuptools output which verified that all but 13 of the passing tests are the same. Where test cases have docstrings, setuptools prints the docstring rather then the path, so it's hard to automatically match up. It seems a pretty good bet to me that these 13 unmatched cases are the same though. Same can be said for the 22 skips.

Back to the failures described in the OP: I think PartialDateTime() == 1 probably should produce an error, as it doesn't make sense to constrain a coordinate with a PartialDateTime if the coordinate points are not datetime-like. I don't know how to implement that though - if I've understood, the comparison is defaulting back to checking whether the object identities are the same. How would you override that?

@rcomer
Copy link
Member Author

rcomer commented Jun 16, 2021

the comparison is defaulting back to checking whether the object identities are the same. How would you override that?

We previously used the cmp method to raise the error. But that is not a thing in python3.

# behaviour (which compares using object IDs), so we raise
# an exception here instead.
fmt = "unable to compare PartialDateTime with {}"
raise TypeError(fmt.format(type(other)))
Copy link
Member Author

Choose a reason for hiding this comment

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

This works but feels like a bit of a hack. Alternative solutions welcome!

@rcomer rcomer marked this pull request as ready for review June 16, 2021 14:24

class SystemInitialTest(tests.IrisTest):
def system_test_supported_filetypes(self):
def test_supported_filetypes(self):
Copy link
Member Author

@rcomer rcomer Jun 16, 2021

Choose a reason for hiding this comment

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

pytest requires test names to start with "test" or it just won't discover them

)

def system_test_imports_general(self):
def test_imports_general(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

pytest requires test names to start with "test"

pdt = PartialDateTime(month=3, microsecond=2)
pdt = PartialDateTime(month=3, second=2)
other = cftime.datetime(year=2013, month=3, day=20, second=2)
self.assertTrue(pdt == other)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was failing because the equality operator returned False. Possibly cftime.datetimes (or their predecessors) did not used to have a microseconds, but they do now.

@bjlittle bjlittle self-assigned this Jun 29, 2021
@bjlittle
Copy link
Member

@rcomer I'm trying to curate the v3.0.x branch into shape for a v3.0.4 release.

There are a few PRs that need to land in the v3.0.x release feature branch, most notably #4222. Once that happens I'd be keen to see this PR target v3.0.x instead of main and rebased, as I'd be keen to see this PR as part of the v3.0.4 release.

@rcomer rcomer mentioned this pull request Jul 22, 2021
@rcomer
Copy link
Member Author

rcomer commented Jul 22, 2021

@bjlittle please see #4249, which targets the v3.0.x branch.

@rcomer rcomer closed this Jul 22, 2021
@rcomer rcomer deleted the fix-with-pytest branch September 24, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants