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

Feature: WP3 Process metadata validation #44

Merged
merged 16 commits into from
Feb 7, 2024

Conversation

GeraldIr
Copy link
Member

Add: functional and non-functional metadata tests to the openeo-test-suite as requested in #19

Functional metadata includes parameter types and return types.
Non-functional metadata includes descriptions, summaries and links.

Existence of the processes is naturally also checked.

To run these tests --openeo-backend-url has to be specified for the pytest command such that '/processes' or 'processes' can be added at the end in order to call the processes endpoint of the given backend (e.g.: "https://openeo-dev.eodc.eu/openeo/1.1.0")

This PR also adds a metadata field to the ProcessData class such that the existing infrastructure for process selection could be reused.

Example call:
pytest src/openeo_test_suite/tests/processes/metadata --html=reports/individual-processes.html --openeo-backend-url=https://openeo-dev.eodc.eu/openeo/1.1.0 --processes=pi

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some initial feedback

@soxofaan
Copy link
Member

soxofaan commented Jan 30, 2024

On a more general note, I see that this pattern is often used:

        assert (
            expected_parameter["name"] == actual_parameter["name"]
        ), f"The parameter named '{actual_parameter['name']}' of the process \
            '{expected_process.process_id}' should be named '{expected_parameter['name']}'"

With pytest, I don't think you have to build such a message and you can replace these 4 lines with just 1:

        assert expected_parameter["name"] == actual_parameter["name"]

pytest will automatically create a message describing the difference, and will do it smarter, less error prone and more flexible based on a verbosity level.

For example: I had this failing test on a schema in load_collection. The manually constructed error message is this hard to read blob (I stripped some parts to keep it short here):

AssertionError: The parameter named 'spatial_extent' of the process 'load_collection' should have the schema '[{'title': 'Bounding Box', 'type': 'object', 'subtype': 'bounding-box', 'required': ['west', 'south', 'east', 'north'], 'properties': {'west': {'description': 'West (lower left corner, coordinate axis 1).', 'type': 'num.... metry'}]}, {'title': 'No filter', 'description': "Don't filter spatially. All data is included in the data cube.", 'type': 'null'}]' but has the schema '[{'properties': {'base': {'default': None, 'description': 'Base (optional, lower left corner, coordinate axis 3).', 'type': ['number', 'null']}, 'crs': {'anyOf': [{'examples': [3857], 'minimum': 1000, 'subtype': 'epsg-code', 'title': 'EPSG Code', 'type': 'integer'}, {'subtype': 'wkt2-definition', 'title': 'WKT2', 'type': 'string'}], 'default': 4326, 'description': 'Coordinate reference system of the extent, specified as as EPSG code or WKT2 CRS string. Defaults to 4326 (EPSG code 4326) unless the client explicitly requests a different coor....metries in the vector data cube. For raster data, all pixels inside the bounding box that do not intersect with any of the polygons will be set to no data (null). Empty geometries are ignored.', 'dimensions': [{'type': 'geometry'}], 'subtype': 'datacube', 'title': 'Vector data cube', 'type': 'object'}, {'description': "Don't filter spatially. All data is included in the data cube.", 'title': 'No filter', 'type': 'null'}]'

The automatic message from pytest (with increased verbosity through cli option -vv), gives this structured readable diff with smart inline ASCII art pointers to the actual parts that are different:

E             Full diff:
E               [
E                {'properties': {'base': {'default': None,
E                                         'description': 'Base (optional, lower left corner, '
E                                                        'coordinate axis 3).',
E                                         'type': ['number',
E                                                  'null']},
E                                'crs': {'anyOf': [{'examples': [3857],
E                                                   'minimum': 1000,
E                                                   'subtype': 'epsg-code',
E                                                   'title': 'EPSG Code',
E                                                   'type': 'integer'},
E                                                  {'subtype': 'wkt2-definition',
E                                                   'title': 'WKT2',
E                                                   'type': 'string'}],
E                                        'default': 4326,
E                                        'description': 'Coordinate reference system of the '
E                                                       'extent, specified as as [EPSG '
E                                                       'code](http://www.epsg-registry.org/) '
E                                                       'or [WKT2 CRS '
E                                                       'string](http://docs.opengeospatial.org/is/18-010r7/18-010r7.html). '
E                                                       'Defaults to `4326` (EPSG code 4326) '
E                                                       'unless the client explicitly requests '
E                                                       'a different coordinate reference '
E             +                                         'system. If the bounding box is not '
E             +                                         'provided in the coordinate reference '
E             +                                         'system (CRS) of the data cube, the '
E             +                                         'bounding box is reprojected to the '
E             +                                         'CRS of the spatial data cube '
E             -                                         'system.'},
E             ?                                           ^ ---
E             +                                         'dimensions.'},
E             ?                                          +++++ ^^^
E                                'east': {'description': 'East (upper right corner, coordinate '
E                                                        'axis 1).',
E                                         'type': 'number'},
E                                'height': {'default': None,
E                                           'description': 'Height (optional, upper right '
E                                                          'corner, coordinate axis 3).',
...

Copy link
Member

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Left two comments for consideration.

Gerald Walter Irsiegler added 2 commits January 31, 2024 14:39
… so short forms of urls are supported, remove superfluous code, remove message from all asserts, cleaned up getting actual process to only be a single line and be reused
soxofaan added a commit that referenced this pull request Feb 1, 2024
a non-reusable generator was being cached instead of a cachable concrete result (list)

related to PR #44
@GeraldIr
Copy link
Member Author

GeraldIr commented Feb 5, 2024

This should be ready for merging.

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some more notes

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some more notes

…or links, links now sorted before comparing, changed the way additional or removed parameters are checked, removed todo from readme.md, documented L4 default
@soxofaan
Copy link
Member

soxofaan commented Feb 7, 2024

ok, many thanks for the quick updates,
this now looks ready to merge

@soxofaan soxofaan merged commit 89867c9 into Open-EO:main Feb 7, 2024
2 checks passed
@soxofaan
Copy link
Member

soxofaan commented Feb 7, 2024

@GeraldIr I just noticed that your commits list an invalid email address, which probably ruins the proper attribution of your contributions in github and other tools:

$ git log --format=full eodcgmbh/process-metadata -1
commit 8424bb9fe44773a4d831da4ff0b3a0fd973d674c
Author: Gerald Walter Irsiegler <gerald@Irsiegler-P14s>
Commit: Gerald Walter Irsiegler <gerald@Irsiegler-P14s>

    mark non functional tests as opti...

I guess there is something wrong with your git setup

@GeraldIr
Copy link
Member Author

GeraldIr commented Feb 7, 2024

@GeraldIr I just noticed that your commits list an invalid email address, which probably ruins the proper attribution of your contributions in github and other tools:

$ git log --format=full eodcgmbh/process-metadata -1
commit 8424bb9fe44773a4d831da4ff0b3a0fd973d674c
Author: Gerald Walter Irsiegler <gerald@Irsiegler-P14s>
Commit: Gerald Walter Irsiegler <gerald@Irsiegler-P14s>

    mark non functional tests as opti...

I guess there is something wrong with your git setup

@soxofaan thanks for noticing that, I just updated my global git config. Contributions are still correctly displayed as well.

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

Successfully merging this pull request may close these issues.

3 participants