Repository: ESMValGroup/ESMValCore

Issue:
Tests involving `isodate` failed
```bash
=========================== short test summary info ============================
FAILED tests/integration/recipe/test_check.py::test_valid_time_selection[19900101T12H00M00S/19920101T12H00M00] - esmvalcore.exceptions.RecipeError: Invalid value encountered for `timerange`. Valid value must follow ISO 8601 standard for dates and duration periods, or be set to '*' to load available years. Got ['19900101T12H00M00S', '19920101T12H00M00'] instead.
FAILED tests/integration/recipe/test_check.py::test_valid_time_selection[19900101TH00M00S/P2Y2M1DT12H00M00S] - esmvalcore.exceptions.RecipeError: Invalid value encountered for `timerange`. Valid value must follow ISO 8601 standard for dates and duration periods, or be set to '*' to load available years. Got ['19900101TH00M00S', 'P2Y2M1DT12H00M00S'] instead.
FAILED tests/integration/recipe/test_check.py::test_valid_time_selection[P2Y2M1D/19920101T12H00M00S] - esmvalcore.exceptions.RecipeError: Invalid value encountered for `timerange`. Valid value must follow ISO 8601 standard for dates and duration periods, or be set to '*' to load available years. Got ['P2Y2M1D', '19920101T12H00M00S'] instead.
FAILED tests/integration/recipe/test_recipe.py::test_recipe_iso_timerange[19900101TH00M00S/P2Y2M1DT12H00M00S-19900101TH00M00S-P2Y2M1DT12H00M00S] - isodate.isoerror.ISO8601Error: Unrecognised ISO 8601 date format: '19900101TH00M00S'
FAILED tests/integration/recipe/test_recipe.py::test_recipe_iso_timerange[P1Y2M1D/19920101T12H00M00S-P1Y2M1D-19920101T12H00M00S] - isodate.isoerror.ISO8601Error: Unrecognised ISO 8601 date format: '19920101T12H00M00S'
FAILED tests/integration/recipe/test_recipe.py::test_recipe_iso_timerange_as_dataset[19900101TH00M00S/P2Y2M1DT12H00M00S-19900101TH00M00S-P2Y2M1DT12H00M00S] - isodate.isoerror.ISO8601Error: Unrecognised ISO 8601 date format: '19900101TH00M00S'
FAILED tests/integration/recipe/test_recipe.py::test_recipe_iso_timerange_as_dataset[P1Y2M1D/19920101T12H00M00S-P1Y2M1D-19920101T12H00M00S] - isodate.isoerror.ISO8601Error: Unrecognised ISO 8601 date format: '19920101T12H00M00S'
= 7 failed, 3877 passed, 13 skipped, 1 xpassed, 1081 warnings in 112.33s (0:01:52) =
```

See https://app.circleci.com/pipelines/github/ESMValGroup/ESMValCore/11808/workflows/9fcf7ed9-5d01-4686-a504-6aca0941b2aa/jobs/49698.

Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/ESMValGroup/ESMValCore/pull/2546?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ESMValGroup) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 94.91%. Comparing base [(`99c6a7b`)](https://app.codecov.io/gh/ESMValGroup/ESMValCore/commit/99c6a7be1e995f8b4fcb61f3a44bd46aa828c397?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ESMValGroup) to head [(`3b0d43c`)](https://app.codecov.io/gh/ESMValGroup/ESMValCore/commit/3b0d43ce5c0396caffe0dac7e01578e8268b68f8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ESMValGroup).

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main    #2546   +/-   ##
=======================================
  Coverage   94.91%   94.91%           
=======================================
  Files         251      251           
  Lines       14301    14301           
=======================================
  Hits        13574    13574           
  Misses        727      727           
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/ESMValGroup/ESMValCore/pull/2546?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ESMValGroup).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ESMValGroup).

