Adding unit tests

[lilyclements]

Note that they are commented out since we call code from the bucket, which is not accessible to this package directly. Hence, all tests would fail unless we store the data in the package. I can uncomment these out and test them that way remotely.

[lloyddewit]

@lilyclements Thank you for adding unit tests, I think these can significantly reduce regression risk from future changes. Some questions:

• I don't see any expected test results to compare against. Should the tests check that the meta data and data returned from the tested function are as expected?
• is it worth testing the case where the function is called with only mandatory parameters (optional parameters unspecified), e.g. crop_success_probabilities(country="zm", station_id="16")?
• is it worth adding tests for annual_temperature_summaries() and monthly_temperature_summaries() or do you think these functions are adequately tested by the total_temperature_summaries() tests?

[lilyclements]

I can do this. The reason I didn’t was in case the definitions changed further in the definitions json file. However, where we give the parameters in the r function this is straight forward.

I believe that I do test without the parameters specified in the r code (see ‘result_2’ or ‘result_3’).

I think these are adequately covered by the total_temperature_summaries test but happy to add for monthly and annual if you think so.

[lloyddewit]

I can do this. The reason I didn’t was in case the definitions changed further in the definitions json file. However, where we give the parameters in the r function this is straight forward.

If we don't check the function's return data, then we won't detect most of the potential regression. Tests where the paramaters are fully specified would still be very valuable. If we want to test non-specified parameters also, then could we have a set of definition files used for testing?

I believe that I do test without the parameters specified in the r code (see ‘result_2’ or ‘result_3’).

yes, that's correct, my mistake. Is it also worth testing annual_rainfall_summaries() without a summaries parameter (R function signature allows this)?

I think these are adequately covered by the total_temperature_summaries test but happy to add for monthly and annual if you think so.

That sounds fine. You know the code the best, and you're the best person to judge where to focus the testing.

[lilyclements]

@lloyddewit Great! I'll work on this tomorrow since I have quite a few other bits to get on with today. Is that okay with you? I'm optimistic that there shouldn't be any other changes.