Validator overhaul #417

ml-evs · 2020-07-20T12:25:36Z

This PR updates the way we do validation. Instead of applying an arbitrary list of filter examples from the spec (which provide useless results), this PR adds the following:

Get the list of supported properties (and if available, their types) from the implementations /info endpoint
Compare that list to (currently) hardcoded values for the fields that "SHOULD", "MUST" and are "OPTIONALLY" supported for each endpoint (eventually we will get these from our schema models directly, see Add classes to operate on model schemas directly #277)
For each endpoint, grab a random entry (say {"int_field": 1, "string_field": "foo"}), then:
- for each property, test the values of that particular entry according to the type of the field for all appropriate operators, e.g. int_field = 1, int_field >= 1 and so on, making sure that the results are all consistent (i.e. int_field = 1 returns at most as many results as int_field >= 1).
- if the property has no reported type, do not perform any query checks
- if the property is an OPTIMADE "MUST", then error if any check fails.
- if the property is a "SHOULD" or an "OPTIONAL", then emit an optional test failure.
- in the case of a string or numeric comparison, also try reversing the order of the identifier and value (e.g. id =1 and 1 = id must return the same results).

This PR also introduces some extra changes:

deserialization of endpoints into our models is now OPTIONAL, until we can properly validate models with missing optional fields.
improved @test_case decorator to allow tests to be ignored after running (if they turned out to be unsuitable), decluttering the output by adding the concept of a multi-stage test that only reports the last step
adds tests for the @test_case decorator
entry_info is now checked to ensure that all "MUST" properties are present
closes Test data_returned #402 by testing that data_returned matches data_available when there's no filter
closes Nonexistent property 'list' in validator tests #423 by disabling all of the old scraped filter example tests, using the dynamic ones instead
closes Validate bad version URLs responding with 553 Version Not Supported #427 by checking that queries to the dummy version "v123123" return 553
closes Validator should validate versions endpoint #491 by validating the versions endpoint

There are still many things missing from this PR, but I think its at a useful stage (and will require some effort to review...):

Still missing:

.github/workflows/deps_lint.yml

CasperWA · 2020-07-20T12:43:41Z

Yay! This PR is all I've ever dreamed of... 🤩

codecov · 2020-07-20T13:18:03Z

Codecov Report

Merging #417 into master will decrease coverage by 0.18%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   91.66%   91.48%   -0.19%     
==========================================
  Files          60       61       +1     
  Lines        2856     3065     +209     
==========================================
+ Hits         2618     2804     +186     
- Misses        238      261      +23

Flag	Coverage Δ
#project	`91.48% <89.47%> (?)`
#unittests	`?`
#validator	`63.29% <74.14%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/validator/__init__.py	`9.37% <0.00%> (ø)`
optimade/validator/validator.py	`82.18% <88.57%> (+5.08%)`	⬆️
optimade/validator/utils.py	`90.15% <90.15%> (ø)`
optimade/validator/config.py	`100.00% <100.00%> (ø)`
optimade/validator/data/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21e3ab7...060da80. Read the comment docs.

ml-evs · 2020-07-20T13:18:12Z

Yay! This PR is all I've ever dreamed of... star_struck

Give it a while... having to unpick my code from a couple of weeks ago from the other PRs we've merged recently.

.github/workflows/deps_lint.yml

CasperWA

Some very few comments.

There are a lot of new stuff (it seems to me) in the validator, which is absolutely awesome!
But maybe we can have a live walkthrough at some point, perhaps via Zoom or some such?

optimade/validator/validator.py

ml-evs · 2020-09-03T15:59:34Z

Some very few comments.

There are a lot of new stuff (it seems to me) in the validator, which is absolutely awesome!
But maybe we can have a live walkthrough at some point, perhaps via Zoom or some such?

Yeah, I'm happy to. I think at the very least I need to write a fat module docstring and some notes on how to extend it...

optimade/validator/validator.py

CasperWA

This is looking very clean now, great work @ml-evs !

From the new fancy in-line codecov warnings, it seems there are a lot of code-blocks that are not being checked in the test_case function, i.e., we should probably have a static test file that represents a response with some errors?

Also, there are a lot of stuff in the validator file. I have tried to pick out some nit-picky stuff (like the doc-strings, type-info, etc.), which should be applied in a more broader sense throughout the file.
Furthermore, I am unsure whether I have actually found all I wanted.

I think it's a good idea to check which lines are not covered in the coverage tests and attempt to add some mock-responses, e.g., static files that should test these various lines?

optimade/validator/config.py

optimade/validator/utils.py

optimade/validator/validator.py

optimade/validator/config.py

optimade/validator/utils.py

optimade/validator/validator.py

CasperWA

Added some doc strings for the tests.

tests/validator/test_utils.py

CasperWA

Lot's of nitpicking here - mainly doc strings and such.

After this has been addressed, I'd say this should be merged.

Again, great job here @ml-evs !

optimade/validator/validator.py

ml-evs · 2020-09-10T17:18:27Z

I think that's all your docstring changes now @CasperWA, plus the SystemExit stuff in 60772e5 that I mentioned, and the changes to make most of the ImplementationValidator private. Once you're happy, I'll do a final rebase that squashes the last few commits and leaves us with about 15 to rebase merge into master?

CasperWA

Last change I just spotted.
After that I'll approve and you can merge.

If you wish to convert this PR to several commits, instead of a single, I think that's fair enough. E.g., one for the move separately and then the updates?

tests/validator/test_utils.py

- Split validator into validator and utils submodules - Add validator specific-coverage report - Disable parsing filter examples from specification for validation - Removed `optimade.validator.data` submodule - Removed corresponding invoke task - Removed tests from validator - Minor tweaks, function renames and update docs - Added dynamic filters of every field based on type reported in entry info - Add another test client that fakes HTTP exceptions - Created optimade.validator.config module - Updates to @test_case decorator, and test for MUST fields in entry info - Added test to check that all MUST properties are present in entry info - Allow test cases to return None to indicate that the test should be ignored. The use case here is e.g. if an optional field is missing, then we don't care and the test can be ignored - Added `multistage` option to `@test_case` that suppresses output and counters of success on multistage tests - Added hard filter to running optional tests in @test_case itself - Made deserialization of multi-entry endpoints optional

- Test bad version responds with 553 (closes #427) - Add validation of versions endpoint (closes #491) - Add test for matching data_available and data_returned (closes #402)

- Improved module docstrings - Fixed hardcoded endpoint in logging - Updated base info error message - Removed extraneous base info attribute - Tweaked endpoint variables and added missing logging - Make wider use of config values in error messages - Tidied some function docstrings - Remove argument from test_versions - Added middle ground verbosity - Better handling of fatal errors (SystemExit) in the test_case decorator - Standardization of test_case API - Unify test_case decorator and wrapped method API by returning string summary in both cases - Updated all type hints and docstrings - Slightly refactored test_case decorator for clearer exception handling - Add validator.utils tests for @test_case decorator - Remove "Keyword arguments" from docstrings - Made many validator methods/attributes private

CasperWA

Great job with this @ml-evs !

ml-evs · 2020-09-11T13:03:29Z

Great job with this @ml-evs !

Always improved by your reviews 👍

CasperWA reviewed Jul 20, 2020

View reviewed changes

.github/workflows/deps_lint.yml Outdated Show resolved Hide resolved

ml-evs mentioned this pull request Jul 22, 2020

Nonexistent property 'list' in validator tests #423

Closed

ml-evs changed the title ~~[WIP] validator overhaul~~ Validator overhaul Sep 3, 2020

ml-evs marked this pull request as ready for review September 3, 2020 13:14

ml-evs commented Sep 3, 2020

View reviewed changes

.github/workflows/deps_lint.yml Outdated Show resolved Hide resolved

CasperWA reviewed Sep 3, 2020

View reviewed changes

optimade/validator/validator.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Outdated Show resolved Hide resolved

This was linked to issues Sep 3, 2020

Nonexistent property 'list' in validator tests #423

Closed

Validate bad version URLs responding with 553 Version Not Supported #427

Closed

ml-evs commented Sep 4, 2020

View reviewed changes

optimade/validator/validator.py Outdated Show resolved Hide resolved

ml-evs mentioned this pull request Sep 8, 2020

Validator should validate versions endpoint #491

Closed

ml-evs requested a review from CasperWA September 9, 2020 01:24

CasperWA requested changes Sep 9, 2020

View reviewed changes

CasperWA reviewed Sep 9, 2020

View reviewed changes

optimade/validator/config.py Show resolved Hide resolved

CasperWA reviewed Sep 9, 2020

View reviewed changes

optimade/validator/utils.py Outdated Show resolved Hide resolved

optimade/validator/utils.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Outdated Show resolved Hide resolved

ml-evs requested a review from CasperWA September 9, 2020 18:04

CasperWA requested changes Sep 10, 2020

View reviewed changes

ml-evs requested a review from CasperWA September 10, 2020 18:09

CasperWA requested changes Sep 11, 2020

View reviewed changes

tests/validator/test_utils.py Outdated Show resolved Hide resolved

CasperWA previously approved these changes Sep 11, 2020

View reviewed changes

ml-evs dismissed CasperWA’s stale review via ce867f0 September 11, 2020 12:31

ml-evs added 3 commits September 11, 2020 13:32

Validate versions endpoint and add extra test on meta->data_returned

7ea3f95

- Test bad version responds with 553 (closes #427) - Add validation of versions endpoint (closes #491) - Add test for matching data_available and data_returned (closes #402)

ml-evs requested a review from CasperWA September 11, 2020 12:37

CasperWA approved these changes Sep 11, 2020

View reviewed changes

ml-evs merged commit d53ab89 into Materials-Consortia:master Sep 11, 2020

ml-evs deleted the ml-evs/validator_overhaul branch September 11, 2020 13:03

ml-evs mentioned this pull request Sep 11, 2020

Improving the validator #357

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validator overhaul #417

Validator overhaul #417

ml-evs commented Jul 20, 2020 •

edited

CasperWA commented Jul 20, 2020

codecov bot commented Jul 20, 2020 •

edited

ml-evs commented Jul 20, 2020

CasperWA left a comment

ml-evs commented Sep 3, 2020

CasperWA left a comment

CasperWA left a comment

CasperWA left a comment

ml-evs commented Sep 10, 2020

CasperWA left a comment

CasperWA left a comment

ml-evs commented Sep 11, 2020

Validator overhaul #417

Validator overhaul #417

Conversation

ml-evs commented Jul 20, 2020 • edited

CasperWA commented Jul 20, 2020

codecov bot commented Jul 20, 2020 • edited

Codecov Report

ml-evs commented Jul 20, 2020

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Sep 3, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Sep 10, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Sep 11, 2020

ml-evs commented Jul 20, 2020 •

edited

codecov bot commented Jul 20, 2020 •

edited