Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic valid and invalid tests for the json schema #141

Merged

Conversation

jorisvandenbossche
Copy link
Collaborator

Closes #135 (rework of #134 to focus on json schema for now)

@jorisvandenbossche jorisvandenbossche changed the title Add basis valid and invalid tests for the json schema Add basic valid and invalid tests for the json schema Nov 7, 2022
@jorisvandenbossche
Copy link
Collaborator Author

cc @m-mohr do you think this is a sensible approach to test the json schema? (ensure it passes when it should and errors when it should)
Feedback would be very welcome (I never did this before)

@m-mohr
Copy link
Collaborator

m-mohr commented Nov 7, 2022

I'll have a look in the next days :-)

@jorisvandenbossche
Copy link
Collaborator Author

jorisvandenbossche commented Nov 21, 2022

OK, I updated this PR for the latest changes in the spec and updated to use pytest. This should be ready for review.

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 5, 2022

Looks reasonable to me. Why commit the test files when you can always generate them on the fly though? May lead to PRs where people update them by hand...

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 5, 2022

I'd like to propose some additional checks:

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["invalid_column_object"] = {"foo": "bar"}
invalid_cases["invalid_column_object"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["bbox"] = [0,0,0]
invalid_cases["3_element_bbox"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["bbox"] = [0,0,0,0]
valid_cases["4_element_bbox"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["bbox"] = ["0","0","0","0"]
invalid_cases["bbox_invalid_type"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["bbox"] = [0,0,0,0,0]
invalid_cases["5_element_bbox"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["bbox"] = [0.0,0.0,0.0,0.0,0.0,0.0]
valid_cases["6_element_bbox"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["bbox"] = [0,0,0,0,0,0,0]
invalid_cases["7_element_bbox"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"][""] = metadata["columns"]["geometry"]
invalid_cases["empty_column_name"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["primary_geometry"] = ""
invalid_cases["empty_primary_geometry"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["geometry_types"] = ["Point", "Point"]
invalid_cases["geometry_type_uniqueness"] = metadata

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["geometry"]["geometry_types"] = ["PointZ"]
invalid_cases["geometry_type_missing_space"] = metadata

Some of these may fail with the existing JSON Schema and may need to be added in #131, where we can fix this.
I could also add them all in #131.
Now that #131 was merged, you should be able to just add them here.

@jorisvandenbossche
Copy link
Collaborator Author

@m-mohr thanks for those additional cases! This one I don't fully understand what it is testing for:

metadata = copy.deepcopy(metadata_template)
metadata["columns"]["primary_geometry"] = ""
invalid_cases["empty_primary_geometry"] = metadata


metadata = copy.deepcopy(metadata_template)
metadata["columns"] = {}
invalid_cases["missing_columns_entry"] = metadata
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is still failing (it is xfailed for now to have the tests passing):

E           AssertionError: This is an invalid GeoParquet file, but no validation error occurred for 'missing_columns_entry':
E           {
E             "geo": {
E               "columns": {},
E               "primary_column": "geometry",
E               "version": "0.5.0-dev"
E             }
E           }

But I suppose that should be easy to solve to require a minimum length of one for columns?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. We can fix that, sure. Just add "minProperties": 1, in the columns schema.
By the way, the written spec doesn't forbid an empty column object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Example:

    "columns": {
      "type": "object",
      "minProperties": 1,
      "patternProperties": {
        ".+": {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or use/merge #158.

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 7, 2022

@jorisvandenbossche In #153 we require a minimum length of 1 for the primary geometry and I'm trying to check whether the schema fails for an empty primary geometry.

But I just now realize that it's wrong and should be:

metadata = copy.deepcopy(metadata_template)
metadata["primary_geometry"] = ""
invalid_cases["empty_primary_geometry"] = metadata

@jorisvandenbossche
Copy link
Collaborator Author

OK, that one is already included!

@jorisvandenbossche jorisvandenbossche merged commit a6c102d into opengeospatial:main Dec 7, 2022
@jorisvandenbossche jorisvandenbossche deleted the schema-tests branch December 7, 2022 22:04
@tschaub
Copy link
Collaborator

tschaub commented Dec 7, 2022

Would there be support for moving the test_json_schema.py script to the scripts directory? It feels like that directory has the most complete story around managing python dependencies.

@jorisvandenbossche
Copy link
Collaborator Author

I wouldn't mind that, certainly now it is only a single file (I am also already making use of the python dependencies that are installed for the scripts to run the tests in CI)

@tschaub
Copy link
Collaborator

tschaub commented Dec 8, 2022

I moved this script in #159 and updated it to read the version constant from the schema file. So the tests now assert that metadata without a version or with a version that is different than the schema version is invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests on JSON schema, to ensure that updates to the schema are valid / good
6 participants