From f683e36468aac50957b60e5c3ab8d73b24f5e23b Mon Sep 17 00:00:00 2001 From: Kshitij Aranke Date: Wed, 27 Mar 2024 15:58:16 +0000 Subject: [PATCH] Fix #9608: Unit test path outputs (#9793) --- .../unreleased/Fixes-20240326-003411.yaml | 6 ++ core/dbt/contracts/graph/nodes.py | 10 --- core/dbt/parser/unit_tests.py | 22 +++---- core/dbt/task/printer.py | 2 +- core/dbt/task/test.py | 28 ++++++--- .../unit_testing/test_unit_testing.py | 61 +++++++++++++++++++ .../functional/unit_testing/test_ut_names.py | 16 +++-- 7 files changed, 105 insertions(+), 40 deletions(-) create mode 100644 .changes/unreleased/Fixes-20240326-003411.yaml diff --git a/.changes/unreleased/Fixes-20240326-003411.yaml b/.changes/unreleased/Fixes-20240326-003411.yaml new file mode 100644 index 00000000000..f5b5fe9e095 --- /dev/null +++ b/.changes/unreleased/Fixes-20240326-003411.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Unit test path outputs +time: 2024-03-26T00:34:11.162594Z +custom: + Author: aranke + Issue: "9608" diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index e62bcdafb48..134c272db23 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -954,16 +954,6 @@ class UnitTestDefinition(NodeInfoMixin, GraphNode, UnitTestDefinitionResource): def resource_class(cls) -> Type[UnitTestDefinitionResource]: return UnitTestDefinitionResource - @property - def build_path(self): - # TODO: is this actually necessary? - return self.original_file_path - - @property - def compiled_path(self): - # TODO: is this actually necessary? - return self.original_file_path - @property def depends_on_nodes(self): return self.depends_on.nodes diff --git a/core/dbt/parser/unit_tests.py b/core/dbt/parser/unit_tests.py index 1355a29f671..763efab44aa 100644 --- a/core/dbt/parser/unit_tests.py +++ b/core/dbt/parser/unit_tests.py @@ -120,10 +120,12 @@ def parse_unit_test_case(self, test_case: UnitTestDefinition): original_input_node = self._get_original_input_node( given.input, tested_node, test_case.name ) + input_name = original_input_node.name common_fields = { "resource_type": NodeType.Model, - "original_file_path": original_input_node.original_file_path, + # root directory for input and output fixtures + "original_file_path": unit_test_node.original_file_path, "config": ModelConfig(materialized="ephemeral"), "database": original_input_node.database, "alias": original_input_node.identifier, @@ -131,6 +133,10 @@ def parse_unit_test_case(self, test_case: UnitTestDefinition): "fqn": original_input_node.fqn, "checksum": FileHash.empty(), "raw_code": self._build_fixture_raw_code(given.rows, None), + "package_name": original_input_node.package_name, + "unique_id": f"model.{original_input_node.package_name}.{input_name}", + "name": input_name, + "path": f"{input_name}.sql", } if original_input_node.resource_type in ( @@ -138,14 +144,7 @@ def parse_unit_test_case(self, test_case: UnitTestDefinition): NodeType.Seed, NodeType.Snapshot, ): - input_name = original_input_node.name - input_node = ModelNode( - **common_fields, - package_name=original_input_node.package_name, - unique_id=f"model.{original_input_node.package_name}.{input_name}", - name=input_name, - path=original_input_node.path or f"{input_name}.sql", - ) + input_node = ModelNode(**common_fields) if ( original_input_node.resource_type == NodeType.Model and original_input_node.version @@ -156,13 +155,8 @@ def parse_unit_test_case(self, test_case: UnitTestDefinition): # We are reusing the database/schema/identifier from the original source, # but that shouldn't matter since this acts as an ephemeral model which just # wraps a CTE around the unit test node. - input_name = original_input_node.name input_node = UnitTestSourceDefinition( **common_fields, - package_name=original_input_node.package_name, - unique_id=f"model.{original_input_node.package_name}.{input_name}", - name=original_input_node.name, # must be the same name for source lookup to work - path=input_name + ".sql", # for writing out compiled_code source_name=original_input_node.source_name, # needed for source lookup ) # Sources need to go in the sources dictionary in order to create the right lookup diff --git a/core/dbt/task/printer.py b/core/dbt/task/printer.py index e39c9548f1e..1e4d37878ab 100644 --- a/core/dbt/task/printer.py +++ b/core/dbt/task/printer.py @@ -111,7 +111,7 @@ def print_run_result_error(result, newline: bool = True, is_warning: bool = Fals else: fire_event(RunResultErrorNoMessage(status=result.status, node_info=node_info)) - if result.node.build_path is not None: + if result.node.compiled_path is not None: with TextOnly(): fire_event(Formatting("")) fire_event(SQLCompiledPath(path=result.node.compiled_path, node_info=node_info)) diff --git a/core/dbt/task/test.py b/core/dbt/task/test.py index d3b16b88bc0..7f5ed77f7e1 100644 --- a/core/dbt/task/test.py +++ b/core/dbt/task/test.py @@ -7,12 +7,18 @@ from dbt_common.events.format import pluralize from dbt_common.dataclass_schema import dbtClassMixin import threading -from typing import Dict, Any, Optional, Union, List, TYPE_CHECKING +from typing import Dict, Any, Optional, Union, List, TYPE_CHECKING, Tuple from .compile import CompileRunner from .run import RunTask -from dbt.contracts.graph.nodes import TestNode, UnitTestDefinition, UnitTestNode +from dbt.contracts.graph.nodes import ( + TestNode, + UnitTestDefinition, + UnitTestNode, + GenericTestNode, + SingularTestNode, +) from dbt.contracts.graph.manifest import Manifest from dbt.artifacts.schemas.results import TestStatus from dbt.artifacts.schemas.run import RunResult @@ -180,7 +186,7 @@ def build_unit_test_manifest_from_test( def execute_unit_test( self, unit_test_def: UnitTestDefinition, manifest: Manifest - ) -> UnitTestResultData: + ) -> Tuple[UnitTestNode, UnitTestResultData]: unit_test_manifest = self.build_unit_test_manifest_from_test(unit_test_def, manifest) @@ -190,6 +196,7 @@ def execute_unit_test( # Compile the node unit_test_node = self.compiler.compile_node(unit_test_node, unit_test_manifest, {}) + assert isinstance(unit_test_node, UnitTestNode) # generate_runtime_unit_test_context not strictly needed - this is to run the 'unit' # materialization, not compile the node.compiled_code @@ -243,18 +250,21 @@ def execute_unit_test( rendered=rendered, ) - return UnitTestResultData( + unit_test_result_data = UnitTestResultData( diff=diff, should_error=should_error, adapter_response=adapter_response, ) - def execute(self, test: Union[TestNode, UnitTestDefinition], manifest: Manifest): + return unit_test_node, unit_test_result_data + + def execute(self, test: Union[TestNode, UnitTestNode], manifest: Manifest): if isinstance(test, UnitTestDefinition): - unit_test_result = self.execute_unit_test(test, manifest) - return self.build_unit_test_run_result(test, unit_test_result) + unit_test_node, unit_test_result = self.execute_unit_test(test, manifest) + return self.build_unit_test_run_result(unit_test_node, unit_test_result) else: # Note: manifest here is a normal manifest + assert isinstance(test, (SingularTestNode, GenericTestNode)) test_result = self.execute_data_test(test, manifest) return self.build_test_run_result(test, test_result) @@ -293,7 +303,7 @@ def build_test_run_result(self, test: TestNode, result: TestResultData) -> RunRe return run_result def build_unit_test_run_result( - self, test: UnitTestDefinition, result: UnitTestResultData + self, test: UnitTestNode, result: UnitTestResultData ) -> RunResult: thread_id = threading.current_thread().name @@ -306,7 +316,7 @@ def build_unit_test_run_result( failures = 1 return RunResult( - node=test, # type: ignore + node=test, status=status, timing=[], thread_id=thread_id, diff --git a/tests/functional/unit_testing/test_unit_testing.py b/tests/functional/unit_testing/test_unit_testing.py index c5ce83d2eb3..887c1907e76 100644 --- a/tests/functional/unit_testing/test_unit_testing.py +++ b/tests/functional/unit_testing/test_unit_testing.py @@ -5,6 +5,9 @@ run_dbt, write_file, get_manifest, + run_dbt_and_capture, + read_file, + file_exists, ) from dbt.contracts.results import NodeStatus from dbt.exceptions import DuplicateResourceNameError, ParsingError @@ -30,6 +33,7 @@ test_my_model_incremental_yml_wrong_override, test_my_model_incremental_yml_no_this_input, ) +from tests.unit.utils import normalize class TestUnitTests: @@ -442,3 +446,60 @@ def test_unit_test_ext_nodes( run_dbt(["run"], expect_pass=True) results = run_dbt(["test", "--select", "valid_emails"], expect_pass=True) assert len(results) == 1 + + +subfolder_model_a_sql = """select 1 as id, 'blue' as color""" + +subfolder_model_b_sql = """ +select + id, + color +from {{ ref('model_a') }} +""" + +subfolder_my_model_yml = """ +unit_tests: + - name: my_unit_test + model: model_b + given: + - input: ref('model_a') + rows: + - { id: 1, color: 'blue' } + expect: + rows: + - { id: 1, color: 'red' } +""" + + +class TestUnitTestSubfolderPath: + @pytest.fixture(scope="class") + def models(self): + return { + "subfolder": { + "model_a.sql": subfolder_model_a_sql, + "model_b.sql": subfolder_model_b_sql, + "my_model.yml": subfolder_my_model_yml, + } + } + + def test_subfolder_unit_test(self, project): + results, output = run_dbt_and_capture(["build"], expect_pass=False) + + # Test that input fixture doesn't overwrite the original model + assert ( + read_file("target/compiled/test/models/subfolder/model_a.sql").strip() + == subfolder_model_a_sql.strip() + ) + + # Test that correct path is written in logs + assert ( + normalize( + "target/compiled/test/models/subfolder/my_model.yml/models/subfolder/my_unit_test.sql" + ) + in output + ) + assert file_exists( + normalize( + "target/compiled/test/models/subfolder/my_model.yml/models/subfolder/my_unit_test.sql" + ) + ) diff --git a/tests/functional/unit_testing/test_ut_names.py b/tests/functional/unit_testing/test_ut_names.py index 27c0a56201c..d1721438576 100644 --- a/tests/functional/unit_testing/test_ut_names.py +++ b/tests/functional/unit_testing/test_ut_names.py @@ -27,33 +27,37 @@ def test_duplicate_test_names_across_models(self, project): # Select duplicate tests results, log_output = run_dbt_and_capture(["test"], expect_pass=True) assert len(results) == 2 - assert ["my_model_a", "my_model_b"] == sorted([result.node.model for result in results]) + assert {"model.test.my_model_a", "model.test.my_model_b"} == { + result.node.tested_node_unique_id for result in results + } assert "my_model_a::my_test_name" in log_output assert "my_model_b::my_test_name" in log_output # Test select duplicates by by test name results = run_dbt(["test", "--select", "test_name:my_test_name"]) assert len(results) == 2 - assert ["my_model_a", "my_model_b"] == sorted([result.node.model for result in results]) + assert {"model.test.my_model_a", "model.test.my_model_b"} == { + result.node.tested_node_unique_id for result in results + } assert "my_model_a::my_test_name" in log_output assert "my_model_b::my_test_name" in log_output results = run_dbt(["test", "--select", "my_model_a,test_name:my_test_name"]) assert len(results) == 1 - assert results[0].node.model == "my_model_a" + assert results[0].node.tested_node_unique_id == "model.test.my_model_a" results = run_dbt(["test", "--select", "my_model_b,test_name:my_test_name"]) assert len(results) == 1 - assert results[0].node.model == "my_model_b" + assert results[0].node.tested_node_unique_id == "model.test.my_model_b" # Test select by model name results = run_dbt(["test", "--select", "my_model_a"]) assert len(results) == 1 - assert results[0].node.model == "my_model_a" + assert results[0].node.tested_node_unique_id == "model.test.my_model_a" results = run_dbt(["test", "--select", "my_model_b"]) assert len(results) == 1 - assert results[0].node.model == "my_model_b" + assert results[0].node.tested_node_unique_id == "model.test.my_model_b" class TestUnitTestDuplicateTestNamesWithinModel: