Harden validator for partially implemented APIs #1181
Conversation
- Remove CLI control of page_limit and set the default value to something less common (4) - Check `next` links for duplicates during pagination
- Make sure optional queries don't do an 'allowed failure' before more important non-optional checks - Assume that the archetypal entry will always be on the first page of results (assumes constant default sorting from the API) rather than allowing bad results to hide in future pages
ml-evs
added
priority/high
|
I think this is fairly major, if tests pass I will self-merge my changes and release so that the dashboard is up to date before the workshop. |
| @@ -825,8 +823,48 @@ def _construct_single_property_filters( | |||
| else: | |||
| num_data_returned[operator] = response["meta"].get("data_returned") | |||
|
|
|||
| if prop in CONF.unique_properties and operator == "=": | |||
There was a problem hiding this comment.
This code block was just moved up from below the reversed query tests, with this line being the only change (previously it assumed "=" was already in the dict).
Codecov Report
@@ Coverage Diff @@
## master #1181 +/- ##
==========================================
- Coverage 92.42% 92.40% -0.02%
==========================================
Files 68 68
Lines 3909 3912 +3
==========================================
+ Hits 3613 3615 +2
- Misses 296 297 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| response, message = self._get_endpoint( | ||
| f"{endp}?page_offset={random.randint(0, data_returned-1)}&response_fields={','.join(properties)}", | ||
| multistage=True, | ||
| archetypal_entry = response.json()["data"][ |
There was a problem hiding this comment.
This change removes the need for page_offset to be implemented by the API, and instead just chooses a random result from the first page of the API results to use for the filter tests.
| @@ -557,7 +555,7 @@ def _check_response_fields(self, endp: str, fields: List[str]) -> Tuple[bool, st | |||
| test_query = f"{endp}?response_fields={','.join(subset_fields)}&page_limit=1" | |||
| response, _ = self._get_endpoint(test_query, multistage=True) | |||
|
|
|||
| if response and len(response.json()["data"]) == 1: | |||
| if response and len(response.json()["data"]) >= 0: | |||
There was a problem hiding this comment.
This was previously allowing databases that did not support page_limit to skip this test
| if next_link in previous_links: | ||
| raise ResponseError( | ||
| f"The next link {next_link} has been provided already for a previous page." | ||
| ) | ||
| previous_links.add(next_link) | ||
|
|
There was a problem hiding this comment.
Previously, it was valid to return the same next link for every page and get stuck in a cycle
|
Just added some comments summarizing the changes in this PR, for posterity. |
Closes #1180 by hardening some of the validator testing, namely:
response_fieldtests ifpage_limithas no effectpage_limit(4 instead of 5) so that tests cannot accidentally pass.page_offsetbeing implemented when choosing the archetypal entry for filtering