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 manifest testing behavior #955

Merged

Conversation

chris-okorodudu
Copy link
Contributor

Description

Updating the update_node_dependency method to ensure all relevant test nodes are included in the airflow graph. This fix stems from the test nodes not being included in the filtered_nodes dict after the select_nodes method is used to do filtering by graph operator.

Related Issue(s)

#949

Breaking Change?

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

@chris-okorodudu chris-okorodudu requested a review from a team as a code owner May 13, 2024 15:07
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 13, 2024
Copy link

netlify bot commented May 13, 2024

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 4d8d503
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66447216c7a8510008e6b900

@dosubot dosubot bot added the area:testing Related to testing, like unit tests, integration tests, etc label May 13, 2024
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this fix, @chris-okorodudu !

It seems some of the tests are breaking, such as:

 +  where {'/home/runner/work/astronomer-cosmos/astronomer-cosmos/dev/dags/example_virtualenv.py': 'Traceback (most recent call ...ate_node_dependency\n    for _, node in self.nodes.items():\nRuntimeError: dictionary changed size during iteration\n'} = 

Could you fix this, please?

@chris-okorodudu
Copy link
Contributor Author

Thanks a lot for working on this fix, @chris-okorodudu !

It seems some of the tests are breaking, such as:

 +  where {'/home/runner/work/astronomer-cosmos/astronomer-cosmos/dev/dags/example_virtualenv.py': 'Traceback (most recent call ...ate_node_dependency\n    for _, node in self.nodes.items():\nRuntimeError: dictionary changed size during iteration\n'} = 

Could you fix this, please?

Yes definitely I'll try to take a look later today

pankajastro and others added 2 commits May 14, 2024 21:35
closes: astronomer#952

This PR addresses the following issues:

**1. Pre-commit Setup:**
- Pre-commit was not running in the CI pipeline because it was neither
configured for this repository nor were we using the pre-commit command
in CI to run it manually. To fix this:
Installed and added pre-commit to this repository. This allows
pre-commit to automatically run the pre-commit configuration on every
push.

**2. Type Check Job:**
- Previously, the type-check job was running with mypy directly, and the
pre-commit type check command had different configurations. This PR
aligns the configurations.

**3. Type Check Fixes:**
- Ignored the type check for __dataclass_fields__ since mypy seems to
not recognize it.
- Added a check for None for ti.task
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.69%. Comparing base (e4aa3c4) to head (4d8d503).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #955   +/-   ##
=======================================
  Coverage   95.69%   95.69%           
=======================================
  Files          59       59           
  Lines        2879     2880    +1     
=======================================
+ Hits         2755     2756    +1     
  Misses        124      124           

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

Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot, @chris-okorodudu !

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 15, 2024
@tatiana tatiana merged commit 48ad267 into astronomer:main May 15, 2024
69 checks passed
@tatiana tatiana mentioned this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:testing Related to testing, like unit tests, integration tests, etc lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants