From 3e5a32b59e3531a86874f9e35273c86d0ce204b1 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 13 Jun 2024 17:51:09 -0700 Subject: [PATCH 1/5] Add basic semantic layer fixture nodes to unit test `manifest` fixture We're doing this in preperation to a for a unit test which will be testing these nodes (as well as others) and thus we want them in the manifest. --- tests/unit/utils/manifest.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/unit/utils/manifest.py b/tests/unit/utils/manifest.py index d1210694762..7db0a2a652a 100644 --- a/tests/unit/utils/manifest.py +++ b/tests/unit/utils/manifest.py @@ -1,4 +1,5 @@ from argparse import Namespace +from typing import List import pytest from dbt.artifacts.resources.v1.model import ModelConfig @@ -976,13 +977,18 @@ def unit_tests(unit_test_table_model) -> list: @pytest.fixture -def metrics() -> list: - return [] +def metrics(metric: Metric) -> List[Metric]: + return [metric] @pytest.fixture -def semantic_models() -> list: - return [] +def semantic_models(semantic_model: SemanticModel) -> List[SemanticModel]: + return [semantic_model] + + +@pytest.fixture +def saved_queries(saved_query: SavedQuery) -> List[SavedQuery]: + return [saved_query] @pytest.fixture @@ -1001,6 +1007,7 @@ def manifest( metrics, semantic_models, files, + saved_queries, ) -> Manifest: manifest = Manifest( nodes={n.unique_id: n for n in nodes}, @@ -1012,6 +1019,7 @@ def manifest( files=files, exposures={}, metrics={m.unique_id: m for m in metrics}, + saved_queries={s.unique_id: s for s in saved_queries}, disabled={}, selectors={}, groups={}, From 6bd0642d571c27ec8d332d108199fbf1fab31967 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 13 Jun 2024 17:55:02 -0700 Subject: [PATCH 2/5] Add `WhereFilterInteresection` to `QueryParams` of `saved_query` fixture NOTE: This was cherry-picked, and interestingly this specific commit, isn't entirely necessary. That is because the test this commit wasn't migrated to the new pytest `manifest` fixture when the `1.8.latest` branch was made. However, for the sake of simplicity, we're not dropping it from the range of commits we're cherry-picking because doing so creates extra confusion In the previous commit, 58990aa450c188b9c25346208853e43149aed877, we added the `saved_query` fixture to the `manifest` fixture. This broke the test `tests/unit/parser/test_manifest.py::TestPartialParse::test_partial_parse_by_version`. It broke because the `Manifest.deepcopy` manifest basically dictifies things. When we were dictifying the `QueryParams` of the `saved_query` before, the `where` key was getting dropped because it was `None`. We'd then run into a runtime error on instantiation of the `QueryParams` because although `where` is declared as _optional_, we don't set a default value for it. And thus, kaboom :( We should probably provide a default value for `where`. However that is out of scope for this branch of work. --- tests/unit/utils/manifest.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/utils/manifest.py b/tests/unit/utils/manifest.py index 7db0a2a652a..2596235dcf8 100644 --- a/tests/unit/utils/manifest.py +++ b/tests/unit/utils/manifest.py @@ -33,6 +33,8 @@ TestConfig, TestMetadata, RefArgs, + WhereFilter, + WhereFilterIntersection, ) from dbt.contracts.graph.unparsed import ( UnitTestInputFixture, @@ -855,7 +857,11 @@ def saved_query() -> SavedQuery: query_params=QueryParams( metrics=["my_metric"], group_by=[], - where=None, + where=WhereFilterIntersection( + where_filters=[ + WhereFilter(where_sql_template="1=1"), + ] + ), ), exports=[], unique_id=f"saved_query.{pkg}.{name}", From be812e2032682268631128e704fb97b38468eaba Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 13 Jun 2024 18:01:50 -0700 Subject: [PATCH 3/5] Fix `test_select_fqn` to account for newly included semantic model In 58990aa450c188b9c25346208853e43149aed877 we added a semantic model to the `manifest` fixture. This broke the test `tests/unit/graph/test_selector_methods.py::test_select_fqn` because in the test it selects nodes based on the string `*.*.*_model`. The newly added semantic model matches this, and thus needed to be added to the expected results. --- tests/unit/graph/test_selector_methods.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/graph/test_selector_methods.py b/tests/unit/graph/test_selector_methods.py index 58e414baafc..a796aadd79c 100644 --- a/tests/unit/graph/test_selector_methods.py +++ b/tests/unit/graph/test_selector_methods.py @@ -113,6 +113,7 @@ def test_select_fqn(manifest): assert search_manifest_using_method(manifest, method, "*.*.*_model") == { "mynamespace.union_model", "mynamespace.ephemeral_model", + "test_semantic_model", "union_model", "unit_test_table_model", } From 8dadd06ab717ee207f23db5cd6f9b2ab07bbc7d6 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 13 Jun 2024 18:25:00 -0700 Subject: [PATCH 4/5] Add unit tests for `_warn_for_unused_resource_config_paths` method Note: At this point the test when run with for a `unit_test` config taht should be considered used, fails. This is because it is not being properly identified as used. --- tests/unit/parser/test_manifest.py | 76 +++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index 6a643e444f3..4aea05b948d 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -4,9 +4,13 @@ from dbt.contracts.graph.manifest import Manifest -from dbt.parser.manifest import ManifestLoader +from dbt.events.types import UnusedResourceConfigPath +from dbt.parser.manifest import ManifestLoader, _warn_for_unused_resource_config_paths from dbt.config import RuntimeConfig from dbt.flags import set_from_args +from dbt_common.events.event_manager_client import add_callback_to_manager +from tests.functional.utils import EventCatcher +from tests.unit.utils import config_from_parts_or_dicts @pytest.fixture @@ -91,3 +95,73 @@ def test_partial_parse_safe_update_project_parser_files_partially( assert "full_reparse_reason" in exc_info assert "KeyError: 'Whoopsie!'" == exc_info["exception"] assert isinstance(exc_info["code"], str) or isinstance(exc_info["code"], type(None)) + + +class TestWarnUnusedConfigs: + @pytest.fixture + def runtime_config(self) -> RuntimeConfig: + profile_cfg = { + "outputs": { + "test": { + "type": "postgres", + "dbname": "postgres", + "user": "test", + "host": "test", + "pass": "test", + "port": 5432, + "schema": "test", + }, + }, + "target": "test", + } + + project_cfg = { + "name": "query_headers", + "version": "0.1", + "profile": "test", + "config-version": 2, + } + + return config_from_parts_or_dicts(project=project_cfg, profile=profile_cfg) + + @pytest.mark.parametrize( + "resource_type,path,expect_used", + [ + ("data_tests", "unused_path", False), + ("data_tests", "minimal", True), + ("metrics", "unused_path", False), + ("metrics", "test", True), + ("models", "unused_path", False), + ("models", "pkg", True), + ("saved_queries", "unused_path", False), + ("saved_queries", "test", True), + ("seeds", "unused_path", False), + ("seeds", "pkg", True), + ("semantic_models", "unused_path", False), + ("semantic_models", "test", True), + ("sources", "unused_path", False), + ("sources", "pkg", True), + ("unit_tests", "unused_path", False), + ("unit_tests", "pkg", True), + ], + ) + def test_warn_for_unused_resource_config_paths( + self, + resource_type: str, + path: str, + expect_used: bool, + manifest: Manifest, + runtime_config: RuntimeConfig, + ) -> None: + catcher = EventCatcher(UnusedResourceConfigPath) + add_callback_to_manager(catcher.catch) + + setattr(runtime_config, resource_type, {path: {"+materialized": "table"}}) + + _warn_for_unused_resource_config_paths(manifest=manifest, config=runtime_config) + + if expect_used: + assert len(catcher.caught_events) == 0 + else: + assert len(catcher.caught_events) == 1 + assert f"{resource_type}.{path}" in str(catcher.caught_events[0].data) From 737da2137226cf2561e1e6c65207805d47241984 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 13 Jun 2024 18:32:02 -0700 Subject: [PATCH 5/5] Include `unit_tests` in `Manifest.get_resouce_fqns` Because `unit_tests` weren't being included in calls to `Manifest.get_resource.fqns`, it always appeared to `_warn_for_unused_resource_config_paths` that there were no unit tests in the manifest. Because of this `_warn_for_unused_resource_config_paths` thought that _any_ `unit_test` config in `dbt_project.yaml` was unused. --- .changes/unreleased/Fixes-20240613-183117.yaml | 6 ++++++ core/dbt/contracts/graph/manifest.py | 1 + 2 files changed, 7 insertions(+) create mode 100644 .changes/unreleased/Fixes-20240613-183117.yaml diff --git a/.changes/unreleased/Fixes-20240613-183117.yaml b/.changes/unreleased/Fixes-20240613-183117.yaml new file mode 100644 index 00000000000..14b1ee2bf08 --- /dev/null +++ b/.changes/unreleased/Fixes-20240613-183117.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: DOn't warn on `unit_test` config paths that are properly used +time: 2024-06-13T18:31:17.486497-07:00 +custom: + Author: QMalcolm + Issue: "10311" diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 7138087d446..43209b6f9eb 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -986,6 +986,7 @@ def get_resource_fqns(self) -> Mapping[str, PathSet]: self.metrics.values(), self.semantic_models.values(), self.saved_queries.values(), + self.unit_tests.values(), ) for resource in all_resources: resource_type_plural = resource.resource_type.pluralize()