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

Fix resource calls #16

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

mcebria-hg
Copy link
Contributor

The calls to resources don't work, I have adapted them according to geoserver documentation and performed the call tests.

@mcebria-hg mcebria-hg force-pushed the fix-rest-resources-and-tests branch from 6a9d929 to 099f19d Compare January 21, 2025 15:40
@arthurdjn
Copy link
Owner

Thanks for your contribution!
Could you add a small line about this change in the CHANGELOG.md file too?

@arthurdjn arthurdjn added the bug Something isn't working label Jan 21, 2025
@mcebria-hg mcebria-hg force-pushed the fix-rest-resources-and-tests branch from 099f19d to d82b4c8 Compare January 22, 2025 11:16
@mcebria-hg
Copy link
Contributor Author

Done, it's ok?

@arthurdjn
Copy link
Owner

arthurdjn commented Jan 22, 2025

I added some reviews in your tests, otherwise everything looks good !

@mcebria-hg mcebria-hg force-pushed the fix-rest-resources-and-tests branch from d82b4c8 to d4f8358 Compare January 22, 2025 14:56
@mcebria-hg
Copy link
Contributor Author

mcebria-hg commented Jan 23, 2025

I don't know why test_head_resource breaks, I run tests on my machine with a geoserver v2.23 and it works great.
image

If you prefer we can delete it and set with "Not implemented yet" or we try it without path.

If test_get_resource works, test_head_resource should work .

@arthurdjn
Copy link
Owner

arthurdjn commented Jan 23, 2025

There are two errors: one due to test_head_resource the other from test_delete_resource

I agree that for now test_head_resource can be skipped, I think that the documentation from geoserver is not up to date

Comment on lines 887 to 888
assert isinstance(data, bool)
assert data
Copy link
Owner

Choose a reason for hiding this comment

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

I think something like the below is easier to read:

assert data is True

WDYT?

Comment on lines 897 to 905
def test_delete_resource(test_geoserver: GeoServer, test_workspace: str, test_coverage: str) -> None:
path = f"data/{test_workspace}/{test_coverage}"
msg = test_geoserver.delete_resource(path)
assert isinstance(msg, str)
Copy link
Owner

Choose a reason for hiding this comment

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

This test is problematic because it will delete the fixture test_coverage, and the fixture teardown will break (because it will try to remove an already deleted coverage).

Instead I would suggest to upload a tmp coverage, and then delete it within the test.

def test_delete_resource(test_geoserver: GeoServer, test_workspace: str) -> None:
    coveragestore = "tmp-coveragestore"
    file_path = Path(TEST_DATA_DIR, "rasters", "raster.tif").resolve()
    test_geoserver.upload_coverage_store(
        file=file_path,
        name=coveragestore,
        workspace=test_workspace,
        format="geotiff",
    )
    
    path = f"data/{test_workspace}/{coveragestore}"
    msg = test_geoserver.delete_resource(path)
    # ...

@mcebria-hg mcebria-hg force-pushed the fix-rest-resources-and-tests branch from d4f8358 to 305b691 Compare January 27, 2025 15:41
* test: add resource calls tests
@mcebria-hg mcebria-hg force-pushed the fix-rest-resources-and-tests branch from 3684d19 to 65fba01 Compare January 27, 2025 15:54
@arthurdjn arthurdjn merged commit a65c8cf into arthurdjn:main Jan 27, 2025
7 checks passed
@arthurdjn
Copy link
Owner

Thanks @mcebria-hg for this PR and your effort & time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants