Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/iris/src/whatsnew/3.0.4.rst
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ This document explains the changes made to Iris for this release
to allow use of the latest versions of `cftime`_, `cf-units`_ and `nc-time-axis`_.
(:pull:`4222`)

#. `@rcomer`_ modified test modules so they run consistently under ``pytest`` and
``nose``, and also fixed some minor issues with :class:`~iris.time.PartialDateTime`.
(:pull:`4249`)

Note that, we are forced to drop support for ``Python 3.6`` in this patch due to
the third-party package dependencies required by (:pull:`4222`).

Expand Down
4 changes: 2 additions & 2 deletions lib/iris/tests/system_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@


class SystemInitialTest(tests.IrisTest):
def system_test_supported_filetypes(self):
def test_supported_filetypes(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" or it just won't discover them.

nx, ny = 60, 60
data = np.arange(nx * ny, dtype=">f4").reshape(nx, ny)

Expand Down Expand Up @@ -74,7 +74,7 @@ def horiz_cs():
new_cube, ("system", "supported_filetype_%s.cml" % filetype)
)

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" or it just won't discover them

if tests.MPL_AVAILABLE:
import matplotlib # noqa
import netCDF4 # noqa
Expand Down
6 changes: 3 additions & 3 deletions lib/iris/tests/test_merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import iris.tests.stock


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.

"""
Mix-in class for attributes & utilities common to these test cases.

Expand Down Expand Up @@ -55,15 +55,15 @@ def test_duplication(self):


@tests.skip_data
class TestSingleCube(tests.IrisTest, TestMixin):
class TestSingleCube(tests.IrisTest, MergeMixin):
def setUp(self):
self._data_path = tests.get_data_path(("PP", "globClim1", "theta.pp"))
self._num_cubes = 1
self._prefix = "theta"


@tests.skip_data
class TestMultiCube(tests.IrisTest, TestMixin):
class TestMultiCube(tests.IrisTest, MergeMixin):
def setUp(self):
self._data_path = tests.get_data_path(
("PP", "globClim1", "dec_subset.pp")
Expand Down
6 changes: 6 additions & 0 deletions lib/iris/tests/unit/analysis/scipy_interpolate/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Copyright Iris contributors
#
# This file is part of Iris and is released under the LGPL license.
# See COPYING and COPYING.LESSER in the root of the repository for full
# licensing details.
"""Unit tests for the :mod:`iris.analysis.scipy_interpolate` module."""
6 changes: 6 additions & 0 deletions lib/iris/tests/unit/time/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Copyright Iris contributors
#
# This file is part of Iris and is released under the LGPL license.
# See COPYING and COPYING.LESSER in the root of the repository for full
# licensing details.
"""Unit tests for the :mod:`iris.time` module."""
4 changes: 2 additions & 2 deletions lib/iris/tests/unit/time/test_PartialDateTime.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,12 @@ def setUp(self):
self.expected_value = EQ_EXPECTATIONS

def test_cftime_equal(self):
pdt = PartialDateTime(month=3, microsecond=2)
pdt = PartialDateTime(month=3, second=2)
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. The microsecond comparison is skipped if other doesn't have a microsecond attribute:

iris/lib/iris/time.py

Lines 152 to 157 in a6cd220

# 'microsecond' is optional
if result and hasattr(other, "microsecond"):
attr = self.microsecond
other_attr = other.microsecond
if attr is not None and attr != other_attr:
result = False

My guess is that, when this test was written, cftime.datetimes' predecessors did not have a microsecond attribute, but they do now.

other = cftime.datetime(year=2013, month=3, day=20, second=2)
self.assertTrue(pdt == other)

def test_cftime_not_equal(self):
pdt = PartialDateTime(month=3, microsecond=2)
pdt = PartialDateTime(month=3, second=2)
other = cftime.datetime(year=2013, month=4, day=20, second=2)
self.assertFalse(pdt == other)

Expand Down
22 changes: 10 additions & 12 deletions lib/iris/time.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,23 +155,21 @@ def __eq__(self, other):
other_attr = other.microsecond
if attr is not None and attr != other_attr:
result = False

except AttributeError:
result = NotImplemented
result = other.__eq__(self)
if result is NotImplemented:
# Equality is undefined between these objects. We don't
# want Python to fall back to the default `object`
# 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

@rcomer rcomer Jul 22, 2021

Choose a reason for hiding this comment

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

One test compares a PartialDateTime to an int, and expects a TypeError:

class _Test_operator:
def test_invalid_type(self):
pdt = PartialDateTime()
with self.assertRaises(TypeError):
self.op(pdt, 1)

It was getting False instead.

We previously used the __cmp__ method to stop the equality comparison falling back to object identities. But this method is not a thing in python3.

My above solution works, but feels like a bit of a hack. Alternative solutions welcome!

Copy link
Member

@bjlittle bjlittle Jul 22, 2021

Choose a reason for hiding this comment

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

@rcomer Rings a bell... see Unidata/cftime#81 😉

This might explain this change in behaviour, which was kinda py2 centric 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@bjlittle yup the failures I saw were just for == and != operators. So I think >=, etc. were still raising the TypeError as there's no way to check that based on identities.

Copy link
Member

Choose a reason for hiding this comment

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

@rcomer Okay, I'm fine with this.

It seems completely reasonable to me and maintains prior behaviour both within iris and for users 👍


return result

def __ne__(self, other):
result = self.__eq__(other)
if result is not NotImplemented:
result = not result
return result

def __cmp__(self, other):
# Since we've defined all the rich comparison operators (via
# functools.total_ordering), we can only reach this point if
# neither this class nor the other class had a rich comparison
# that could handle the type combination.
# We don't want Python to fall back to the default `object`
# 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)))