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

Refactor integration tests to remove random collection sampling #749

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

mfisher87
Copy link
Collaborator

@mfisher87 mfisher87 commented Jul 6, 2024

Resolves #215

Replaces random collection sampling with hardcoded lists of 100 top collections per provider in popularity order, with script to regenerate the lists as needed. Instead of sampling n random collections we select n most popular.

There's still a clear need for refactoring of the 4 cloud/onprem download/open test modules. They share a lot of code that can be fixturized. I don't want this PR to grow larger than it already is, so IMO that should be a follow-up activity.

@mfisher87 mfisher87 changed the title Refactor integration tests Refactor integration tests to remove random collection sampling Jul 6, 2024
"page_num": 1,
"page_size": 100,
"sort_key[]": "-usage_score",
},
Copy link
Collaborator Author

@mfisher87 mfisher87 Jul 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied this out of my browser dev tools' network tab after doing a similar query in earthdata search client. I'm sure we can run an equivalent query with earthaccess.

@mfisher87
Copy link
Collaborator Author

Worked on this with @itcarroll during hack day. Notes: #755

@mfisher87
Copy link
Collaborator Author

We considered the usefulness of random sampling tests. We don't think we should be doing this for integration tests, especially when they execute on every PR. We could, for example, run them on a cron job and create reports, but that seems like overkill when we have a community to help us identify datasets and connect with the right support channel if there's an issue with the provider.

We may still consider a cron job for, for examle, recalculating the most popular datasets on a monthly basis.

@mfisher87
Copy link
Collaborator Author

We decided we can hardcode a small number and expand the list as we go. Other things like random tests on a cron or updating the list of popular datasets on a cron can be addressed separately.

@mfisher87
Copy link
Collaborator Author

@betolink will take on work to update generate.py to generate top N collections for all providers.

@mfisher87 will continue working on test_onprem_download.py for just NSIDC_ECS for now to make it use the new source of collections.

@mfisher87
Copy link
Collaborator Author

We will update the .txt files to .csv files and add boolean field for "does the collection have a EULA?" and then we'll use that field to mark those tests as xfail.

@mfisher87
Copy link
Collaborator Author

mfisher87 commented Aug 21, 2024

Two major milestones:

  1. @danielfromearth updated the script which generates the top collection lists to use all providers supported by earthaccess 🎉 Still TODO: Make them CSVs with a boolean representing whether the collection has a EULA
  2. We just got the test_onprem_download.py module working without randomization! 🎉 Still TODO: Refactor the other 3 integration test modules to share this behavior. Let's try and remove duplicate code while we're at it!

Thanks to @DeanHenze and @Sherwin-14 for collaborating on this on today's hackathon!

@@ -244,6 +244,9 @@ def _repr_html_(self) -> str:
granule_html_repr = _repr_granule_html(self)
return granule_html_repr

def __hash__(self) -> int:
return hash(self["meta"]["concept-id"])
Copy link
Collaborator Author

@mfisher87 mfisher87 Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@betolink @chuckwondo This seems reasonable to me, but please validate me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it for like 5 minutes, this is obviously a bad idea. This class is subclassing dict. We'd need to implement like a frozendict.

@mfisher87
Copy link
Collaborator Author

Also still TODO: Run generate.py in GHA on a monthly/quarterly cron and auto-open a PR with the changes to top collections?

@mfisher87
Copy link
Collaborator Author

If we want to determine whether a collection has a EULA, this example was provided:

curl -i -XGET "https://cmr.earthdata.nasa.gov/search/collections.json?concept_id=C1808440897-ASF&pretty=true"

The metadata "eula_identifiers" : [ "1b454cfb-c298-4072-ae3c-3c133ce810c8" ] is present in the response. We're not 100% sure whether this can be used authoritatively. Discussion in progress: https://nsidc.slack.com/archives/C2LRKMDEV/p1724179804149239

@mfisher87 mfisher87 added the help wanted Extra attention is needed label Sep 3, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Add tests for OBDAAC on-prem open. Related to #828 - we want to make sure the data streams successfully. Opening data from OBDAAC on-prem relies on both #828 and a (potentially) unreleased change to fsspec! (check the September release notes)

@danielfromearth
Copy link
Collaborator

Looks like part of this issue may be related to work on EULAs in this issue.

Copy link

github-actions bot commented Nov 26, 2024

Binder 👈 Launch a binder notebook on this branch for commit 3a09c11

I will automatically update this comment whenever this PR is modified

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

Binder 👈 Launch a binder notebook on this branch for commit 6e0ca0b

Binder 👈 Launch a binder notebook on this branch for commit 85c4bb9

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

Binder 👈 Launch a binder notebook on this branch for commit 1b9de98

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

Binder 👈 Launch a binder notebook on this branch for commit 5b40af6

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

Binder 👈 Launch a binder notebook on this branch for commit 1d3df25

Binder 👈 Launch a binder notebook on this branch for commit 670b0f1

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

@mfisher87 mfisher87 removed the help wanted Extra attention is needed label Nov 26, 2024
@mfisher87
Copy link
Collaborator Author

For some reason, running

nox -s integration-tests -- --cov=earthaccess --cov-report=term-missing --capture=no --tb=native --log-cli-level=INFO

is failing, when

nox -s integration-tests

is passing.

@mfisher87
Copy link
Collaborator Author

Looks like this was a coincidence with intermittent uptime on an external dependency.

@mfisher87 mfisher87 marked this pull request as ready for review November 27, 2024 00:27
@mfisher87 mfisher87 requested review from chuckwondo, jhkennedy and betolink and removed request for chuckwondo and jhkennedy November 27, 2024 00:30
@@ -0,0 +1,100 @@
C2799438299-POCLOUD
C1996881146-POCLOUD
# C2204129664-POCLOUD
Copy link
Collaborator Author

@mfisher87 mfisher87 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This collection isn't working so good 🤒

We get 0 granules:

>           assert len(granules) > 0, msg
E           AssertionError: AssertionError for C2204129664-POCLOUD
E           assert 0 > 0
E            +  where 0 = len([])

But it's still 3rd most-popular? I'm confused :)

@@ -6,6 +6,7 @@
from fsspec.core import strip_protocol

logger = logging.getLogger(__name__)
pytestmark = pytest.mark.skip(reason="Tests are broken.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are failing on the release. Maybe xfail is a better mark. I preferred not to get into fixing this in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt this needed simplification as I added more.

  • We have a few "guide" things, so I gave them a naming pattern so they can be mentally grouped
  • Removed the word "Our" because it wasn't adding anything
  • "Naming conventions" felt out of place, too specific. Like the new integration test doc. So I created a new "Topics" subsection (but not a subdirectory to keep the URL flatter). I don't like "Topics", but it's the best I have thought of so far.

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.

Integration tests are flaky -- replace dataset sampling with top 50 datasets
2 participants