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

Move integration testing feedstock to tests directory #171

Merged
merged 9 commits into from
Feb 17, 2024

Conversation

moradology
Copy link
Contributor

@moradology moradology commented Feb 12, 2024

Move integration testing feedstock into this repo to avoid doing the two-phase updates where possible.
Closes #143

@ranchodeluxe ranchodeluxe added test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test. labels Feb 13, 2024
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (028903c) 95.93% compared to head (59042fc) 95.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   95.93%   95.53%   -0.40%     
==========================================
  Files          14       14              
  Lines         492      493       +1     
==========================================
- Hits          472      471       -1     
- Misses         20       22       +2     

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

@ranchodeluxe
Copy link
Collaborator

Currently looking at the integration tests now 👀

@ranchodeluxe
Copy link
Collaborator

Alrighty, this is all up and working

@ranchodeluxe ranchodeluxe marked this pull request as ready for review February 13, 2024 19:04
Copy link
Collaborator

@ranchodeluxe ranchodeluxe left a comment

Choose a reason for hiding this comment

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

LGTM 🥳 😉

I've noticed DataFlow jobs are slower than Flink jobs and so we often have some fail just b/c they are "Pending" and surpass the timeout. That's okay, we just rerun failed tests until they disappear

@@ -72,6 +78,15 @@ def test_dataflow_integration():

# okay, time to start checking if the job is done
show_job = f"gcloud dataflow jobs show {job_id} --format=json".split()
show_job_errors = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

reviewer note: let's use this command to dump logs when job fails so we don't have to use GCP console

# dictobj runs do not generate any datasets b/c they are not recipes
# so we've asserted what we can already, just move on
if recipes_version_ref.endswith("dictobj"):
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

reviewer note: this couldn't have been actually producing output for 0.10.x so I think this is fine

pfr_version = parse_version(version("pangeo-forge-recipes"))
if pfr_version >= parse_version("0.10"):
recipe_version_ref = str(pfr_version)
recipe_version_ref = "0.10.x"
Copy link
Collaborator

@ranchodeluxe ranchodeluxe Feb 13, 2024

Choose a reason for hiding this comment

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

reviewer note:
given that integrate/unit tests only test beginning and end range of versions I made the unilateral decision to just have 0.10.x listed in test-data which I think works fine

ranchodeluxe added 2 commits February 14, 2024 16:07
…m:pangeo-forge/pangeo-forge-runner into npz/feature/internal-integration-fixtures
@ranchodeluxe ranchodeluxe removed test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test. labels Feb 15, 2024
# We expect `recipe` to be 1) a beam PTransform or 2) or a a string that leverages the
# `dict_object:` see `tests/test-data/gpcp-from-gcs/feedstock-0.10.x-dictobj/meta.yaml`
# as an example
if isinstance(recipe, PTransform):
Copy link
Collaborator

Choose a reason for hiding this comment

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

note to reviewer: zero idea how unit tests for dict-object are passing on main b/c we need the conditional here or they fail like they do on this PR 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

another question @cisaacstern: we have a lot of special logic for dict_object recipes where we seem to not test list recipes. Is the list version deprecated in favor of the dict version?

ranchodeluxe added 2 commits February 15, 2024 11:19
…m:pangeo-forge/pangeo-forge-runner into npz/feature/internal-integration-fixtures
@ranchodeluxe ranchodeluxe added test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test. labels Feb 15, 2024
@ranchodeluxe
Copy link
Collaborator

@cisaacstern: this is finally ready to go (will rebase my other one)

@ranchodeluxe ranchodeluxe merged commit 340dc74 into main Feb 17, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintain integration testing recipes in this repo?
2 participants