Fix adding test nodes to DAGs built using `LoadMethod.DBT_MANIFEST` and `LoadMethod.CUSTOM` #615

navedgaras · 2023-10-19T13:39:25Z

Resolving issues with the DBT_MANIFEST/CUSTOM load methods when the has_test attribute is not assigned to the node correctly.

Description

When a tag selector is used, all tests are filtered out because of the DbtResourceType.TEST node does not have any information about tags. To bypass this limitation - tags are assigned to tests based on their parent model.

Related Issue(s)

Closes #580

Breaking Change?

The change should not be breaking anything.

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

…ibute is not assigned to the node correctly

netlify · 2023-10-19T13:39:30Z

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

Name	Link
🔨 Latest commit	`61fdeaf`
🔍 Latest deploy log	https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/653f633483986e0008dd05ea

tatiana · 2023-10-20T09:51:28Z

Thank you very much for working on this, @edgga ! Please let us know if you'd like reviews. We'll assume that while it is in the draft, it's a work in progress, and we shouldn't review it yet.

…d_test_fix

navedgaras · 2023-10-23T10:02:41Z

Hello, @tatiana, I made a minor change that assigns tags to tests based on their parent model. It works as expected but I am not sure if this is the best approach. Would love to get some feedback!

codecov · 2023-10-23T11:34:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c1a1406) 93.37% compared to head (61fdeaf) 93.38%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #615   +/-   ##
=======================================
  Coverage   93.37%   93.38%           
=======================================
  Files          53       53           
  Lines        2113     2115    +2     
=======================================
+ Hits         1973     1975    +2     
  Misses        140      140

Files	Coverage Δ
cosmos/dbt/selector.py	`96.58% <100.00%> (+0.05%)`	⬆️

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

tatiana

@edgga This seems like a very pragmatic solution to the problem, and it seems it allows us to have consistent behaviour on test tag-selection when using all the supported LoadMethod. Do you see any downside to this approach?

It would be great if we could add a test!

navedgaras · 2023-10-25T10:04:34Z

@tatiana I could not think of any downsides, but it's very possible I haven't thought about every existing use case. Right, I could add some tests, I would like to clarify - should I only add tags for testing purposes only in manifest.json, or should I also make changes to the underlying DBT project for consistency?

tatiana · 2023-10-25T10:13:02Z

@edgga Since ATM, this change is meant to solve a custom selection issue, we can probably have it where you've implemented it, for now. In the future, if we see the need for it, we can move this change to the DbtGraph - WDYT?

…sts based on the has_test attribute + specifying tag in manifest.json for a couple of models.

navedgaras · 2023-10-26T11:05:27Z

@tatiana, I have added a pretty basic test that checks the general functionality of the has_test attribute update when node selection based on a TAG is used. Not sure if that's enough though.

If you have any pointers on what else I need to do to get this merged, I am all ears, I would love to have this merged next week.

tatiana

@edgga thank you very much. This is looking very good!
I left a minor comment, and we'll release this as part of 1.2.2, next week.

tests/dbt/test_graph.py

…h isn't empty

tatiana

Thank you very much, @edgga !

