Conversation
weiji14
left a comment
weiji14
left a comment
There was a problem hiding this comment.
Hi @rwegener2! First off, thanks for all your great work on icepyx recently! Really nice to see more unit tests added to icepyx because it really improves the long term sustainability and quality of the software 😄
Taking a look at your code, the implementation of the tests are pretty good, and it looks like you've covered most of the public methods (those not starting with an underscore (e.g. _check_valid_lists). Ideally though, we'll need to figure a way to avoid storing the 8MB HDF5 file in git, hopefully via a dynamic download step, or some alternative way. See specific comments below.
| def variables_obj_atl06(): | ||
| return Variables( | ||
| "file", | ||
| path='./test_data/processed_ATL06_20191130112041_09860505_006_01.h5', |
There was a problem hiding this comment.
Hmm, we'll need to see if it's possible to avoid storing this 8MB HDF5 file in git, because it will slow down git clone for future developers. There's actually some work to prevent big files from being uploaded at #143, and we had some nasty work a few years ago to remove HDF5 files at #114.
There are some tests in the test_behind_NSIDC_API_login.py file which actually downloads data from NSIDC. Maybe it is possible to reuse some functionality from there?
| @pytest.fixture() | ||
| def variables_obj_atl06(): | ||
| return Variables( | ||
| "file", |
There was a problem hiding this comment.
Is it possible to use vartype="order" instead of vartype="file" to dynamically download the file?
| region = Query('ATL06',[-45, 74, -44,75],['2019-11-30','2019-11-30'], \ | ||
| start_time='00:00:00', end_time='23:59:59') |
There was a problem hiding this comment.
Recommend to set the version here, because the returned schema can change from version to version.
| region = Query('ATL06',[-45, 74, -44,75],['2019-11-30','2019-11-30'], \ | |
| start_time='00:00:00', end_time='23:59:59') | |
| region = Query( | |
| product="ATL06", | |
| spatial_extent=[-45, 74, -44, 75], | |
| date_range=["2019-11-30", "2019-11-30"], | |
| start_time="00:00:00", | |
| end_time="23:59:59", | |
| version="006", | |
| ) |
2 participants
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:
.avail()function, reading from a local file.avail()function, using an order result.append()function with thedefaultsflag set to True.remove()function for all.append()function when given avars_listargumentHow it was Changed
A
test_variables.pyfile already existed, so I added additional tests there. For expected outputs longer than a few lines the output is saved in atxtorjsonfile within theexpected_outputsfolder. These are read into a Python object during the test and compared to theicepyxproduced result.For the test using a local file I saved the local file in a
test_datafolder. I used an ATL06 product because it was small (less than 8MB). TheVariablesobject that was created using this dataset was created as apytest.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.pyfile. To run the tests I moved into thetestsfolder and ranpytest test_variables.py.Notes / Questions
Earthdata Login
In line 53 of
test_variables.pyI login to NASA Earthdata. This works because I have a.netrcfile 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.pythere 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
getvarsbranch and thought this would provide a baseline for what the Variables module does right now when local files. When thegetvarsbranch 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.)