Skip to content

Commit

Permalink
[Backport 1.8.latest] Stop mis-identifying unit_test config paths in …
Browse files Browse the repository at this point in the history
…dbt_project.yaml as unused (#10314)

* 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.

* 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, 58990aa, 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.

* Fix `test_select_fqn` to account for newly included semantic model

In 58990aa 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.

* 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.

* 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.
  • Loading branch information
QMalcolm authored Jun 17, 2024
1 parent 20eecad commit 341d025
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 6 deletions.
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20240613-183117.yaml
Original file line number Diff line number Diff line change
@@ -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"
1 change: 1 addition & 0 deletions core/dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions tests/unit/graph/test_selector_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}
Expand Down
76 changes: 75 additions & 1 deletion tests/unit/parser/test_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
24 changes: 19 additions & 5 deletions tests/unit/utils/manifest.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from argparse import Namespace
from typing import List
import pytest

from dbt.artifacts.resources.v1.model import ModelConfig
Expand Down Expand Up @@ -32,6 +33,8 @@
TestConfig,
TestMetadata,
RefArgs,
WhereFilter,
WhereFilterIntersection,
)
from dbt.contracts.graph.unparsed import (
UnitTestInputFixture,
Expand Down Expand Up @@ -854,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}",
Expand Down Expand Up @@ -976,13 +983,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(semantic_model: SemanticModel) -> List[SemanticModel]:
return [semantic_model]


@pytest.fixture
def semantic_models() -> list:
return []
def saved_queries(saved_query: SavedQuery) -> List[SavedQuery]:
return [saved_query]


@pytest.fixture
Expand All @@ -1001,6 +1013,7 @@ def manifest(
metrics,
semantic_models,
files,
saved_queries,
) -> Manifest:
manifest = Manifest(
nodes={n.unique_id: n for n in nodes},
Expand All @@ -1012,6 +1025,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={},
Expand Down

0 comments on commit 341d025

Please sign in to comment.