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

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

Merged
merged 12 commits into from
Jun 14, 2024
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20240606-112334.yaml
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
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

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