Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pqdm_kwargs integration tests #866

Closed
wants to merge 2 commits into from

Conversation

chuckwondo
Copy link
Collaborator

@chuckwondo chuckwondo commented Nov 4, 2024

Integration tests are failing, with many errors of the following form:

FAILED tests/integration/test_cloud_open.py::test_***_can_open_onprem_collection_granules[daac1] - NotImplementedError: granules should be a list of DataGranule or URLs

This PR fixes this issue, although it's odd that the integration tests did not fail prior to the code landing on main that is now causing this problem.

It turns out to be an issue with use of singledispatch, and having registered functions with signatures that differed (accidentally, it seems) by more than simply a difference of type of the first argument.


📚 Documentation preview 📚: https://earthaccess--866.org.readthedocs.build/en/866/

Copy link

github-actions bot commented Nov 4, 2024

Binder 👈 Launch a binder notebook on this branch for commit 7b7b132

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit cf77990

@mfisher87
Copy link
Collaborator

Does it have something to do with our magic code that allows 10% of integration tests to fail? 😆

@chuckwondo
Copy link
Collaborator Author

Does it have something to do with our magic code that allows 10% of integration tests to fail? 😆

I don't think so. Something very odd seems to be happening, and I can't figure it out.

The code in this PR passes all unit and integration tests when I run them locally with nox, so I have no idea what's going on with all the CI failures.

It appears my attempt to "fix" the recent integration test failures has only produced even more failures. I'm still digging to see what's up.

@mfisher87
Copy link
Collaborator

Want to pair on it sometime this week?

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 5.00000% with 19 lines in your changes missing coverage. Please review.

Project coverage is 58.72%. Comparing base (2f49c08) to head (cf77990).
Report is 70 commits behind head on main.

Files with missing lines Patch % Lines
earthaccess/store.py 0.00% 15 Missing ⚠️
earthaccess/api.py 20.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (2f49c08) and HEAD (cf77990). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (2f49c08) HEAD (cf77990)
14 8
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #866       +/-   ##
===========================================
- Coverage   73.88%   58.72%   -15.17%     
===========================================
  Files          31       13       -18     
  Lines        2003     1100      -903     
===========================================
- Hits         1480      646      -834     
+ Misses        523      454       -69     

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

@chuckwondo
Copy link
Collaborator Author

chuckwondo commented Nov 5, 2024

@mfisher87, I discovered one of the problems, which was that the unit tests that were added for the original pqdm_kwargs work should have been integration tests instead. The unit tests were failing because they were waiting for interactive input of creds for login. I have moved them (and tweaked them) to be integration tests, so now they are passing.

However, what's puzzling now is why are integration tests failing?

What I've discovered by looking at the most recent failure is that the integration test jobs are not running the most recent code that I've pushed. I am coming to this conclusion because of the following:

  • The new integration tests are not being executed (because they're not in whichever version of code is being run)

  • This error shows that the reported line is not the latest version of code that I've pushed:

    File "/opt/hostedtoolcache/Python/3.11.10/x64/lib/python3.11/site-packages/***/store.py", line 364, in open
      return self._open(granules, provider, pqdm_kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    

    That is not the current code on line 364 of store.py in my repo branch.

Oddly, it appears that the code being run is coming from nsidc:main, not from chuckwondo:fix-pqdm-kwargs.

I might simply close this PR and open a new one to see if that fixes the problem.

@chuckwondo
Copy link
Collaborator Author

Closing this PR to see if opening a new one gets past the odd behavior I'm seeing in this one.

@chuckwondo chuckwondo closed this Nov 5, 2024
@chuckwondo chuckwondo mentioned this pull request Nov 5, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants