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: WP4 General API compliance validation #45

Merged
merged 14 commits into from
Feb 9, 2024

Conversation

GeraldIr
Copy link
Member

@GeraldIr GeraldIr commented Jan 31, 2024

NOTE: because I kept working on my own fork of the openeo-test-suite repo, this also includes the original changes from WP3, these can be ignored for the purpose of review (and should probably be merged seperately and not rely on this PR alone).

Add: general API compliance tests to the openeo-test-suite as requested in #20

The tests cover a range of different scenarios and endpoints and the openapi-core library is used to check the validity of responses from the API against the openapi specification. (The one included is the newest at version 1.2.0).

The files included are basic payloads, api spec (openapi.json), a utility python file and the test file containing the full range of tests.

Also note that actual requests are sent to the API, I have put skip marks on any tests that need a process graph to run through so that runtimes are not too long. Any created batch jobs, UDPs or other objects are cleaned up after every test.

Usage:

pytest src/openeo_test_suite/tests/general --html=reports/general-compliance.html --openeo-backend-url=openeo-dev.eodc.eu

@GeraldIr
Copy link
Member Author

A personal note for reviewers considerations, this code is not cleaned up. It does work and I did my best to integrate it with the existing infrastructure, but keep in mind that big refactors or cleanups might take some additional time. For now I suggest focusing on the functional aspects and any missing endpoints/scenarios and if time allows I will look into cleaning up in the future.

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 notes

@@ -0,0 +1,532 @@
from typing import Union
Copy link
Member

Choose a reason for hiding this comment

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

I guess this module and maybe the whole of general are utilities, without actual tests for the test suite.

This should be moved under src/openeo_test_suite/lib to have better separation between reusable components and actual tests

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved the compliance_util.py, but I'm unsure what you mean when you say the whole of general. test_compliance.py does contain tests, just to make sure, you don't want those moved too, correct?

src/openeo_test_suite/tests/general/openapi.json Outdated Show resolved Hide resolved
@@ -0,0 +1,340 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This file is invalid, e.g. schemas, examples etc. Make sure only properties are provided when needed.

@@ -0,0 +1,103 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Does the test suite somehow makes sure that the processes used here are actually available?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not currently, how would you suggest that we separate testing for the existence of processes and this specific process. The included process includes "subtract", "multiply", "sum" and "divide". I was hoping we can just assume the existence of those very basic processes.

If we cannot assume the existence of a single process then I am confused how I am supposed to construct a valid example for the validation endpoint at all.

Copy link
Member

Choose a reason for hiding this comment

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

I'd just do the same as in process testing: skip if not available with a corresponding message.

@@ -0,0 +1,41 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

How does this ensure the data and file formats are available?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not, is there some base dataset/file format that we can expect to always be here?

Is there a way we can include a baseline/test process that can be guaranteed to never fail?

Copy link
Member

Choose a reason for hiding this comment

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

I'd just do the same as in process testing: skip if not available with a corresponding message.

@m-mohr
Copy link
Member

m-mohr commented Feb 3, 2024

Conducted a very high-level review, didn't look into details yet. But it should already help to avoid some potential issues. For a more detailed review I'm waiting until the changes from #44 are not part of this PR changelist any longer.

@soxofaan
Copy link
Member

soxofaan commented Feb 7, 2024

quick note: can you move the submodule to https://github.com/Open-EO/openeo-test-suite/tree/main/assets (which already has the openeo-processes submodule)?

@GeraldIr
Copy link
Member Author

GeraldIr commented Feb 7, 2024

quick note: can you move the submodule to https://github.com/Open-EO/openeo-test-suite/tree/main/assets (which already has the openeo-processes submodule)?

Yes of course!

@GeraldIr
Copy link
Member Author

GeraldIr commented Feb 7, 2024

quick note: can you move the submodule to https://github.com/Open-EO/openeo-test-suite/tree/main/assets (which already has the openeo-processes submodule)?

@soxofaan
This is done. I've also updated the readme.

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.

this is quite a large PR and I haven't gotten through it all yet, but here are some notes already

.gitmodules Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
src/openeo_test_suite/lib/compliance_util.py Outdated Show resolved Hide resolved
…on and test_endpoint function, replace getting filenames with glob, fix typehints, introduce validate_uri function, more robust get_spec_path and get_examples_path functions
@soxofaan soxofaan merged commit fb36b9e into Open-EO:main Feb 9, 2024
2 checks passed
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.

3 participants