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

Initial configuration for oarec tests #683

Merged
merged 4 commits into from
May 24, 2021

Conversation

ricardogsilva
Copy link
Member

@ricardogsilva ricardogsilva commented May 16, 2021

This PR adds initial support for testing in the oarec branch.

Contrary to the previous tests, the proposed testing strategy is now more inline with the usual practices of a flask app.

Previous functional tests for the CSW standard (and friends) relied on a custom pytest setup that basically discovered and created tests based on each suite found under tests/functional/suites. This approach had been taken because at the time we introduced pytest as our testing framework there were already a large number of functional tests that used a custom runner.

This time around, since oarec support is brand new, we have an opportunity to adopt a more standard testing setup.

This PR includes some sample tests to serve as a proof-of-concept and also to lead the way for the implementation of a more comprehensive set of tests for oarec.

Modifications overview

This PR focuses mainly on adding tests. However, a few modifications have been made to the main pycsw codebase. These were done with the aim of making testing easier. The modifications made are about removing some global variables that were being set in the new oarec paths and also moving the main pycsw configuration ConfigParser object into the flask configuration object. These allow a more flexible way to initialize pycsw, making it more convenient for testing.

Included tests

This PR includes two parametrized tests, which then are expanded into 30 unique tests by pytest:

  • tests/oarec/test_oarec_functional.py:test_page_renders - this is a basic smoke test. It just verifies that the included endpoints can be reached via HTTP GET and their response status code is 200. Multiple endpoints are included by using pytest parametrization, including the landing page, conformance, openapi, etc.

  • tests/oarec/test_oarec_functional.py:test_landing_page_includes_link_to_collections - this is a more focused test that verifies if the links present on the landing page (when rendered as application/json) include a link to the collections endpoint. This test demonstrates how to perform assertions about the expected response of a request.

All of the included tests are currently passing.

Running oarec tests

Tests can be run with:

pytest -m oarec

@ricardogsilva
Copy link
Member Author

@tomkralidis this is still early work. I'll request a review (and remove draft status) when it is ready for your appreciation

@ricardogsilva ricardogsilva marked this pull request as ready for review May 17, 2021 22:06
@ricardogsilva
Copy link
Member Author

The failing CI seems to be related to a conflict in the version of jinja2 that gets installed. I tracked this down to something related with Sphinx. I think it is not in scope for this PR to address it though.

@kalxas
Copy link
Member

kalxas commented May 18, 2021

How about we pin sphinx<4.0 and see what happens?

@kalxas
Copy link
Member

kalxas commented May 18, 2021

sphinx-doc/sphinx#9216

@kalxas
Copy link
Member

kalxas commented May 18, 2021

Just tested locally, unit and oarec tests work fine.

Copy link
Member

@tomkralidis tomkralidis left a comment

Choose a reason for hiding this comment

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

Nice work here @ricardogsilva ! Just one comment. I'm also wondering whether we should try to install sphinx<4 as mentioned by @kalxas to see if the tests pass on GitHub Actions?


api_ = API(PYCSW_CONFIG)
Copy link
Member

Choose a reason for hiding this comment

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

I see that the API object is now instantiated within each route. What's the benefit/rationale?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to be able to import the wsgi_flask module with little side-effects, to make it easier to test pycsw. Testing makes use of a Flask.test_client structure, as mentioned in the flask docs. And in order to use this test_client we need to be able to import the module that defines the flask application, which in this case is pycsw.wsgi_flask.

Previously, API was a global variable that was being accessed inside each route. Since it takes the pycsw configuration as an initialization parameter, this meant that there was not a clean way to provide a custom config for pycsw. By the time we created the flask test_client the pycsw config was already read and the API was already instantiated. This made it cumbersome to write the tests, as we would have to resort to some mocking, which IMO seemed like unnecessary complexity.

With this change, it becomes possible to modify pycsw config at any time, by interacting with the flask config object. The fact that API is instantiated inside each route means we can provide a custom configs in the tests.

Let me know if you feel strongly against this and I can try to come up with some alternative solution (most likely using mocks for testing).

Copy link
Member

Choose a reason for hiding this comment

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

Is the approach taken in the pygeoapi API tests something we should consider? In other words, we test the pycsw.ogc.api.records itself / directly?

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 would add both.

Testing the various functions directly (AKA unit testing) is nice to catch logic errors. Unit tests verify implementation details, so they usually require that the person writing the test to know the underlying code well.

Testing how it all interacts together (AKA functional testing) is nice to check the big picture of things. Functional tests focus on correctness of outputs, so they are usually more of a black-box type of thing. The person writing the test just needs to check if the expected result is met.

Both of these are useful.

Usually the OGC test suites focus on functional testing, as that is what gives you some assurance about the overall behavior of the system being as expected.

In our older code we actually have both approaches too. If you look at pycsw/tests there is a functionaltests (where the test suites are) and a unittests directories.

Copy link
Member

Choose a reason for hiding this comment

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

Reading the config on every api call adds an overhead though, correct?

@ricardogsilva
Copy link
Member Author

I'll try to update sphinx then, to see whether it fixes CI.

@ricardogsilva
Copy link
Member Author

@kalxas @tomkralidis just to manage your expectations, I will likely not have much time this week to work on this PR. I should be able to work on it from next week. So if you are very eager, please feel free to take over.

@tomkralidis tomkralidis merged commit aa72aca into geopython:oarec May 24, 2021
@tomkralidis
Copy link
Member

@ricardogsilva merging for now given we have a test harness for oarec -- thanks so much for the contribution!

tomkralidis pushed a commit that referenced this pull request May 30, 2021
* Initial configuration for oarec tests

* Refactor initial tests to a single test

* Add test that verifies links of landing page

* Install oarec requirements in github actions
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