…nd `LoadMethod.CUSTOM` (#615) Resolving issues with the DBT_MANIFEST/CUSTOM load methods when the has_test attribute is not assigned to the node correctly. ## Description When a tag selector is used, all tests are filtered out because of the DbtResourceType.TEST node does not have any information about tags. To bypass this limitation - tags are assigned to tests based on their parent model. ## Related Issue(s) Closes: #580 Co-authored-by: edgarasnavickas <[email protected]> (cherry picked from commit 58de67e)

@MrBones757

Bug fixes * Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in #634 * Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in #615 Others * Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in #629 * Update contributing docs for running integration tests by @jbandoro in #638 * Fix CI issue running integration tests by @tatiana in #640 and #644 * pre-commit updates in #637

…nd `LoadMethod.CUSTOM` (#615) Resolving issues with the DBT_MANIFEST/CUSTOM load methods when the has_test attribute is not assigned to the node correctly. ## Description When a tag selector is used, all tests are filtered out because of the DbtResourceType.TEST node does not have any information about tags. To bypass this limitation - tags are assigned to tests based on their parent model. ## Related Issue(s) Closes: #580 Co-authored-by: edgarasnavickas <[email protected]> (cherry picked from commit 58de67e)

@MrBones757

Bug fixes * Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in #634 * Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in #615 Others * Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in #629 * Update contributing docs for running integration tests by @jbandoro in #638 * Fix CI issue running integration tests by @tatiana in #640 and #644 * pre-commit updates in #637

@MrBones757

Bug fixes * Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in #634 * Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in #615 Others * Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in #629 * Update contributing docs for running integration tests by @jbandoro in #638 * Fix CI issue running integration tests by @tatiana in #640 and #644 * pre-commit updates in #637 (cherry picked from commit fa0620a)

@MrBones757

Bug fixes * Support ProjectConfig.dbt_project_path = None & different paths for Rendering and Execution by @MrBones757 in astronomer#634 * Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM by @edgga in astronomer#615 Others * Add pre-commit hook for McCabe max complexity check and fix errors by @jbandoro in astronomer#629 * Update contributing docs for running integration tests by @jbandoro in astronomer#638 * Fix CI issue running integration tests by @tatiana in astronomer#640 and astronomer#644 * pre-commit updates in astronomer#637 (cherry picked from commit fa0620a)

resolving issues with the DBT_MANIFEST load method when has_test attr…

31c1d4a

…ibute is not assigned to the node correctly

navedgaras requested a review from a team as a code owner October 19, 2023 13:39

navedgaras requested a review from a team October 19, 2023 13:39

navedgaras had a problem deploying to external October 19, 2023 13:39 — with GitHub Actions Error

🎨 [pre-commit.ci] Auto format from pre-commit.com hooks

b09fb28

pre-commit-ci bot had a problem deploying to external October 19, 2023 13:39 Error

accidentally removed a part of the docstring. Undoing

ce5b99f

navedgaras had a problem deploying to external October 19, 2023 13:42 — with GitHub Actions Error

specifying argument type for the node_dependencies argument

81bca50

navedgaras had a problem deploying to external October 19, 2023 13:45 — with GitHub Actions Error

navedgaras marked this pull request as draft October 19, 2023 13:46

🎨 [pre-commit.ci] Auto format from pre-commit.com hooks

998fafd

pre-commit-ci bot temporarily deployed to external October 19, 2023 13:48 Inactive

Merge remote-tracking branch 'origin/main' into manifest_render_metho…

cd3c257

…d_test_fix

navedgaras temporarily deployed to external October 23, 2023 07:59 — with GitHub Actions Inactive

tatiana added this to the 1.3.0 milestone Oct 23, 2023

tags are assigned to a test based on the parent model

6526ca4

navedgaras had a problem deploying to external October 23, 2023 09:50 — with GitHub Actions Error

adding newline which I for some reason removed in an earlier commit

78615d8

navedgaras temporarily deployed to external October 23, 2023 09:52 — with GitHub Actions Inactive

navedgaras marked this pull request as ready for review October 23, 2023 10:00

Merge branch 'main' into manifest_render_method_test_fix

a241f1b

navedgaras temporarily deployed to external October 24, 2023 06:06 — with GitHub Actions Inactive

tatiana reviewed Oct 25, 2023

View reviewed changes

adding a test that checks if models that are selected by tags have te…

adaab2c

…sts based on the has_test attribute + specifying tag in manifest.json for a couple of models.

navedgaras had a problem deploying to external October 26, 2023 10:43 — with GitHub Actions Error

🎨 [pre-commit.ci] Auto format from pre-commit.com hooks

01e870f

pre-commit-ci bot had a problem deploying to external October 26, 2023 10:44 Error

Merge branch 'main' into manifest_render_method_test_fix

3bfc8f9

navedgaras temporarily deployed to external October 26, 2023 10:48 — with GitHub Actions Inactive

missed the fact that a naming change was made to the DbtProject class

3a8bf12

navedgaras temporarily deployed to external October 26, 2023 11:01 — with GitHub Actions Inactive

fabiomx mentioned this pull request Oct 27, 2023

Tests not rendered with DBT_MANIFEST mode #631

Closed

tatiana modified the milestones: 1.3.0, 1.2.2 Oct 27, 2023

tatiana reviewed Oct 27, 2023

View reviewed changes

tests/dbt/test_graph.py Show resolved Hide resolved

adding an assert statement to ensure that a list we're looping throug…

bb58d82

…h isn't empty

navedgaras had a problem deploying to external October 30, 2023 08:02 — with GitHub Actions Error

Merge branch 'main' into manifest_render_method_test_fix

61fdeaf

navedgaras temporarily deployed to external October 30, 2023 08:03 — with GitHub Actions Inactive

tatiana approved these changes Oct 30, 2023

View reviewed changes

tatiana changed the title ~~resolving issues with the DBT_MANIFEST load method when has_test attr…~~ Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM Oct 30, 2023

tatiana merged commit 58de67e into astronomer:main Oct 30, 2023
40 checks passed

tatiana mentioned this pull request Nov 6, 2023

Release 1.2.2 #650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix adding test nodes to DAGs built using `LoadMethod.DBT_MANIFEST` and `LoadMethod.CUSTOM` #615

Fix adding test nodes to DAGs built using `LoadMethod.DBT_MANIFEST` and `LoadMethod.CUSTOM` #615

navedgaras commented Oct 19, 2023 •

edited

Loading

netlify bot commented Oct 19, 2023 •

edited

Loading

tatiana commented Oct 20, 2023

navedgaras commented Oct 23, 2023

codecov bot commented Oct 23, 2023 •

edited

Loading

tatiana left a comment

navedgaras commented Oct 25, 2023

tatiana commented Oct 25, 2023

navedgaras commented Oct 26, 2023

tatiana left a comment

tatiana left a comment

Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM #615

Fix adding test nodes to DAGs built using LoadMethod.DBT_MANIFEST and LoadMethod.CUSTOM #615

Conversation

navedgaras commented Oct 19, 2023 • edited Loading

Description

Related Issue(s)

Breaking Change?

Checklist

netlify bot commented Oct 19, 2023 • edited Loading

👷 Deploy Preview for amazing-pothos-a3bca0 processing.

tatiana commented Oct 20, 2023

navedgaras commented Oct 23, 2023

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

tatiana left a comment

Choose a reason for hiding this comment

navedgaras commented Oct 25, 2023

tatiana commented Oct 25, 2023

navedgaras commented Oct 26, 2023

tatiana left a comment

Choose a reason for hiding this comment

tatiana left a comment

Choose a reason for hiding this comment

Fix adding test nodes to DAGs built using `LoadMethod.DBT_MANIFEST` and `LoadMethod.CUSTOM` #615

Fix adding test nodes to DAGs built using `LoadMethod.DBT_MANIFEST` and `LoadMethod.CUSTOM` #615

navedgaras commented Oct 19, 2023 •

edited

Loading

netlify bot commented Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading