Unit tests for opening-saving YAML #6
Conversation
Kate/yaml validation
* adjust datetime format * run tests on PR, show failed tests * add pytests for file open-save * typo * separate the workflow * absolute import * stop ignoring yml on commit * import top level repo * run pytests from root * run pytests from the workspace * update imports in test folder * init file for tests * mode importlib * import qgis as optional * avoiding qgis imports if possible * fix imports * optional typing for qgis imports * same * make pyqt5 offscreen mode * run tests in a virtual display * Install PyQt5 dependencies manually * switch to qbot instead of QApplication * only init dialog * remove typos * YAML for windows * runt test * ignore Qgs imports * run a bit more * print URL * print from start * show print statements * print statements flush * more prints * check file location * more prints * wrap QgsMessageLog to try/except * print new file path for checking * write wrong YAML * Revert "write wrong YAML" This reverts commit dd7a6e4. * fix test * remove datatime validation (for testing) * Revert "remove datatime validation (for testing)" This reverts commit 627f488.
sync to master
|
👍🏽 |
There was a problem hiding this comment.
Thanks for adding the unit tests, and for including them in the GitHub actions! 🥇
Could you also add a section about setting up and running the tests (standalone) in the README file?
I wrote some comments about reading the yaml test files directory from their repositories. Do you think that makes sense, or maybe there is a reason for having them here?
If we copy them here, I think that we may run into the risk that they become outdated, and we won't even know from which version they belong to. At the same time, reading them from the upstream repositories can also lead to failing tests, and it will be hard to pinpoint if it is because of code changes, or schema changes. I think one other option could also be to tag the config files with a commit (or release) hashtag, and in that way we would know exactly to which version they belong. I would ❤️ to hear your thoughts about this.
I think with all the code that you introduced, you are in a position to implement the most challenging test of all: comparing the input yaml and the output yaml 😺
I am aware this may require a refactoring of the code, but from a user's perspective, I think it is super important for the tool to not change anything that was not strictly updated by the user (even when these changes are apparently harmless).
tests/yaml_samples/geopython_pygeoapi_@5ed31b4_pygeoapi-test-config-ogr.yml
Show resolved
Hide resolved
| import subprocess | ||
|
|
||
| from ..pygeoapi_config_dialog import PygeoapiConfigDialog | ||
|
|
There was a problem hiding this comment.
I think we need a third test, to validate that the yaml that we input is the same as the yaml that we output:
Example:
diff cite.config.yml saved_cite.config.yml - The output of this command should be an empty set.
I foresee some challenges, in order to pass that text (but I am confident you can address them 😁 ):
- The order of the keys may be different in the returned file; nevertheless that should still yield a valid result.
- Some optional fields with default values are being added; that should not happen - we can delegate the task of adding default values to pygeoapi. For example this:
hello-world:
type: process
processor:
name: HelloWorld
Is being transformed into this:
hello-world:
type: collection
title: ''
description: ''
keywords: []
links: []
extents:
spatial:
bbox: [-180, -90, 180, 90]
crs: http://www.opengis.net/def/crs/OGC/1.3/CRS84
providers: []
There was a problem hiding this comment.
I see the issue here. The test is only writing YAML, while the normal user interaction would be to see the Error window and not be able to proceed with Save file option (the validation check is done before opening a window to choose Save file directory - that part is skipped in unit tests). This is solvable. But the mandatory fields would still be added on file opening, so I'll look for a way around to ignore them in diff.
There was a problem hiding this comment.
I don´t think you should implement validation for this test. Maybe I should have picked an example of a collection type that is supported (not OGC API - Processes) (:
Let's take the lakes collection, at cite.config.yml:
lakes:
type: collection
title: Large Lakes
description: lakes of the world, public domain
keywords:
- lakes
crs:
- CRS84
links:
- type: text/html
rel: canonical
title: information
href: http://www.naturalearthdata.com/
hreflang: en-US
extents:
spatial:
bbox: [-180,-90,180,90]
crs: http://www.opengis.net/def/crs/OGC/1.3/CRS84
temporal:
begin: 2011-11-11T00:00:00Z
end: null # or empty
providers:
- type: tile
name: MVT-tippecanoe
data: ../data/tiles/ne_110m_lakes
options:
bounds: [[-124.953634,-16.536406],[109.929807,66.969298]]
zoom:
min: 0
max: 5
format:
name: pbf
mimetype: application/vnd.mapbox-vector-tile
The tests output this collection:
lakes:
type: collection
title: Large Lakes
description: lakes of the world, public domain
keywords:
- lakes
links:
- type: text/html
rel: canonical
href: http://www.naturalearthdata.com/
title: information
hreflang: en-US
extents:
spatial:
bbox: [-180, -90, 180, 90]
crs: http://www.opengis.net/def/crs/OGC/1.3/CRS84
temporal:
begin: 2011-11-11T00:00:00Z
providers:
- type: tile
name: MVT-tippecanoe
data: ../data/tiles/ne_110m_lakes
options:
layer: ''
style: ''
version: ''
format:
name: pbf
mimetype: application/vnd.mapbox-vector-tile
The provider options had bounds and a zoom dictionary, but now I see a layer, style and version options that did not exist before, with empty values (maybe these come from WMSFacade?) I think this is the sort of thing that could be captured by a simple test that compares the input yaml and the output yaml (without ny validation).
There was a problem hiding this comment.
What is happening to the OGC API - Processes collection points out a different issue. What happens to collections or providers that are not supported? I think they should be written in the output file without any changes, but some warning should be thrown to the user stating that a non supported key was detected and that we cannot guarantee the validation of that element. Having elements that are not supported breaks our ability to provide a valid pygeoapi config, but I guess there is not much we can do about that.
There was a problem hiding this comment.
The process collection is a bit different from the others; [the only required fields are 'type' and 'processor.name']. Type in this case is process, rather than collection.(https://github.com/geopython/pygeoapi/blob/5ed31b47773e7fc0b2a5b1a34055c3b2659d4952/pygeoapi/resources/schemas/config/pygeoapi-config-0.x.yml#L591).
A process is a mathematical function, which may not have a spatial extent. These fields do not apply in the case of processes:
title:
description:
keywords:
links:
extents:
spatial:
bbox:
crs:
If we cannot disable the validation, maybe it is better to updated the validator to look for these fields.
|
Regarding the files to test against, I would keep them deterministic with the similar set of files to test against. With testing against the versioned config files, what would be that we want to test for exactly? |
Hopefully I have answered that question in my other comments. Let me know, if I didn't (: |
|
Thanks, all clear now! :) |
Uodate field values according to byteroad#6
There was a problem hiding this comment.
@KatKatKateryna Thanks for adding a script to add the tests locally (and a reference to it in the README): now it is much easier to run the unit tests from the cli. I think that with the sample files you added, we have a variety of configurations that allows us to capture edge cases. 💯
- Is the convention on the sample yaml file name:
organisation_repository_commit hash? I think that works pretty well; could you add to the README too? 🙏🏽 - When I run the tests locally, it correctly generates all the files; however, it does not output if (and which) tests are passed. This is the output:
collected 0 items ========================================= no tests ran in 9.61s ==========================================. Not sure if this is intended, but shouldn't it compare the input and output files and flag differences as errors? - There seems to be a problem (at least with some) changes that go through the UI, which are not captured by the tests. The example I noted is the server languages, which have a particular order (that matters). In the case of the unit tests, that order does not change, but if we open the file in the UI and then save it, it does.
- The non supported providers are still problematic. Ideally, we should not touch those, but if we run the tests, the non supported providers have some keys removed and appended which make them invalid (I have pointed some examples in my comments). If we open these non supported providers in the UI, they are set as read-only, but the validation is still applied. Let me know if this approach makes sense: #9
| gzip: false | ||
| languages: | ||
| # First language is the default language | ||
| - en-US |
There was a problem hiding this comment.
The order of the items in this array is important, because it defines the default language. I just realised that although this is preserved through the unit tests, it is not the case when we load the file and save it through the UI; this must be because the items are assigned to the UI on loading, and then read from the UI, when saving. Unfortunately we cannot capture these transformations through the unit tests
| time_field: created | ||
| title_field: title | ||
|
|
||
| hello-world: |
There was a problem hiding this comment.
Although this provider is read-only, it is validated when saving through the UI. This means that if there is a missing field here (for instance, description) it won't save the file unless we delete this resource.
| import subprocess | ||
|
|
||
| from ..pygeoapi_config_dialog import PygeoapiConfigDialog | ||
|
|
There was a problem hiding this comment.
The process collection is a bit different from the others; [the only required fields are 'type' and 'processor.name']. Type in this case is process, rather than collection.(https://github.com/geopython/pygeoapi/blob/5ed31b47773e7fc0b2a5b1a34055c3b2659d4952/pygeoapi/resources/schemas/config/pygeoapi-config-0.x.yml#L591).
A process is a mathematical function, which may not have a spatial extent. These fields do not apply in the case of processes:
title:
description:
keywords:
links:
extents:
spatial:
bbox:
crs:
If we cannot disable the validation, maybe it is better to updated the validator to look for these fields.
| @dataclass(kw_only=True) | ||
| class ResourceTemporalConfig: | ||
|
|
||
| # optional |
There was a problem hiding this comment.
I think we are missing an optional 'crs' field on the top level of ResourceConfigTemplate (which is different from the one inside extents.spatial):
https://github.com/geopython/pygeoapi/blob/5ed31b47773e7fc0b2a5b1a34055c3b2659d4952/pygeoapi/resources/schemas/config/pygeoapi-config-0.x.yml#L547
We are also missing the optional "links" key (and its sub-keys), which is resulting in it being wiped out during the saving process.
| title: Daily Mean of Water Level or Flow | ||
| description: The daily mean is the average of all unit values for a given day. | ||
| keywords: [Daily, Daily Mean, Water Level, Flow, Discharge] | ||
| crs: |
There was a problem hiding this comment.
We do not have support for this key, and if it exists it is disappearing during the saving process (see the comment regarding ResourceConfigTemplate).
| begin: str | datetime | None = None | ||
| end: str | datetime | None = None | ||
| begin: datetime | None = None | ||
| end: datetime | None = None |
There was a problem hiding this comment.
The spatial.extents.crs key is being updated to the default value of'http://www.opengis.net/def/crs/OGC/1.3/CRS84', regardless of its original value.
| map: | ||
| url: https://tile.openstreetmap.org/{z}/{x}/{y}.png | ||
| attribution: '© <a href="https://openstreetmap.org/copyright">OpenStreetMap contributors</a>' | ||
| manager: |
There was a problem hiding this comment.
This key (and its sub-keys) is disappearing during the saving process of the cite tests.
There was a problem hiding this comment.
The temporal extent of lakes collection:
temporal:
begin: 2011-11-11
end: null # or empty (either means open ended)
Is being converted during the tests to:
temporal: {}
Maybe because the datetime object is not complete with the timestamp?
In this file we are hitting the problem of a provider that is not supported. As an example, the dutch_georef_stations uses the OGR provider. The tests are appending some fields that do not exist in this provider (data:, host: , dbname: user: ') and removing fields that exist for this provider (gdal_ogr_options, source_options, etc).
|
@doublebyte1 thank you for the thorough review! I added the third comparison test and will push it now - the tests will fail. This revealed many issues, including the ones you mentioned. Addressing some of your points:
Other notes:
|
|
@KatKatKateryna Thanks for the changes 👍🏽 . Now I am able to run the tests and get output. 
I think splitting the tests into different smaller files makes sense. Also, the issues we are having with unimplemented providers only affect the resources part, and it may be easier to tackle them that way. Regarding the In YAML you can have lists as inline (with square brackets) or in multiple lines, so I think we should support both formats. https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html |
|
The DIFF files are significantly minimized, so there is no need to test each data type and provider type separately 🙏
Last touches to complete:
|
I cannot reproduce it either, nevermind that! 😅
You are right, but I am a bit puzzled that the collection level metadata crs is present at the configuration files for the demo cite instance: Let me dig a bit in the code and come back to you, on this one. 👍🏽
|
Unit tests added to run PyQt plugin in a headless mode. This required some changes in the main code:
Additionally:
Screenshots:
Failed tests:
