icesat2py / icepyx

Python tools for obtaining and working with ICESat-2 data
https://icepyx.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
203 stars 101 forks source link

add a few tests for Variables class #431

Open rwegener2 opened 1 year ago

rwegener2 commented 1 year ago

What was Changed

This PR adds a few tests for the Variables class. It doesn't cover all the available arguments, but it adds the following tests:

  1. the .avail() function, reading from a local file
  2. the .avail() function, using an order result
  3. the .append() function with the defaults flag set to True
  4. the .remove() function for all
  5. the .append() function when given a vars_list argument

How it was Changed

A test_variables.py file already existed, so I added additional tests there. For expected outputs longer than a few lines the output is saved in a txt or json file within the expected_outputs folder. These are read into a Python object during the test and compared to the icepyx produced result.

For the test using a local file I saved the local file in a test_data folder. I used an ATL06 product because it was small (less than 8MB). The Variables object that was created using this dataset was created as a pytest.fixture, to avoid repeating the class initiation code multiple times.

How to test it

I actually couldn't get all the tests to run, so I was only testing the test_variables.py file. To run the tests I moved into the tests folder and ran pytest test_variables.py.

Notes / Questions

Earthdata Login

In line 53 of test_variables.py I login to NASA Earthdata. This works because I have a .netrc file saved in my environment, so anyone else who runs these tests will need to have that setup as well. It looks like we maybe have some testing CI setup, so I assume that this method of logging in will fail. How do we deal with logins in CI right now? Do we already have CI EDL credentials stored in GH Secrets?

Assuming a Query will return consistently

In line 57 of test_variables.py there is a sort of awkward check to make sure that the Query only returned one granule. I can't come up with any real reason why that would ever change, but I wasn't sure. Do we feel quite certain that past granules (from 2019) won't be added (in which case I'd remove the if statement), or is it worth it to keep that check?

Context

(As a note for you @JessicaS11 I made this PR because I was starting to work on the getvars branch and thought this would provide a baseline for what the Variables module does right now when local files. When thegetvars branch enables variable search with cloud data I think ideally the calls tested here will still work swimmingly when given an s3 url instead of a local filepath.)

github-actions[bot] commented 1 year ago

Binder :point_left: Launch a binder notebook on this branch for commit d7489a5f0891a61d372e0a66ba3baa7561f14af8

I will automatically update this comment whenever this PR is modified

Binder :point_left: Launch a binder notebook on this branch for commit 6946952a0d74f2337f29af8415608e9773f4f8b9