Repository: Materials-Consortia/optimade-python-tools
Pull request #665 (2021-01-06 15:29:18)

Validator changes: always check unversioned '/versions' and handle rich HTML pages
This PR does two things:
- Some services e.g. netlify have rich 404 pages which yield unhelpful error messages, those error messages are now improved.
- Following discussion in #662, this PR always checks the existence of the unversioned `/versions` endpoint, even when passed a versioned URL

codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=h1) Report
> Merging [#665](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=desc) (ccb9cb5) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/790ab7de2efc17b2a5f16b9024590075e503ceed?el=desc) (790ab7d) will **decrease** coverage by `0.08%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665/graphs/tree.svg?width=650&height=150&src=pr&token=UJAtmqkZZO)](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
- Coverage   93.45%   93.36%   -0.09%     
==========================================
  Files          61       61              
  Lines        3299     3302       +3     
==========================================
  Hits         3083     3083              
- Misses        216      219       +3     
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `93.36% <0.00%> (-0.09%)` | :arrow_down: |
| validator | `66.05% <0.00%> (-0.07%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [optimade/validator/validator.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665/diff?src=pr&el=tree#diff-b3B0aW1hZGUvdmFsaWRhdG9yL3ZhbGlkYXRvci5weQ==) | `82.61% <0.00%> (-0.54%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=footer). Last update [790ab7d...ccb9cb5](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/665?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ml-evs: Changed to low-priority as this is only an issue in cases where the validator *should* fail anyway, i.e. this PR is just an error message improvement, initially we thought it was bug fix that it was failing *because* of this 

Patch:
diff --git a/optimade/validator/utils.py b/optimade/validator/utils.py
index ec07afbc..0f02604b 100644
--- a/optimade/validator/utils.py
+++ b/optimade/validator/utils.py
@@ -289,13 +289,7 @@ def test_case(test_fn: Callable[[Any], Tuple[Any, str]]):
                 else:
                     result, msg = test_fn(validator, *args, **kwargs)
 
-            except json.JSONDecodeError as exc:
-                msg = (
-                    "Critical: unable to parse server response as JSON. "
-                    f"{exc.__class__.__name__}: {exc}"
-                )
-                raise exc
-            except (ResponseError, ValidationError) as exc:
+            except (json.JSONDecodeError, ResponseError, ValidationError) as exc:
                 msg = f"{exc.__class__.__name__}: {exc}"
                 raise exc
             except Exception as exc:
diff --git a/optimade/validator/validator.py b/optimade/validator/validator.py
index 3255b6ad..f67dd00c 100644
--- a/optimade/validator/validator.py
+++ b/optimade/validator/validator.py
@@ -922,18 +922,24 @@ class ImplementationValidator:
         for versioned base URLs.
 
         """
-        expected_status_code = 200
+
+        # First, check that there is a versions endpoint in the appropriate place:
+        # If passed a versioned URL, then strip that version from
+        # the URL before looking for `/versions`.
         if re.match(VERSIONS_REGEXP, self.base_url_parsed.path) is not None:
-            expected_status_code = 404
+            self.client.base_url = "/".join(self.client.base_url.split("/")[:-1])
 
         response, _ = self._get_endpoint(
-            CONF.versions_endpoint, expected_status_code=expected_status_code
+            CONF.versions_endpoint, expected_status_code=200
         )
 
-        if expected_status_code == 200:
-            self._test_versions_endpoint_content(
-                response, request=CONF.versions_endpoint
-            )
+        self._test_versions_endpoint_content(response, request=CONF.versions_endpoint)
+
+        # If passed a versioned URL, first reset the URL of the client to the
+        # versioned one, then that this versioned URL does NOT host a versions endpoint
+        if re.match(VERSIONS_REGEXP, self.base_url_parsed.path) is not None:
+            self.client.base_url = self.base_url
+            self._get_endpoint(CONF.versions_endpoint, expected_status_code=404)
 
     @test_case
     def _test_versions_endpoint_content(
@@ -1212,9 +1218,12 @@ class ImplementationValidator:
 
         if response.status_code not in expected_status_code:
             message = f"Request to '{request_str}' returned HTTP code {response.status_code} and not expected {expected_status_code}."
-            message += "\nError(s):"
-            for error in response.json().get("errors", []):
-                message += f'\n  {error.get("title", "N/A")}: {error.get("detail", "N/A")} ({error.get("source", {}).get("pointer", "N/A")})'
+            message += "\nAdditional details:"
+            try:
+                for error in response.json().get("errors", []):
+                    message += f'\n  {error.get("title", "N/A")}: {error.get("detail", "N/A")} ({error.get("source", {}).get("pointer", "N/A")})'
+            except json.JSONDecodeError:
+                message += f"\n  Could not parse response as JSON. Content type was {response.headers.get('content-type')!r}."
             raise ResponseError(message)
 
         return response, f"received expected response: {response}."


Test patch:
diff --git a/tests/validator/test_utils.py b/tests/validator/test_utils.py
index 5b69199e..5c9e983e 100644
--- a/tests/validator/test_utils.py
+++ b/tests/validator/test_utils.py
@@ -164,17 +164,14 @@ def test_expected_failure_test_case():
     assert validator.results.internal_failure_count == 0
 
     assert output[0] is None
-    assert (
-        output[1]
-        == "Critical: unable to parse server response as JSON. JSONDecodeError: Dummy JSON error: line 1 column 1 (char 0)"
-    )
+    assert output[1] == "JSONDecodeError: Dummy JSON error: line 1 column 1 (char 0)"
     assert (
         validator.results.optional_failure_messages[-1][0]
         == "http://example.org/test_request - dummy_test_case - failed with error"
     )
     assert (
         validator.results.optional_failure_messages[-1][1]
-        == "Critical: unable to parse server response as JSON. JSONDecodeError: Dummy JSON error: line 1 column 1 (char 0)"
+        == "JSONDecodeError: Dummy JSON error: line 1 column 1 (char 0)"
     )