fix tests to allow for variable STAC versions in output #73
Conversation
There was a problem hiding this comment.
Without some kind of dependapot checking packages regularly, I'm expecting maintenance creep and vulnerabilities in the long run.
For example, requests and lxml are regularly updated to resolve security issues. I somewhat like the fact that those are auto-bumped to latest version. The "pinned" versions, if needed, are available in the docker.
Note also that limiting MAJOR is not really a fix, just as in the case of pystac. It is a seemingly "MINOR" update that broke the tests by changing STAC version entirely.
tests/test_standalone_stac_item.py
Outdated
| @@ -86,5 +88,6 @@ def test_cmip6_stac_thredds_catalog_parsing(): | |||
| ref_file = os.path.join(CUR_DIR, "data/stac_collection_testdata_xclim_cmip6_catalog.json") | |||
| with open(ref_file, mode="r", encoding="utf-8") as ff: | |||
| reference = json.load(ff) | |||
| reference["stac_version"] = get_stac_version() | |||
There was a problem hiding this comment.
Seems problematic IMO.
There are different properties (eg: new bands) in STAC 1.1 for which stac_version should not be blindly "overridden", but actually migrated.
If there was an error when using pystac within stac-populator, I'd argue this is probably a mishandling somewhere which should be addressed. I find this test properly early-detected an issue that must be investigated further, since pystac is supposed to support both versions, and do appropriate migrations as needed.
Are we not simply ignoring a potential issue if the catalog targeted by stac-populator still uses STAC 1.0? We cannot just push a STAC 1.1 to it, and the populator code should adapt to this (if needed).
There was a problem hiding this comment.
See https://github.com/stac-utils/pystac/blob/main/CHANGELOG.md#v1121
Maybe we should have migrate=False explicitly?
There was a problem hiding this comment.
Are we not simply ignoring a potential issue if the catalog targeted by stac-populator still uses STAC 1.0? We cannot just push a STAC 1.1 to it, and the populator code should adapt to this (if needed).
I don't think that is what this is testing. This does not try to upload any data to an existing STAC server so it doesn't have any information from a server to know what version it is running.
We should definitely add a test that checks for that but that should be additional to this one.
There are different properties (eg: new bands) in STAC 1.1 for which stac_version should not be blindly "overridden", but actually migrated.
I'll update this test to ensure that any migrations beyond just the "stac_version" between versions are being tested as well.
There was a problem hiding this comment.
What I mean is that the populator code should work with either 1.0 or 1.1 STAC documents.
If there is any dependency (pystac here) that suddenly breaks because 1.0 isn't parsed as expected, or results in a different output, the STAC Item that would eventually be pushed to the referenced catalog could fail during that step, since that catalog might not yet support more recent versions.
Even if the test indeed doesn't upload anything here, we were "lucky" to actually detect this here. pystac should not impose its own newer version if the STAC document already indicated one (as it does here), since it might have a legitimate reason to do so, just like using 1.1 explicitly most probably means that item relies on the new bands property.
There was a problem hiding this comment.
The STAC document doesn't indicate anything here. It is used as a reference in the test to check that the pystac item matches some hard-coded json. At no point is that reference json used as an input to pystac.
If we need a new test or new code to ensure that pystac respects existing versions then great! Good idea! Let's add that in. Let's make an issue for it. Let's add a new test in a different PR that checks for that.
This pull request is trying to fix existing tests so that our test suite continues to work as it did before. We cannot merge any existing PRs without the test suite passing.
If you will not approve this PR until the test you want has been added then fine. Either me or you or @huard can add that test when we have the time to dedicate to this and all the other PRs will wait in the meantime.
There was a problem hiding this comment.
I have a proposed solution for this issue in #79.
Can we please merge this PR so that the initial issue of failing tests is resolved and then the secondary issue can be discussed in the other PR if needed.
There was a problem hiding this comment.
I'm not sure the solution is quite right yet.
See my comment directly on the PR.
There was a problem hiding this comment.
Oh, I just saw your #73 (comment) after the PR review.
Seems like the tooling is a limiting factor, so I guess we don't have much choice to move forward...
Is the currently packaged STAC-FastAPI able to support 1.1 seemlessly? If so, then I think pinning pystac in #79 would be a good enough tradeoff.
There was a problem hiding this comment.
Is the currently packaged STAC-FastAPI able to support 1.1 seemlessly?
Yes, probably since version 1.1 is compatible with previous versions: stac-utils/pystac#1375 (comment)
Can we continue this discussion in #79 and if you're happy with the changes made in the PR to simply get the tests working again can you please approve this PR.
I can't merge this PR without your approval and I can't merge any other PR that is currently open while the tests are failing.
There was a problem hiding this comment.
OK. Let's do this to unblock the PRs, and we address 1.1 separately after.
Ok let's add a dependabot
We're not just developing a docker image though, this is also usable as a package and dependency versions should be managed properly so they don't break things for other users.
Ok so we'll keep pystac more limited. Good to know. |
FYI I don't have sufficient permissions to enable dependabot for this repo. If you have permissions @huard or @fmigneault please feel free to set it up. |
Tests currently assume that the STAC version in item and collection JSONs will always be
1.0.0.Since we don't pin a specific version of
pystac, a new version ofpystacuses STAC version1.1.0which now causes tests to fail.This PR updates tests to allow for item and collection JSONs to have a
stac_versionfield that matches the version used by the installed version ofpystac.As part of this PR, I also updated the dependency versions to not allow major version upgrades automatically. For example,
pystacis now pinned to version~=1.12which will mean is will not install version 2+. This should avoid similar breaking changes caused by dependency updates in the future.