valeriupredoi: BRB
valeriupredoi: @schlunma cheers, Manu! Yes, it is backwards compatible bc the Circle tests still use the older `isodate=0.6.1`; if we want to, we can just pin `isodate>=0.7.0` in our env - the previous version is ancient anyhow; more pressing is you comment about the other bits where `isodate.parse_ ...` is invoked (well spotted!) - I reckon we ought to check, but am not fully sold on it
valeriupredoi: OK I've now pretty much refactored all the handling of `isodate` strings (apart from the ones in `local.py` that are correctly handled) , added a couple tests, and pinned to `isodate>=0.7.0` - the old package, apart from being very old, has some funny ways of handling parsing exceptions, and those better in the new 0.7.0, hence some of our negative tests are not compatible anymore with the old one (in essence the same fail, but contorted exceptions for 0.6.1). May goal was no not catch any iso errors and filter them through our stack, but rather, let them immediately be raised from `isodate` - lot more straightforward that way :beer: 
valeriupredoi: the [upstream dev deps test](https://app.circleci.com/pipelines/github/ESMValGroup/ESMValCore/11827/workflows/c98ca245-eb85-415a-b672-6897bf4efabf/jobs/49778?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link&utm_content=summary) fails bc of esgf-pyclient and that means I have to go there and fix whatever broke there too - not in a rush though, we can get this in no problem in the meantime, just another bit for me to look into :confounded: 
bouweandela: It may have been a network issue, re-running tests seems to have solved it.
valeriupredoi: cheers @bouweandela :beers: You here to review as well, or just just passing by :grin: 
bouweandela: Just passing by, it looks like @schlunma is doing an excellent job reviewing. 
valeriupredoi: Danke viel Manu :beer: 

Patch:
diff --git a/environment.yml b/environment.yml
index 636883f04..5cf256c22 100644
--- a/environment.yml
+++ b/environment.yml
@@ -21,7 +21,7 @@ dependencies:
   - iris >=3.10.0
   - iris-esmf-regrid >=0.11.0
   - iris-grib >=0.20.0  # github.com/ESMValGroup/ESMValCore/issues/2535
-  - isodate
+  - isodate >=0.7.0  # incompatible with very old 0.6.1
   - jinja2
   - libnetcdf !=4.9.1  # to avoid hdf5 warnings
   - nc-time-axis
diff --git a/esmvalcore/_recipe/check.py b/esmvalcore/_recipe/check.py
index 4e4fa6d2b..e4f60e9d7 100644
--- a/esmvalcore/_recipe/check.py
+++ b/esmvalcore/_recipe/check.py
@@ -366,20 +366,33 @@ def _check_delimiter(timerange):
 
 
 def _check_duration_periods(timerange):
-    try:
-        isodate.parse_duration(timerange[0])
-    except ValueError:
-        pass
-    else:
+    # isodate duration must always start with P
+    if timerange[0].startswith("P") and timerange[1].startswith("P"):
+        raise RecipeError(
+            "Invalid value encountered for `timerange`. "
+            "Cannot set both the beginning and the end "
+            "as duration periods."
+        )
+
+    if timerange[0].startswith("P"):
+        try:
+            isodate.parse_duration(timerange[0])
+        except isodate.isoerror.ISO8601Error as exc:
+            raise RecipeError(
+                "Invalid value encountered for `timerange`. "
+                f"{timerange[0]} is not valid duration according to ISO 8601."
+                + "\n"
+                + str(exc)
+            )
+    elif timerange[1].startswith("P"):
         try:
             isodate.parse_duration(timerange[1])
-        except ValueError:
-            pass
-        else:
+        except isodate.isoerror.ISO8601Error as exc:
             raise RecipeError(
                 "Invalid value encountered for `timerange`. "
-                "Cannot set both the beginning and the end "
-                "as duration periods."
+                f"{timerange[1]} is not valid duration according to ISO 8601."
+                + "\n"
+                + str(exc)
             )
 
 
@@ -391,20 +404,26 @@ def _check_format_years(date):
 
 
 def _check_timerange_values(date, timerange):
+    # Wildcards are fine
+    if date == "*":
+        return
+    # P must always be in a duration string
+    # if T in date, that is a datetime; otherwise it's date
     try:
-        isodate.parse_date(date)
-    except ValueError:
-        try:
+        if date.startswith("P"):
             isodate.parse_duration(date)
-        except ValueError as exc:
-            if date != "*":
-                raise RecipeError(
-                    "Invalid value encountered for `timerange`. "
-                    "Valid value must follow ISO 8601 standard "
-                    "for dates and duration periods, or be "
-                    "set to '*' to load available years. "
-                    f"Got {timerange} instead."
-                ) from exc
+        elif "T" in date:
+            isodate.parse_datetime(date)
+        else:
+            isodate.parse_date(date)
+    except isodate.isoerror.ISO8601Error as exc:
+        raise RecipeError(
+            "Invalid value encountered for `timerange`. "
+            "Valid value must follow ISO 8601 standard "
+            "for dates and duration periods, or be "
+            "set to '*' to load available years. "
+            f"Got {timerange} instead." + "\n" + str(exc)
+        )
 
 
 def valid_time_selection(timerange):
diff --git a/esmvalcore/preprocessor/_time.py b/esmvalcore/preprocessor/_time.py
index fd9d4c16a..522a312f5 100644
--- a/esmvalcore/preprocessor/_time.py
+++ b/esmvalcore/preprocessor/_time.py
@@ -117,17 +117,17 @@ def _parse_start_date(date):
     """Parse start of the input `timerange` tag given in ISO 8601 format.
 
     Returns a datetime.datetime object.
+
+    Raises an ISO8601 parser error if data can not be parsed.
     """
     if date.startswith("P"):
         start_date = isodate.parse_duration(date)
+    elif "T" in date:
+        start_date = isodate.parse_datetime(date)
     else:
-        try:
-            start_date = isodate.parse_datetime(date)
-        except isodate.isoerror.ISO8601Error:
-            start_date = isodate.parse_date(date)
-            start_date = datetime.datetime.combine(
-                start_date, datetime.time.min
-            )
+        start_date = isodate.parse_date(date)
+        start_date = datetime.datetime.combine(start_date, datetime.time.min)
+
     return start_date
 
 
@@ -135,6 +135,8 @@ def _parse_end_date(date):
     """Parse end of the input `timerange` given in ISO 8601 format.
 
     Returns a datetime.datetime object.
+
+    Raises an ISO8601 parser error if data can not be parsed.
     """
     if date.startswith("P"):
         end_date = isodate.parse_duration(date)
@@ -145,9 +147,9 @@ def _parse_end_date(date):
             month, year = get_next_month(int(date[4:]), int(date[0:4]))
             end_date = datetime.datetime(year, month, 1, 0, 0, 0)
         else:
-            try:
+            if "T" in date:
                 end_date = isodate.parse_datetime(date)
-            except isodate.ISO8601Error:
+            else:
                 end_date = isodate.parse_date(date)
                 end_date = datetime.datetime.combine(
                     end_date, datetime.time.min
@@ -268,6 +270,8 @@ def clip_timerange(cube: Cube, timerange: str) -> Cube:
     ------
     ValueError
         Time ranges are outside the cube's time limits.
+    isodate.isoerror.ISO8601Error:
+        Start/end times can not be parsed by isodate.
 
     """
     start_date = _parse_start_date(timerange.split("/")[0])
diff --git a/setup.py b/setup.py
index 15ea79aae..83c363442 100755
--- a/setup.py
+++ b/setup.py
@@ -40,7 +40,7 @@ REQUIREMENTS = {
         "geopy",
         "humanfriendly",
         "iris-grib>=0.20.0",  # github.com/ESMValGroup/ESMValCore/issues/2535
-        "isodate",
+        "isodate>=0.7.0",  # incompatible with very old 0.6.1
         "jinja2",
         "nc-time-axis",  # needed by iris.plot
         "nested-lookup",


Test patch:
diff --git a/tests/integration/recipe/test_check.py b/tests/integration/recipe/test_check.py
index b118162c1..d168a3e01 100644
--- a/tests/integration/recipe/test_check.py
+++ b/tests/integration/recipe/test_check.py
@@ -122,15 +122,15 @@ GOOD_TIMERANGES = [
     "*",
     "1990/1992",
     "19900101/19920101",
-    "19900101T12H00M00S/19920101T12H00M00",
+    "19900101T120000/19920101T120000",
     "1990/*",
     "*/1992",
     "1990/P2Y",
     "19900101/P2Y2M1D",
-    "19900101TH00M00S/P2Y2M1DT12H00M00S",
+    "19900101T0000/P2Y2M1DT12H00M00S",
     "P2Y/1992",
     "P2Y2M1D/19920101",
-    "P2Y2M1D/19920101T12H00M00S",
+    "P2Y2M1D/19920101T120000",
     "P2Y/*",
     "P2Y2M1D/*",
     "P2Y21DT12H00M00S/*",
@@ -159,13 +159,28 @@ BAD_TIMERANGES = [
         "199035345/19923463164526",
         "Invalid value encountered for `timerange`. Valid value must follow "
         "ISO 8601 standard for dates and duration periods, or be set to '*' "
-        "to load available years. Got ['199035345', '19923463164526'] instead.",
+        "to load available years. Got ['199035345', '19923463164526'] instead.\n"
+        "Unrecognised ISO 8601 date format: '199035345'",
     ),
     (
         "P11Y/P42Y",
         "Invalid value encountered for `timerange`. Cannot set both "
         "the beginning and the end as duration periods.",
     ),
+    (
+        "P11X/19923463164526",
+        "Invalid value encountered for `timerange`. "
+        "P11X is not valid duration according to ISO 8601.\n"
+        "ISO 8601 time designator 'T' missing. "
+        "Unable to parse datetime string '11X'",
+    ),
+    (
+        "19923463164526/P11X",
+        "Invalid value encountered for `timerange`. "
+        "P11X is not valid duration according to ISO 8601.\n"
+        "ISO 8601 time designator 'T' missing. "
+        "Unable to parse datetime string '11X'",
+    ),
 ]
 
 
diff --git a/tests/integration/recipe/test_recipe.py b/tests/integration/recipe/test_recipe.py
index 75bed13e9..ae1ff9b5b 100644
--- a/tests/integration/recipe/test_recipe.py
+++ b/tests/integration/recipe/test_recipe.py
@@ -740,12 +740,12 @@ TEST_ISO_TIMERANGE = [
     ("1990/P2Y", "1990-P2Y"),
     ("19900101/P2Y2M1D", "19900101-P2Y2M1D"),
     (
-        "19900101TH00M00S/P2Y2M1DT12H00M00S",
-        "19900101TH00M00S-P2Y2M1DT12H00M00S",
+        "19900101T0000/P2Y2M1DT12H00M00S",
+        "19900101T0000-P2Y2M1DT12H00M00S",
     ),
     ("P2Y/1992", "P2Y-1992"),
     ("P1Y2M1D/19920101", "P1Y2M1D-19920101"),
-    ("P1Y2M1D/19920101T12H00M00S", "P1Y2M1D-19920101T12H00M00S"),
+    ("P1Y2M1D/19920101T120000", "P1Y2M1D-19920101T120000"),
     ("P2Y/*", "P2Y-2019"),
     ("P2Y2M1D/*", "P2Y2M1D-2019"),
     ("P2Y21DT12H00M00S/*", "P2Y21DT12H00M00S-2019"),
diff --git a/tests/unit/preprocessor/_time/test_time.py b/tests/unit/preprocessor/_time/test_time.py
index e8ddb5aae..9ce5e008b 100644
--- a/tests/unit/preprocessor/_time/test_time.py
+++ b/tests/unit/preprocessor/_time/test_time.py
@@ -12,6 +12,7 @@ import iris.coord_categorisation
 import iris.coords
 import iris.exceptions
 import iris.fileformats
+import isodate
 import numpy as np
 import pytest
 from cf_units import Unit
@@ -486,6 +487,24 @@ class TestClipTimerange(tests.Test):
                 coord_name
             )
 
+    def test_clip_timerange_start_date_invalid_isodate(self):
+        cube = self._create_cube(
+            [[[[0.0, 1.0]]]], [150.0], [[0.0, 365.0]], "standard"
+        )
+        with pytest.raises(isodate.isoerror.ISO8601Error) as exc:
+            clip_timerange(cube, "1950010101/1950")
+        mssg = "Unrecognised ISO 8601 date format: '1950010101'"
+        assert mssg in str(exc)
+
+    def test_clip_timerange_end_date_invalid_isodate(self):
+        cube = self._create_cube(
+            [[[[0.0, 1.0]]]], [150.0], [[0.0, 365.0]], "standard"
+        )
+        with pytest.raises(isodate.isoerror.ISO8601Error) as exc:
+            clip_timerange(cube, "1950/1950010101")
+        mssg = "Unrecognised ISO 8601 date format: '1950010101'"
+        assert mssg in str(exc)
+
 
 class TestExtractSeason(tests.Test):
     """Tests for extract_season."""