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.

Sign up for GitHub

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

Call adapter.pre_model_hook + adapter.post_model_hook within tests #10258

Merged
merged 12 commits into from
Jun 14, 2024
Merged

Call adapter.pre_model_hook + adapter.post_model_hook within tests #10258

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ef67cff
init push for issue 10198
McKnight-42 Jun 3, 2024
c8620c7
update to main
McKnight-42 Jun 6, 2024
62280b2
add changelog
McKnight-42 Jun 6, 2024
09ead95
add unit tests based on michelle example
McKnight-42 Jun 10, 2024
7648500
add data_tests, and post_hook unit tests
McKnight-42 Jun 11, 2024
3e35e91
Merge branch 'main' of https://github.com/dbt-labs/dbt-core into mckn…
McKnight-42 Jun 12, 2024
c15cc74
pull creating macro_func out of try call
McKnight-42 Jun 12, 2024
1b2c839
revert last commit
McKnight-42 Jun 12, 2024
ca29c03
pull macro_func definition back out of try
McKnight-42 Jun 12, 2024
685ccdb
Merge branch 'main' of https://github.com/dbt-labs/dbt-core into mckn…
McKnight-42 Jun 13, 2024
928e63f
Merge branch 'main' of https://github.com/dbt-labs/dbt-core into mckn…
McKnight-42 Jun 13, 2024
fa98fbd
update code formatting
McKnight-42 Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20240606-112334.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: add pre_model and post_model hoook calls to data and unit tests to be able to provide extra config options
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
time: 2024-06-06T11:23:34.758675-05:00
custom:
Author: McKnight-42
Issue: "10198"
23 changes: 16 additions & 7 deletions core/dbt/task/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@
def execute_data_test(self, data_test: TestNode, manifest: Manifest) -> TestResultData:
context = generate_runtime_model_context(data_test, self.config, manifest)

hook_ctx = self.adapter.pre_model_hook(context)

Check warning on line 129 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L129 

Added line #L129 was not covered by tests
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

materialization_macro = manifest.find_materialization_macro_by_name(
self.config.project_name, data_test.get_materialization(), self.adapter.type()
)
Expand All @@ -140,10 +142,13 @@
"Invalid materialization context generated, missing config: {}".format(context)
)

# generate materialization macro
macro_func = MacroGenerator(materialization_macro, context)
# execute materialization macro
macro_func()
try:

Check warning on line 145 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L145 

Added line #L145 was not covered by tests
# generate materialization macro
macro_func = MacroGenerator(materialization_macro, context)

Check warning on line 147 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L147 

Added line #L147 was not covered by tests
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
# execute materialization macro
macro_func()

Check warning on line 149 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L149 

Added line #L149 was not covered by tests
finally:
self.adapter.post_model_hook(context, hook_ctx)

Check warning on line 151 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L151 

Added line #L151 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, this is not a behaviour change because:

cc @jtcohen6

Copy link
Contributor Author

@McKnight-42 McKnight-42 Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think i can help supply a answer here. with this pr and

as tested in dbt-labs/dbt-snowflake#1070

Copy link
Contributor

Choose a reason for hiding this comment

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

ah! gotcha -- so configs were not possible to provide for generic tests in the past, which means there should not be any currently set on them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from my understanding yes, we could use a old pattern to set a set number of accepted configs linked in the generic_config issue, making this update to the newer syntax while maintaining the old will allow us to do this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @MichelleArk @McKnight-42 for thinking of this!

As I see it, the only way in which this could be a "breaking" change is if someone had configured a singular test, or all tests (from dbt_project.yml), with a snowflake_warehouse that they didn't actually want to be used.

Something like:

# dbt_project.yml
tests:
  +snowflake_warehouse: does_not_exist

-- tests/my_singular_test.sql
{{ config(snowflake_warehouse = 'wrong_warehouse') }}

select ...

I do not see that as a breaking change; I see it as this feature in dbt (specifically dbt-snowflake) finally working as intended. As such, I do not believe we need to put it behind a legacy behavior migration flag.

# load results from context
# could eventually be returned directly by materialization
result = context["load_result"]("main")
Expand Down Expand Up @@ -198,6 +203,8 @@
# materialization, not compile the node.compiled_code
context = generate_runtime_model_context(unit_test_node, self.config, unit_test_manifest)

hook_ctx = self.adapter.pre_model_hook(context)

Check warning on line 206 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L206 

Added line #L206 was not covered by tests

materialization_macro = unit_test_manifest.find_materialization_macro_by_name(
self.config.project_name, unit_test_node.get_materialization(), self.adapter.type()
)
Expand All @@ -213,16 +220,18 @@
"Invalid materialization context generated, missing config: {}".format(context)
)

# generate materialization macro
macro_func = MacroGenerator(materialization_macro, context)
# execute materialization macro
try:
# generate materialization macro
macro_func = MacroGenerator(materialization_macro, context)

Check warning on line 225 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L225 

Added line #L225 was not covered by tests
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
# execute materialization macro
macro_func()
except DbtBaseException as e:
raise DbtRuntimeError(
f"An error occurred during execution of unit test '{unit_test_def.name}'. "
f"There may be an error in the unit test definition: check the data types.\n {e}"
)
finally:
self.adapter.post_model_hook(context, hook_ctx)

Check warning on line 234 in core/dbt/task/test.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/test.py#L234 

Added line #L234 was not covered by tests

# load results from context
# could eventually be returned directly by materialization
Expand Down
Loading