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

Update Test Fixtures #82

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bitner
Copy link
Contributor

@bitner bitner commented May 7, 2024

Update the test fixtures with the latest examples from the ogc features cql2 examples.

For the test_json_to_text tests, rather than directly comparing the text output from str(model) on the json examples, round trip from json to model to text and back to the python model parsing the text to standardize representations. This may not be exact as directly comparing the strings, but allows for differences in case, whitespace, and double quotes.

@bitner
Copy link
Contributor Author

bitner commented May 7, 2024

@eseglem I saw that you had done the work to update the examples in the ogc features repo.

@eseglem
Copy link
Owner

eseglem commented May 13, 2024

I am not sure I had thought about doing it that way. I believe I was comparing the strings directly to avoid involving the parser and any possible issues with it in this part of the tests.

I had written some additional tests to run against the ogcapi-features repo, but hadn't pushed it here because it relied on the PR there. The idea was to check out that repo in parallel with this one and test against it so the examples can be directly used instead of keeping a copy which can drift. I just pushed it to https://github.com/eseglem/pycql2/tree/feature/test-ogcapi-features-examples-directly now. It works locally at least. And I figured there should be a way to have it checkout their repo as part of the test actions and run against it. Just had not gotten around to implementing it yet.

It would probably need to pull a specific commit hash to ensure the tests don't randomly break due to a change in their repo. And regularly updating that hash as long as nothing breaks. But I think that still seems like a nicer option than a copy.

There is also a license mismatch which I was trying to avoid. This repo is MIT and theirs is not. I doubt it would cause any real issues, but nothing in this repo is directly copy and paste from theirs at this point to be safe about it.

Perhaps a combination of the two is in order. Where the existing tests can be left as is and the tree comparison can be added to the other tests?

Definitely open to your thoughts on it. Any concerns or issues with that?

@bitner
Copy link
Contributor Author

bitner commented May 13, 2024

Yeah, I think if we left the existing tests as-is and added the ones against the ogc examples separately, it would be better to leave the existing ones as-is as just string tests.

For the OGC examples case, though, I think we would want to keep the text tests exactly as they are in the OGC Features API repo in which case we need to do something like what I did.

Looking at their license, if we just include the tests by manually copying (as in my PR here) we would need to add the copyright notice, but otherwise the license looks very compatible with MIT with no restrictions other than maintaining the copyright/license on copying/modifying things. I would say we could take two approaches to including those tests that would avoid that (by pulling directly, we would also always be pulling the copyright/license with the examples):

  1. Include the ogcapi-features repo as a git sub-module that could just be merged to new versions / hashes of ogcapi-features as they came out.
  2. Use the release packages from ogcapi-features (ie https://github.com/opengeospatial/ogcapi-features/archive/refs/tags/cql2-1.0.0-rc.1.zip) and just include the URL at the top of the tests and to fetch that file before running tests.

Git sub-module's can be confusing for a lot of folks when checking out repos, so I would probably lean towards that second option.

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.

2 participants