Reinstate test_download_granules_without_subsetting #581

weiji14 · 2024-08-22T16:55:06Z

Trying to increase code coverage of ipx.Query class's order_granules and download_granules methods, by ensuring that granules can be ordered from NSIDC and downloaded with the subset=False option.

TODO:

Add test which should fail due to not capturing stdout and stderr messages
Regex match on stdout and stderr so test will pass
Check that downloaded files are actually not subsetted (kinda)

Adjacent stuff done in this PR

Fix a possible race condition in case the NSIDC order status is completed right at the start (e.g. when order was cached/done already).
~~- [x] Upload code coverage for 'behind Earthdata' tests via Travis CI~~

Ensure that granules can be ordered from NSIDC and downloaded with the `subset=False` option. Need to check what the stdout and stderr strings are to get the test to pass.

github-actions · 2024-08-22T16:55:20Z

👈 Launch a binder notebook on this branch for commit 115ce4c

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit 9090378

👈 Launch a binder notebook on this branch for commit 58a4a68

👈 Launch a binder notebook on this branch for commit 97ba3de

👈 Launch a binder notebook on this branch for commit 29a7230

👈 Launch a binder notebook on this branch for commit e47a1a5

👈 Launch a binder notebook on this branch for commit 4fbbb98

👈 Launch a binder notebook on this branch for commit 358a265

👈 Launch a binder notebook on this branch for commit 1e8799c

👈 Launch a binder notebook on this branch for commit d3a0b8a

👈 Launch a binder notebook on this branch for commit 65328a6

👈 Launch a binder notebook on this branch for commit 6434fae

codecov · 2024-08-22T17:01:44Z

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 65.17%. Comparing base (572e48c) to head (6434fae).
Report is 1 commits behind head on development.

Files with missing lines	Patch %	Lines
icepyx/tests/test_behind_NSIDC_API_login.py	0.00%	15 Missing ⚠️
icepyx/core/granules.py	0.00%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (572e48c) and HEAD (6434fae). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (572e48c) HEAD (6434fae)

3 2

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #581      +/-   ##
===============================================
- Coverage        71.08%   65.17%   -5.91%     
===============================================
  Files               37       37              
  Lines             3043     3061      +18     
  Branches           594      596       +2     
===============================================
- Hits              2163     1995     -168     
- Misses             769      981     +212     
+ Partials           111       85      -26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Got `UnboundLocalError: local variable 'loop_root' referenced before assignment` in CI because the order request status completed early, so need to declare the 'loop_root' variable early.

Adding the start of the stdout message, and expanding the reqparams and orderIDs length to get the integration test to pass.

Move the codecov setup to the install step to reduce repetition, so that both the 'basic tests' and 'behind Earthdata' stages can use the same codecov uploader script.

Check that 3 processed_ATL06_201902*.h5 files are downloaded after calling `.download_granules()`.

icepyx/tests/test_behind_NSIDC_API_login.py

Correct test should have been to check for files without 'processed_' prefix. Also checking the .iso.xml metadata files and the filesize for good measure now.

weiji14 · 2024-08-27T23:37:41Z

icepyx/tests/test_behind_NSIDC_API_login.py

+    # check that the max extent of the downloaded granules isn't subsetted
+    assert len(glob.glob(pathname=f"{path}/ATL06_201902*.iso.xml")) == 3
+    h5_paths = glob.glob(pathname=f"{path}/ATL06_201902*.h5")
+    assert len(h5_paths) == 3
+    assert [os.path.getsize(filename=p) for p in h5_paths] == [
+        65120027,  # 62.1 MiB
+        53228429,  # 50.8 MiB
+        49749227,  # 47.4 MiB
+    ]


Checking for the non-subsetted 'ATL06_*.h5' files here now. Let me know if there's a better way to check for non-subsetted files instead of just the filesize here.

@mfisher87 Any ideas other than file size?

Woops, missed this. I don't have any other ideas right now!

icepyx/tests/test_behind_NSIDC_API_login.py

Returned list from glob is not deterministic, so need to sort the filename list to make the test pass always.

This reverts commit d3a0b8a.

weiji14 · 2024-08-30T10:59:31Z

Got the test_download_granules_without_subsetting test to pass when I triggered it at https://github.com/icesat2py/icepyx/actions/runs/10631387124/job/29472168206#step:4:25.

icepyx/tests/test_behind_NSIDC_API_login.py

JessicaS11

Thanks, @weiji14! I think this looks great.

JessicaS11 · 2024-08-30T19:35:33Z

Got the test_download_granules_without_subsetting test to pass when I triggered it

Out of curiousity, how did you trigger it? It doesn't have a manual trigger, and should only run with an "approved" PR review status (but we're having some issues with this, clearly, so trying to learn how you were able to make it go).

weiji14 · 2024-09-02T22:11:41Z

Got the test_download_granules_without_subsetting test to pass when I triggered it

Out of curiousity, how did you trigger it? It doesn't have a manual trigger, and should only run with an "approved" PR review status (but we're having some issues with this, clearly, so trying to learn how you were able to make it go).

I triggered it at commit d3a0b8a by modifying .github/workflows/integration_test.yml to run on all branches for pull requests (not just those that target the main branch). Downside with this trick is that you'll need to have a commit to revert the changes later.

Co-authored-by: Jessica Scheick <[email protected]>

JessicaS11 · 2024-09-03T20:13:01Z

Got the test_download_granules_without_subsetting test to pass when I triggered it

Out of curiousity, how did you trigger it? It doesn't have a manual trigger, and should only run with an "approved" PR review status (but we're having some issues with this, clearly, so trying to learn how you were able to make it go).

