Increase test coverage

[notger]

Pull Request

Description

Adds unit tests where applicable.

Fixes #72 and #9, though both only partly. I will provide a lenghty discussion further below, but the TL;DR is: There are two problems which prevent a higher code coverage or even make it undesirable:

1. Most of the code deals with downloading and with chunking up large sets of data. These actions are not well-testable on a regular basis.
2. A bug in the test setup prevents some code to be tested (details below).

For these reasons (again: details below) do I recommend to leave the code coverage at the 57%(?) that we would have after this PR. Also, I would vote to keep the generate_test_plots as the main method to run a sort-of-system-integration-test and not have any unit-tests on the code run there.

How Has This Been Tested?

Well ... it's unit tests ... so the code has been tested. ;)

• Yes

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

• Yes
• No, no data processing affected.

Checklist:

• My code follows OCF's coding style guidelines
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have checked my code and corrected any misspellings

Reason no. 1 for lower test coverage: download-heavy nature of code

satip/downloads.py

• hard to test, as the downloads take a ton of time and testing various code paths with lengthy downloads each and every time you do some code change somewhere does not seem a good investment.
• code coverage metric is flawed due to _download_time_range not being registered as called when dispatched to the pool
• testing the plot generation generates redundant code tests with satip.downloads

satip/eumetsat.py’s missing coverage is mostly code which governs the case of more than 10.000 search results, which seems to infeasible to test on a regular basis.

satip/intermediate.py mostly deals with splitting stuff into month-wide-zarr-files.
In order to properly test this, we would need downloaded zarr-files for more than a month, which is prohibitive, since five minutes worth of data have a size of about 100 Mb. A month would be in the order of 0.9 Tb. Too much for a recurring test.

Conclusion: Testing these parts would create a huge overhead. It seems preferrable to leave scripts/generate_test_plots.py as the main source of testing the downloading-part of the satip-code.

Reason no. 2 for lower test coverage: Strange test-setup bug

This is mostly centered around scripts/generate_test_plots.py and satip/utils.py.

• Testing satip/utils.py … there is something weird with the setup or the scope of pytest, as when you run scripts/generate_test_plot, things run fine (see also .github/workflows/plotting.yaml), but when you call that code from a pytest-file like tests/test_plot_generation.py, the code fails due to allegedly not being able to read the file (tested on local machine). At least sometimes.
• Similar problem with test_utils (disabled_utils.py), which leads to the conclusion that the culprit seems to be utils.download_cloudmask_to_dataset, which for some reason gives the error that it can not read the grb-file passed. Which it seems to have no troubles doing in the generate-test-plot-script.
• When the function from the generate-test-plot is imported in another environment, e.g. the REPL-shell, it works perfectly as well. There clearly is something weird going on with pytest.

A variant of this error on github: It fails because it can not import cartopy, which was properly installed in the steps before (check “install dependencies”-step) and does not cause any issues with the parallel test-plot-action.

Again, as scripts/generate_test_plots.py is working, adding tests for the utils in the unit-tests seem not to add too much value. While it sure would be nice to not have to rely on the script during testing, it seems a stable way to verify the code working.

[codecov-commenter]

Codecov Report

Merging #102 (da82c26) into main (caf6bf4) will increase coverage by 6.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   39.30%   45.37%   +6.07%     
==========================================
  Files          10       10              
  Lines         832      833       +1     
==========================================
+ Hits          327      378      +51     
+ Misses        505      455      -50     

Impacted Files	Coverage Δ
satip/jpeg_xl_float_with_nans.py	100.00% <ø> (+67.39%)	⬆️
satip/app.py	96.87% <100.00%> (ø)
satip/scale_to_zero_to_one.py	100.00% <100.00%> (+40.00%)	⬆️
satip/utils.py	66.19% <100.00%> (ø)
satip/eumetsat.py	48.33% <0.00%> (+1.66%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[notger]

@jacobbieker As mentioned over in #72 : I am baffled by the error in the tests.
See here: https://github.com/openclimatefix/Satip/runs/5973037341?check_suite_focus=true .

The error message states that it can not load the ecCodes library, but installation was successful, according to the installation logs.
Stack trace complains about the version function in gribapi not working, which is part of eccodes.

I pulled the docker image and there, the imports work.

Do you have any idea what could cause this? I can't replicate this error locally with the eccodes-lib version 0.9.9, which is the same as in the Docker image.

Wild guess: Does the test setup use a different one but the most recent Docker image, which contain different lib-versions?

[jacobbieker]

I think because the plots workflow runs, but the tests do not, we might need to install the test requirements with conda, like we do for the plotting workflow. Otherwise, while eccodes is installed, it doesn't have the binaries to load if its just pip installed. So I think the fix would be to copy the setup steps for the plotting workflow into the test workflow, and then it should work. The reason to leave eccodes and some of the plotting code requirements out of Satip's default requirements is that they can be a bit finicky and are only used for a smaller subset of the overall functionality.

[notger]

I think because the plots workflow runs, but the tests do not, we might need to install the test requirements with conda, like we do for the plotting workflow. ...

@jacobbieker Makes a ton of sense and you were right: The workflow setups for tests and for plots were different.

Now, the weird thing is that if I make the testing workflow use the same setup steps as the plotting job such that the machine setup should be identical up the the install dependencies-step, then that step gets cancelled for the test-workflow, but works for the plot-workflow.
Example of cancellation: https://github.com/openclimatefix/Satip/runs/6019369878?check_suite_focus=true
The same step for a plotting job runs through: https://github.com/openclimatefix/Satip/runs/6019369759?check_suite_focus=true

For a side by side comparison of the workflow-definitions:
https://github.com/openclimatefix/Satip/blob/notger/increase-test-coverage/.github/workflows/workflows.yaml
https://github.com/openclimatefix/Satip/blob/notger/increase-test-coverage/.github/workflows/plotting.yaml

Is there anything I am missing? Any idea why it works in one case but not in the other (I have a hunch I am missing something obvious here)?
Wild guess: Are there any project settings which enforce a cancellation in one case but not the other, e.g. resource constraints?

[jacobbieker]

I think because the plots workflow runs, but the tests do not, we might need to install the test requirements with conda, like we do for the plotting workflow. ...

@jacobbieker Makes a ton of sense and you were right: The workflow setups for tests and for plots were different.

Now, the weird thing is that if I make the testing workflow use the same setup steps as the plotting job such that the machine setup should be identical up the the install dependencies-step, then that step gets cancelled for the test-workflow, but works for the plot-workflow. Example of cancellation: https://github.com/openclimatefix/Satip/runs/6019369878?check_suite_focus=true The same step for a plotting job runs through: https://github.com/openclimatefix/Satip/runs/6019369759?check_suite_focus=true

For a side by side comparison of the workflow-definitions: https://github.com/openclimatefix/Satip/blob/notger/increase-test-coverage/.github/workflows/workflows.yaml https://github.com/openclimatefix/Satip/blob/notger/increase-test-coverage/.github/workflows/plotting.yaml

Is there anything I am missing? Any idea why it works in one case but not in the other (I have a hunch I am missing something obvious here)? Wild guess: Are there any project settings which enforce a cancellation in one case but not the other, e.g. resource constraints?

The only thing I noticed is that the tests also run on MacOS, which is where the installation is failing, while the plots are only on Linux. Now, we could just remove tests on macOS, I'd prefer not to, but for now, it might be the easiest to make sure the unit tests pass that you've written, and we can readd back in MacOS support later?

[notger]

@jacobbieker You were right. So ... the mac-os-test failing during the dependency-installation-step led to a cancellation of the linux-dependency-installation-step? TIL.

Anyway, the removal of mac-os-tests is totally acceptable and I would argue that mac-os-specific unit-tests aren't really needed. I can see the reasoning behind that, but going down that route effectively means lumping unit tests and system setup tests into one. If you feel strongly about the mac-os-tests, we can try to re-add them, as you suggested.

There is something off with the pytest call, and I will look into that tomorrow. Road looks clear-ish from here on.

[notger]

@jacobbieker I took the liberty of adding you as reviewer. Please read the description at the top of the PR for an extensive report. Happy to discuss, if need be even in a vid-call.

[jacobbieker]

It looks good! Just one minor comment, to reduce randomness in the tests. Also, could you make sure the test_generate_test_script passes? Or remove it, if it doesn't work, the plot generation fails anyway and so that acts as its own form of test. But thanks loads for all this work! Looks great overall.

[jacobbieker]

This makes sense, but what might be slightly more reproducible or consistent would be to set to a negative number, like -1 or something, as all values are above 0 in the compressed and decompressed data.

Suggested change

	nan_replacement = np.random.rand()
	nan_replacement = -1.0

[notger]

Good point, will do!

[notger]

Also, could you make sure the test_generate_test_script passes? Or remove it, if it doesn't work, the plot generation fails anyway and so that acts as its own form of test.

As I have no idea what makes this script fail non-reproducibly, I would agree it is best to remove it (see discussion in the PR description for details). Seems the most stable path and leaves the new user with a usable, intuitive script.