Skip to content

Commit

Permalink
Stop mis-identifying unit_test config paths in dbt_project.yaml a…
Browse files Browse the repository at this point in the history
…s unused (#10312)

* 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

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 14, 2024
1 parent 8ee8b25 commit 100352d
Show file tree
Hide file tree
Showing 5 changed files with 76 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 @@ -988,6 +988,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
49 changes: 48 additions & 1 deletion tests/unit/parser/test_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
from dbt.artifacts.resources.base import FileHash
from dbt.config import RuntimeConfig
from dbt.contracts.graph.manifest import Manifest, ManifestStateCheck
from dbt.events.types import UnusedResourceConfigPath
from dbt.flags import set_from_args
from dbt.parser.manifest import ManifestLoader
from dbt.parser.manifest import ManifestLoader, _warn_for_unused_resource_config_paths
from dbt.parser.read_files import FileDiff
from dbt.tracking import User
from dbt_common.events.event_manager_client import add_callback_to_manager
from tests.utils import EventCatcher


class TestPartialParse:
Expand Down Expand Up @@ -191,3 +194,47 @@ def test_partial_parse_file_diff_flag(
set_from_args(Namespace(PARTIAL_PARSE_FILE_DIFF=False), {})
ManifestLoader.get_full_manifest(config=mock_project)
assert mock_file_diff.called


class TestWarnUnusedConfigs:
@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)
25 changes: 20 additions & 5 deletions tests/unit/utils/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
RefArgs,
TestConfig,
TestMetadata,
WhereFilter,
WhereFilterIntersection,
)
from dbt.artifacts.resources.v1.model import ModelConfig
from dbt.contracts.files import AnySourceFile, FileHash
Expand Down Expand Up @@ -851,7 +853,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 @@ -973,13 +979,18 @@ def unit_tests(unit_test_table_model) -> List[UnitTestDefinition]:


@pytest.fixture
def metrics() -> List[Metric]:
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[SemanticModel]:
return []
def saved_queries(saved_query: SavedQuery) -> List[SavedQuery]:
return [saved_query]


@pytest.fixture
Expand All @@ -996,6 +1007,7 @@ def make_manifest(
macros: List[Macro] = [],
metrics: List[Metric] = [],
nodes: List[ModelNode] = [],
saved_queries: List[SavedQuery] = [],
selectors: Dict[str, Any] = {},
semantic_models: List[SemanticModel] = [],
sources: List[SourceDefinition] = [],
Expand All @@ -1015,6 +1027,7 @@ def make_manifest(
selectors=selectors,
groups={g.unique_id: g for g in groups},
metadata=ManifestMetadata(adapter_type="postgres", project_name="pkg"),
saved_queries={s.unique_id: s for s in saved_queries},
)
manifest.build_parent_and_child_maps()
return manifest
Expand All @@ -1031,6 +1044,7 @@ def manifest(
metrics,
semantic_models,
files,
saved_queries,
) -> Manifest:
return make_manifest(
nodes=nodes,
Expand All @@ -1040,4 +1054,5 @@ def manifest(
semantic_models=semantic_models,
files=files,
metrics=metrics,
saved_queries=saved_queries,
)

0 comments on commit 100352d

Please sign in to comment.