search

GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.
https://www.pygmt.org
BSD 3-Clause "New" or "Revised" License
758 stars 220 forks source link

Guidance on types of tests to write for plotting functions #771

Closed willschlitzer closed 3 years ago

willschlitzer commented 3 years ago

The testing section on the contributing guide does a good job of explaining how to write tests, but doesn't really explain what should and should not be tested with the plotting functions. Looking through the tests (specifically the ones testing functions in base_plotting.py) there doesn't appear to be a standard for testing functions that pass arguments to the GMT API. My limited understanding makes me think the most important thing to test is the function's ability to take arguments for its aliases and pass them to GMT. There are some tests (such as test_basemap_winkel_tripel() and test_basemap_power_axis() in test_basemap.py and many of the tests in test_colorbar.py) that test passing different arguments to the same parameters. While I understand the importance of testing that these will return functions, any errors seem like they would be upstream issues with GMT and not an issue with PyGMT itself. In an effort to standardize writing tests and reducing the amount of time spent testing, could there be some community consensus (and formal guidance) on how each wrapped GMT function should be tested?

I'm happy to implement/change tests once a standard has been decided.

weiji14 commented 3 years ago

Yeah, we do need to spell this out. I would think of testing as a spectrum from:

  1. Absolute minimum - every line of code is covered by a test, see https://codecov.io/gh/GenericMappingTools/pygmt. Note that this does not even need to cover all aliases! Just bare functionality.

  2. Full stress testing - Unit tests checking every alias, every possible combination of arguments, hypothesis/mutation testing, behavioural-driven/integration tests, you name it.

Now you'll probably realize that we fall somewhere in the middle, for practical reasons. Not everyone knows how to write tests, hence the range of style. If I could mention 2 things about testing in PyGMT:

  1. Don't test everything that GMT already tests (or that you think it tests, e.g. funky combinations of arguments). There are exceptions to this, such as testing to cover against regressions, and testing important functions used by thousands of people (e.g. plot).

  2. Do test the Python specific stuff. The virtualfile mechanism, numpy/pandas/xarray input/output is a prime example.

Above all, remember the golden rule of testing, which is to not delete or change tests unless absolutely necessary. These tests may be sacredly guarding some obscure logic and we want to ensure things work years into the future.

That said, this is just one perspective, and not a fixed rule. I appreciate this being brought up, but you'll realize that there's no hard and fast rules, only guidelines.

seisman commented 3 years ago
  • Don't test everything that GMT already tests (or that you think it tests, e.g. funky combinations of arguments). There are exceptions to this, such as testing to cover against regressions, and testing important functions used by thousands of people (e.g. plot).
  • Do test the Python specific stuff. The virtualfile mechanism, numpy/pandas/xarray input/output is a prime example.

Yes, I believe we should at least document these two rules!

willschlitzer commented 3 years ago

@weiji14 Thanks for the explanation; I'm pretty new to testing anything that isn't a simple return of a string/integer/float.

In addition to the Python specific stuff, I think all of the aliases should be tested (as I mentioned in #769), and my thought is there should be a single test function that makes sure all of the aliases work; if any of them are mutually exclusive with another alias (no examples comes to mind for this, but I haven't dove too deep into the more advanced PyGMT functions) there should be a separate test that only changes the arguments in question. I don't think there should be advanced inputs, but just valid arguments to make sure that the Python wrapper plays nicely with the GMT API.

weiji14 commented 3 years ago

I think all of the aliases should be tested (as I mentioned in #769), and my thought is there should be a single test function that makes sure all of the aliases work

In an ideal world yes, with a team of a hundred developers and unlimited Continuous Integration resources.

In reality, time could be spent on more productive things. Aliases are not the weakest point in PyGMT, so we don't have to test all of them unless there's a bug caused by some very complex combination of parameters (which needs to be handled in a very specific way in Python).

That said, you're welcome to write new tests for PyGMT for learning purposes, I know I certainly did! But in terms of a formal guideline, I don't think we should enforce such a strict set of rules.

willschlitzer commented 3 years ago

@weiji14 Good to know; thanks!

seisman commented 3 years ago

I think all of the aliases should be tested (as I mentioned in #769)

Another point is that alias works for most cases unless there are typos, because adding a new alias is pretty easy (see https://github.com/GenericMappingTools/pygmt/pull/661 for an example PR, which adds the no_clip alias to some plotting functions) and the documentation is always consistent with the codes.

seisman commented 3 years ago

We should mention that we recommend using SI units in the documentation.