Cif 247 increase unit test coverage for cif #75
Conversation
There was a problem hiding this comment.
There is a lot to review here. I gave it a quick pass and all seams reasonable but I don't know if it is worth reviewing every test one by one to check that it is actually testing what I think it should be.
I also ran pytest locally but it failed to run and when I tried to update my environment it failed because exactextract was not available. Maybe worth getting Github Actions to run using Conda?
…/github.com/wri/cities-cif into CIF-247-Increase-unit-test-coverage-for-CIF
|
I'm now testing how to create GitHub Actions workflow using Conda. |
…ons' into CIF-247-Increase-unit-test-coverage-for-CIF
There was a problem hiding this comment.
I tried conda env update -f environment.yml --prune on my existing cities-cif Conda environment but it failed.
Then I tried creating a new Conda environment and that built fine.
All tests passed (except the 2 skipped ones). Why not just remove these since the layers are deprecated?
tests/test_layer_dimensions.py
Outdated
| expected_y_dimension = 20 | ||
| assert data.dims == {"x": expected_x_dimension, "y": expected_y_dimension} | ||
|
|
||
| def test_albedo_dimensions(): |
There was a problem hiding this comment.
Is "dimensions" abstract here or mean x/y dimensions? It feels like you're mostly test x/y dimensions, but then actually test stats about the values in these two functions.
There was a problem hiding this comment.
"Dimensions" was abstract and I've renamed. I also renamed the overall file to "test_layer_metrics" and added a description near top of file.
|
@jterry64 - Good points about the previous test_layer_dimensions.py (now test_layer_metrics) file. It is on my list of things to improve, so thank you for highlighting the file. |
kcartier-wri
requested review from
jterry64 and
chrowe
and removed request for
chrowe and
weiqi-tori
kcartier-wri
deleted the
CIF-247-Increase-unit-test-coverage-for-CIF
branch
Further increasing test coverage.