Skip to content

Mock DEM retieval tests#223

Merged
cforgaci merged 4 commits into
mainfrom
222-mock-download-dem-cf
Jun 27, 2025
Merged

Mock DEM retieval tests#223
cforgaci merged 4 commits into
mainfrom
222-mock-download-dem-cf

Conversation

@cforgaci
Copy link
Copy Markdown
Contributor

@fnattino, I mocked the two tests. Please have a look if this is what we need. Note that I had to decrease the precision in the tolerance parameter in the first test to make it pass.

@cforgaci cforgaci requested a review from fnattino June 27, 2025 06:57
Copy link
Copy Markdown
Contributor

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

Great! This would solve any problem with accessing the AWS data. I only wonder whether the first test is still meaningful after the mocking is introduced, I think we can remove it, see comment.

Comment thread tests/testthat/test-valley.R Outdated
Comment on lines +31 to +44
with_mocked_bindings(
load_raster = function(...) {
bucharest_dem |>
terra::project("EPSG:4326")
},
{
dem <- load_dem(bb, asset_urls, force_download = TRUE)

expect_equal(terra::crs(dem), terra::crs("EPSG:4326"))
expect_equal(as.vector(terra::ext(dem)),
as.vector(terra::ext(sf::st_bbox(bb))),
tolerance = 1.e-2)
}
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With mocking, I wonder whether this test is still meaningful - I think we could remove it.
Ultimately, load_dem here returns the raster we are passing in via the mocked binding, and then we verify properties of that same raster. In addition, due to the transformation from projected to geographic CRS you add some extra pixels at the edges, so you have to add quite a large tolerance to get this pass..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see, indeed, both the logic of mocking and the issue of tolerance due to the transformation make this useless. I will remove it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel free to merge afterwards!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also suppressed a couple of warnings and updated the release information. I know those are outside the scope of this PR, but I pushed them already. Let me know if this is okay now.

Copy link
Copy Markdown
Contributor

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

Great! This would solve any problem with accessing the AWS data. I only wonder whether the first test is still meaningful after the mocking is introduced, I think we can remove it, see comment.

@cforgaci cforgaci linked an issue Jun 27, 2025 that may be closed by this pull request
@cforgaci cforgaci linked an issue Jun 27, 2025 that may be closed by this pull request
@cforgaci
Copy link
Copy Markdown
Contributor Author

@fnattino based on your comments here and in #224 I assume that this is ready to be merged.

@cforgaci cforgaci merged commit adacc9a into main Jun 27, 2025
12 of 13 checks passed
@cforgaci cforgaci deleted the 222-mock-download-dem-cf branch June 27, 2025 08:45
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.

Warnings in mocked delineation tests Test failures during CRAN submission

2 participants