I triggered it at commit d3a0b8a by modifying .github/workflows/integration_test.yml to run on all branches for pull requests (not just those that target the main branch). Downside with this trick is that you'll need to have a commit to revert the changes later.

Sneaky! Would that work from a fork too?

JessicaS11

Re-approving to trigger integration test.

weiji14 · 2024-09-03T21:47:45Z

Sneaky! Would that work from a fork too?

Not sure 😅 I had a look at the docs and couldn't figure it out. Understandably, this sounds like a security concern, but secrets shouldn't be accesible from forks, and there should be an 'Approve to run' button that is set for new contributors according to https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/approving-workflow-runs-from-public-forks

Reinstate test_download_granules_without_subsetting

115ce4c

Ensure that granules can be ordered from NSIDC and downloaded with the `subset=False` option. Need to check what the stdout and stderr strings are to get the test to pass.

weiji14 added the afternoon_contribution This issue should be resolvable in an afternoon or so of work. label Aug 22, 2024

weiji14 self-assigned this Aug 22, 2024

weiji14 added 4 commits August 22, 2024 10:12

[nsidc_tests] Add assertion statements to check reqparams and orderIDs

9090378

[nsidc_tests] Handle race condition if status completes early

58a4a68

Got `UnboundLocalError: local variable 'loop_root' referenced before assignment` in CI because the order request status completed early, so need to declare the 'loop_root' variable early.

Correct outputs from order_granules method

97ba3de

Adding the start of the stdout message, and expanding the reqparams and orderIDs length to get the integration test to pass.

[nsidc_tests] Upload codecov report for tests requiring Earthdata login

29a7230

Move the codecov setup to the install step to reduce repetition, so that both the 'basic tests' and 'behind Earthdata' stages can use the same codecov uploader script.

weiji14 mentioned this pull request Aug 23, 2024

Delegate full granule query to earthaccess, keep smart subsetting logic in icepyx #575

Open

weiji14 added 2 commits August 23, 2024 10:27

Merge branch 'development' into test_order_granules_without_subset

2a42483

[nsidc_tests] Ensure that 3 processed_ATL06.h5 files are downloaded

e47a1a5

Check that 3 processed_ATL06_201902*.h5 files are downloaded after calling `.download_granules()`.

weiji14 marked this pull request as ready for review August 23, 2024 18:12

JessicaS11 reviewed Aug 26, 2024

View reviewed changes

icepyx/tests/test_behind_NSIDC_API_login.py Outdated Show resolved Hide resolved

weiji14 added 3 commits August 28, 2024 00:30

Merge branch 'development' into test_order_granules_without_subset

db96705

Improve test by checking for iso.xml files and filesize

4fbbb98

Correct test should have been to check for files without 'processed_' prefix. Also checking the .iso.xml metadata files and the filesize for good measure now.

[nsidc_tests] Move comment up a few lines

358a265

weiji14 commented Aug 27, 2024

View reviewed changes

JessicaS11 reviewed Aug 29, 2024

View reviewed changes

icepyx/tests/test_behind_NSIDC_API_login.py Show resolved Hide resolved

weiji14 added 4 commits August 30, 2024 11:40

Merge branch 'development' into test_order_granules_without_subset

1e8799c

Sort list of downloaded ATL06 filenames

82fa916

Returned list from glob is not deterministic, so need to sort the filename list to make the test pass always.

Temporarily trigger integration tests on pull_request

d3a0b8a

Revert "Temporarily trigger integration tests on pull_request"

65328a6

This reverts commit d3a0b8a.

JessicaS11 reviewed Aug 30, 2024

View reviewed changes

icepyx/tests/test_behind_NSIDC_API_login.py Outdated Show resolved Hide resolved

JessicaS11 approved these changes Aug 30, 2024

View reviewed changes

Update comment about assertion code

6434fae

Co-authored-by: Jessica Scheick <[email protected]>

JessicaS11 approved these changes Sep 3, 2024

View reviewed changes

weiji14 merged commit 6d6a085 into development Sep 3, 2024
8 of 10 checks passed

weiji14 deleted the test_order_granules_without_subset branch September 3, 2024 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reinstate test_download_granules_without_subsetting #581

Reinstate test_download_granules_without_subsetting #581

weiji14 commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

weiji14 Aug 27, 2024

JessicaS11 Aug 30, 2024

mfisher87 Sep 4, 2024

weiji14 commented Aug 30, 2024

JessicaS11 left a comment

JessicaS11 commented Aug 30, 2024

weiji14 commented Sep 2, 2024

JessicaS11 commented Sep 3, 2024

JessicaS11 left a comment

weiji14 commented Sep 3, 2024

Reinstate test_download_granules_without_subsetting #581

Reinstate test_download_granules_without_subsetting #581

Conversation

weiji14 commented Aug 22, 2024 • edited Loading

github-actions bot commented Aug 22, 2024 • edited Loading

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

weiji14 Aug 27, 2024

Choose a reason for hiding this comment

JessicaS11 Aug 30, 2024

Choose a reason for hiding this comment

mfisher87 Sep 4, 2024

Choose a reason for hiding this comment

weiji14 commented Aug 30, 2024

JessicaS11 left a comment

Choose a reason for hiding this comment

JessicaS11 commented Aug 30, 2024

weiji14 commented Sep 2, 2024

JessicaS11 commented Sep 3, 2024

JessicaS11 left a comment

Choose a reason for hiding this comment

weiji14 commented Sep 3, 2024

weiji14 commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading