From 8c481dc050c0c8c8b8953ee54ca622d32f2f81e5 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 6 Jun 2024 16:29:15 -0700 Subject: [PATCH 1/7] Rename `maniest` fixture in `test_selector` to `mock_manifest` We have a globally available `manifest` fixture in our unit tests. In the coming commits we're going to add tests to the file which use the gloablly available `manifest` fixture. Prior to this commit, the locally defined `manifest` fixture was taking precidence. To get around this, the easiest solution was to rename the locally defined fixture. I had tried to isolate the locally defined fixture by moving it, and the relevant tests to a test class like `TestNodeSelector`. However because of _how_ the relevant tests were parameterized, this proved difficult. Basically for readability we define a variable which holds a list of all the parameterization variables. By moving to a test class, the definition of the variables would have had to be defined directly in the parameterization macro call. Although possible, it made the readability slighty worse. It might be worth doing anyway in the long run, but instead I used a less heavy handed alternative (already described) --- tests/unit/graph/test_selector.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/graph/test_selector.py b/tests/unit/graph/test_selector.py index 677fb1c46bd..229ac7983a8 100644 --- a/tests/unit/graph/test_selector.py +++ b/tests/unit/graph/test_selector.py @@ -82,7 +82,7 @@ def graph(): @pytest.fixture -def manifest(graph): +def mock_manifest(graph): return _get_manifest(graph) @@ -142,8 +142,8 @@ def id_macro(arg): @pytest.mark.parametrize("include,exclude,expected", run_specs, ids=id_macro) -def test_run_specs(include, exclude, expected, graph, manifest): - selector = graph_selector.NodeSelector(graph, manifest) +def test_run_specs(include, exclude, expected, graph, mock_manifest): + selector = graph_selector.NodeSelector(graph, mock_manifest) spec = graph_cli.parse_difference(include, exclude) selected, _ = selector.select_nodes(spec) From 3ad6c20d15a3e762378bc50ad2147c0a5570ecf4 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 6 Jun 2024 17:46:05 -0700 Subject: [PATCH 2/7] Improve type hinting in `tests/unit/utils/manifest.py` --- tests/unit/utils/manifest.py | 79 +++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/tests/unit/utils/manifest.py b/tests/unit/utils/manifest.py index a7c269cdab2..d7902cfc682 100644 --- a/tests/unit/utils/manifest.py +++ b/tests/unit/utils/manifest.py @@ -1,3 +1,5 @@ +from typing import Dict, List + import pytest from dbt_semantic_interfaces.type_enums import MetricType @@ -14,7 +16,7 @@ TestMetadata, ) from dbt.artifacts.resources.v1.model import ModelConfig -from dbt.contracts.files import FileHash +from dbt.contracts.files import AnySourceFile, FileHash from dbt.contracts.graph.manifest import Manifest, ManifestMetadata from dbt.contracts.graph.nodes import ( AccessType, @@ -23,6 +25,7 @@ GenericTestNode, Group, Macro, + ManifestNode, Metric, ModelNode, NodeConfig, @@ -501,41 +504,41 @@ def make_saved_query(pkg: str, name: str, metric: str, path=None): @pytest.fixture -def macro_test_unique(): +def macro_test_unique() -> Macro: return make_macro( "dbt", "test_unique", "blablabla", depends_on_macros=["macro.dbt.default__test_unique"] ) @pytest.fixture -def macro_default_test_unique(): +def macro_default_test_unique() -> Macro: return make_macro("dbt", "default__test_unique", "blablabla") @pytest.fixture -def macro_test_not_null(): +def macro_test_not_null() -> Macro: return make_macro( "dbt", "test_not_null", "blablabla", depends_on_macros=["macro.dbt.default__test_not_null"] ) @pytest.fixture -def macro_default_test_not_null(): +def macro_default_test_not_null() -> Macro: return make_macro("dbt", "default__test_not_null", "blabla") @pytest.fixture -def seed(): +def seed() -> SeedNode: return make_seed("pkg", "seed") @pytest.fixture -def source(): +def source() -> SourceDefinition: return make_source("pkg", "raw", "seed", identifier="seed") @pytest.fixture -def ephemeral_model(source): +def ephemeral_model(source) -> ModelNode: return make_model( "pkg", "ephemeral_model", @@ -546,7 +549,7 @@ def ephemeral_model(source): @pytest.fixture -def view_model(ephemeral_model): +def view_model(ephemeral_model) -> ModelNode: return make_model( "pkg", "view_model", @@ -558,7 +561,7 @@ def view_model(ephemeral_model): @pytest.fixture -def table_model(ephemeral_model): +def table_model(ephemeral_model) -> ModelNode: return make_model( "pkg", "table_model", @@ -580,7 +583,7 @@ def table_model(ephemeral_model): @pytest.fixture -def table_model_py(seed): +def table_model_py(seed) -> ModelNode: return make_model( "pkg", "table_model_py", @@ -593,7 +596,7 @@ def table_model_py(seed): @pytest.fixture -def table_model_csv(seed): +def table_model_csv(seed) -> ModelNode: return make_model( "pkg", "table_model_csv", @@ -606,7 +609,7 @@ def table_model_csv(seed): @pytest.fixture -def ext_source(): +def ext_source() -> SourceDefinition: return make_source( "ext", "ext_raw", @@ -615,7 +618,7 @@ def ext_source(): @pytest.fixture -def ext_source_2(): +def ext_source_2() -> SourceDefinition: return make_source( "ext", "ext_raw", @@ -624,7 +627,7 @@ def ext_source_2(): @pytest.fixture -def ext_source_other(): +def ext_source_other() -> SourceDefinition: return make_source( "ext", "raw", @@ -633,7 +636,7 @@ def ext_source_other(): @pytest.fixture -def ext_source_other_2(): +def ext_source_other_2() -> SourceDefinition: return make_source( "ext", "raw", @@ -642,7 +645,7 @@ def ext_source_other_2(): @pytest.fixture -def ext_model(ext_source): +def ext_model(ext_source) -> ModelNode: return make_model( "ext", "ext_model", @@ -652,7 +655,7 @@ def ext_model(ext_source): @pytest.fixture -def union_model(seed, ext_source): +def union_model(seed, ext_source) -> ModelNode: return make_model( "pkg", "union_model", @@ -667,7 +670,7 @@ def union_model(seed, ext_source): @pytest.fixture -def versioned_model_v1(seed): +def versioned_model_v1(seed) -> ModelNode: return make_model( "pkg", "versioned_model", @@ -682,7 +685,7 @@ def versioned_model_v1(seed): @pytest.fixture -def versioned_model_v2(seed): +def versioned_model_v2(seed) -> ModelNode: return make_model( "pkg", "versioned_model", @@ -697,7 +700,7 @@ def versioned_model_v2(seed): @pytest.fixture -def versioned_model_v3(seed): +def versioned_model_v3(seed) -> ModelNode: return make_model( "pkg", "versioned_model", @@ -712,7 +715,7 @@ def versioned_model_v3(seed): @pytest.fixture -def versioned_model_v12_string(seed): +def versioned_model_v12_string(seed) -> ModelNode: return make_model( "pkg", "versioned_model", @@ -727,7 +730,7 @@ def versioned_model_v12_string(seed): @pytest.fixture -def versioned_model_v4_nested_dir(seed): +def versioned_model_v4_nested_dir(seed) -> ModelNode: return make_model( "pkg", "versioned_model", @@ -743,27 +746,27 @@ def versioned_model_v4_nested_dir(seed): @pytest.fixture -def table_id_unique(table_model): +def table_id_unique(table_model) -> GenericTestNode: return make_unique_test("pkg", table_model, "id") @pytest.fixture -def table_id_not_null(table_model): +def table_id_not_null(table_model) -> GenericTestNode: return make_not_null_test("pkg", table_model, "id") @pytest.fixture -def view_id_unique(view_model): +def view_id_unique(view_model) -> GenericTestNode: return make_unique_test("pkg", view_model, "id") @pytest.fixture -def ext_source_id_unique(ext_source): +def ext_source_id_unique(ext_source) -> GenericTestNode: return make_unique_test("ext", ext_source, "id") @pytest.fixture -def view_test_nothing(view_model): +def view_test_nothing(view_model) -> SingularTestNode: return make_singular_test( "pkg", "view_test_nothing", @@ -773,7 +776,7 @@ def view_test_nothing(view_model): @pytest.fixture -def unit_test_table_model(table_model): +def unit_test_table_model(table_model) -> UnitTestDefinition: return make_unit_test( "pkg", "unit_test_table_model", @@ -783,12 +786,12 @@ def unit_test_table_model(table_model): # Support dots as namespace separators @pytest.fixture -def namespaced_seed(): +def namespaced_seed() -> SeedNode: return make_seed("pkg", "mynamespace.seed") @pytest.fixture -def namespace_model(source): +def namespace_model(source) -> ModelNode: return make_model( "pkg", "mynamespace.ephemeral_model", @@ -799,7 +802,7 @@ def namespace_model(source): @pytest.fixture -def namespaced_union_model(seed, ext_source): +def namespaced_union_model(seed, ext_source) -> ModelNode: return make_model( "pkg", "mynamespace.union_model", @@ -910,7 +913,7 @@ def nodes( namespaced_seed, namespace_model, namespaced_union_model, -) -> list: +) -> List[ManifestNode]: return [ seed, ephemeral_model, @@ -953,7 +956,7 @@ def macros( macro_default_test_unique, macro_test_not_null, macro_default_test_not_null, -) -> list: +) -> List[Macro]: return [ macro_test_unique, macro_default_test_unique, @@ -963,22 +966,22 @@ def macros( @pytest.fixture -def unit_tests(unit_test_table_model) -> list: +def unit_tests(unit_test_table_model) -> List[UnitTestDefinition]: return [unit_test_table_model] @pytest.fixture -def metrics() -> list: +def metrics() -> List[Metric]: return [] @pytest.fixture -def semantic_models() -> list: +def semantic_models() -> List[SemanticModel]: return [] @pytest.fixture -def files() -> dict: +def files() -> Dict[str, AnySourceFile]: return {} From 2f6654f9a13b91b1cc9cb18305428bd7e7f6fd09 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Fri, 7 Jun 2024 11:27:06 -0700 Subject: [PATCH 3/7] Ensure `args` get set from global flags for `runtime_config` fixture in unit tests The `Compiler.compile` method accesses `self.config.args.which`. The `config` is the `RuntimeConfig` the `Compiler` was instantiated with. Our `runtime_config` fixture was being instatiated with an empty dict for the `args` property. Thus a `which` property of the args wasn't being made avaiable, and if `compile` was run a runtime error would occur. To solve this, we've begun instantiating the args from the global flags via `get_flags()`. This works because we ensure the `set_test_flags` fixture is run first which calls `set_from_args`. --- tests/unit/utils/config.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/unit/utils/config.py b/tests/unit/utils/config.py index 72cb4fa024c..3041bbada7c 100644 --- a/tests/unit/utils/config.py +++ b/tests/unit/utils/config.py @@ -5,6 +5,7 @@ from dbt.config.project import Project from dbt.config.renderer import ProfileRenderer from dbt.config.runtime import RuntimeConfig +from dbt.flags import get_flags @pytest.fixture @@ -42,9 +43,10 @@ def profile() -> Profile: @pytest.fixture -def runtime_config(project: Project, profile: Profile) -> RuntimeConfig: +def runtime_config(project: Project, profile: Profile, set_test_flags) -> RuntimeConfig: + args = get_flags() return RuntimeConfig.from_parts( project=project, profile=profile, - args={}, + args=args, ) From e95188513726bd095c32c1350653924bd5d54ef4 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Fri, 7 Jun 2024 13:11:13 -0700 Subject: [PATCH 4/7] Create a `make_manifest` utility function for use in unit tests and fixture creation --- tests/unit/utils/manifest.py | 59 ++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/tests/unit/utils/manifest.py b/tests/unit/utils/manifest.py index d7902cfc682..98c00007cd5 100644 --- a/tests/unit/utils/manifest.py +++ b/tests/unit/utils/manifest.py @@ -1,4 +1,4 @@ -from typing import Dict, List +from typing import Any, Dict, List import pytest from dbt_semantic_interfaces.type_enums import MetricType @@ -21,8 +21,10 @@ from dbt.contracts.graph.nodes import ( AccessType, DependsOn, + Documentation, Exposure, GenericTestNode, + GraphMemberNode, Group, Macro, ManifestNode, @@ -985,6 +987,39 @@ def files() -> Dict[str, AnySourceFile]: return {} +def make_manifest( + disabled: Dict[str, List[GraphMemberNode]] = {}, + docs: List[Documentation] = [], + exposures: List[Exposure] = [], + files: Dict[str, AnySourceFile] = {}, + groups: List[Group] = [], + macros: List[Macro] = [], + metrics: List[Metric] = [], + nodes: List[ModelNode] = [], + selectors: Dict[str, Any] = {}, + semantic_models: List[SemanticModel] = [], + sources: List[SourceDefinition] = [], + unit_tests: List[UnitTestDefinition] = [], +) -> Manifest: + manifest = Manifest( + nodes={n.unique_id: n for n in nodes}, + sources={s.unique_id: s for s in sources}, + macros={m.unique_id: m for m in macros}, + unit_tests={t.unique_id: t for t in unit_tests}, + semantic_models={s.unique_id: s for s in semantic_models}, + docs={d.unique_id: d for d in docs}, + files=files, + exposures={e.unique_id: e for e in exposures}, + metrics={m.unique_id: m for m in metrics}, + disabled=disabled, + selectors=selectors, + groups={g.unique_id: g for g in groups}, + metadata=ManifestMetadata(adapter_type="postgres", project_name="pkg"), + ) + manifest.build_parent_and_child_maps() + return manifest + + @pytest.fixture def manifest( metric, @@ -997,20 +1032,12 @@ def manifest( semantic_models, files, ) -> Manifest: - manifest = Manifest( - nodes={n.unique_id: n for n in nodes}, - sources={s.unique_id: s for s in sources}, - macros={m.unique_id: m for m in macros}, - unit_tests={t.unique_id: t for t in unit_tests}, - semantic_models={s.unique_id: s for s in semantic_models}, - docs={}, + return make_manifest( + nodes=nodes, + sources=sources, + macros=macros, + unit_tests=unit_tests, + semantic_models=semantic_models, files=files, - exposures={}, - metrics={m.unique_id: m for m in metrics}, - disabled={}, - selectors={}, - groups={}, - metadata=ManifestMetadata(adapter_type="postgres", project_name="pkg"), + metrics=metrics, ) - manifest.build_parent_and_child_maps() - return manifest From c17ea3fa02293ea06d4d9a43c25af9e9d1ce7ed0 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Fri, 7 Jun 2024 13:12:58 -0700 Subject: [PATCH 5/7] Refactor `Compiler` and `NodeSelector` tests in `test_selector.py` to use pytesting methodology --- tests/unit/graph/test_selector.py | 194 ++++++++++++++---------------- 1 file changed, 88 insertions(+), 106 deletions(-) diff --git a/tests/unit/graph/test_selector.py b/tests/unit/graph/test_selector.py index 229ac7983a8..82f607266df 100644 --- a/tests/unit/graph/test_selector.py +++ b/tests/unit/graph/test_selector.py @@ -21,6 +21,7 @@ from dbt.adapters.factory import register_adapter, reset_adapters from dbt.adapters.postgres import Plugin as PostgresPlugin from dbt.cli.flags import convert_config +from dbt.config.runtime import RuntimeConfig from dbt.contracts.files import FileHash, FilePath, SourceFile from dbt.contracts.graph.manifest import MacroManifest, ManifestStateCheck from dbt.contracts.project import ProjectFlags @@ -34,6 +35,7 @@ generate_name_macros, inject_plugin, ) +from tests.unit.utils.manifest import make_manifest, make_model set_from_args(Namespace(WARN_ERROR=False), None) @@ -231,6 +233,87 @@ def test_invalid_specs(invalid): graph_selector.SelectionCriteria.from_single_spec(invalid) +class TestCompiler: + def test_single_model(self, runtime_config: RuntimeConfig): + model = make_model(pkg="pkg", name="model_one", code="SELECT * FROM events") + manifest = make_manifest(nodes=[model]) + + compiler = dbt.compilation.Compiler(config=runtime_config) + linker = compiler.compile(manifest) + + assert linker.nodes() == {model.unique_id} + assert linker.edges() == set() + + def test_two_models_simple_ref(self, runtime_config: RuntimeConfig): + model_one = make_model(pkg="pkg", name="model_one", code="SELECT * FROM events") + model_two = make_model( + pkg="pkg", + name="model_two", + code="SELECT * FROM {{ref('model_one')}}", + refs=[model_one], + ) + models = [model_one, model_two] + manifest = make_manifest(nodes=models) + + compiler = dbt.compilation.Compiler(config=runtime_config) + linker = compiler.compile(manifest) + + expected_nodes = [model.unique_id for model in models] + assert linker.nodes() == set(expected_nodes) + assert list(linker.edges()) == [tuple(expected_nodes)] + + +class TestNodeSelector: + def test_dependency_list(self, runtime_config: RuntimeConfig): + model_one = make_model(pkg="pkg", name="model_one", code="SELECT * FROM events") + model_two = make_model( + pkg="pkg", + name="model_two", + code="SELECT * FROM {{ref('model_one')}}", + refs=[model_one], + ) + model_three = make_model( + pkg="pkg", + name="model_three", + code=""" + SELECT * FROM {{ ref("model_1") }} + union all + SELECT * FROM {{ ref("model_2") }} + """, + refs=[model_one, model_two], + ) + model_four = make_model( + pkg="pkg", + name="model_four", + code="SELECT * FROM {{ref('model_three')}}", + refs=[model_three], + ) + models = [model_one, model_two, model_three, model_four] + manifest = make_manifest(nodes=models) + + # Get the graph + compiler = dbt.compilation.Compiler(runtime_config) + graph = compiler.compile(manifest) + + # Create the selector and get the queue + selector = NodeSelector(graph, manifest) + queue = selector.get_graph_queue( + parse_difference( + None, + None, + ) + ) + + for model in models: + assert not queue.empty() + got = queue.get(block=False) + assert got.unique_id == model.unique_id + with pytest.raises(Empty): + queue.get(block=False) + queue.mark_done(got.unique_id) + assert queue.empty() + + class GraphTest(unittest.TestCase): def tearDown(self): self.mock_filesystem_search.stop() @@ -365,54 +448,7 @@ def load_manifest(self, config): loader.load() return loader.manifest - def test__single_model(self): - self.use_models( - { - "model_one": "select * from events", - } - ) - - config = self.get_config() - manifest = self.load_manifest(config) - - compiler = self.get_compiler(config) - linker = compiler.compile(manifest) - - self.assertEqual(list(linker.nodes()), ["model.test_models_compile.model_one"]) - - self.assertEqual(list(linker.edges()), []) - - def test__two_models_simple_ref(self): - self.use_models( - { - "model_one": "select * from events", - "model_two": "select * from {{ref('model_one')}}", - } - ) - - config = self.get_config() - manifest = self.load_manifest(config) - compiler = self.get_compiler(config) - linker = compiler.compile(manifest) - - self.assertCountEqual( - linker.nodes(), - [ - "model.test_models_compile.model_one", - "model.test_models_compile.model_two", - ], - ) - - self.assertCountEqual( - linker.edges(), - [ - ( - "model.test_models_compile.model_one", - "model.test_models_compile.model_two", - ) - ], - ) - + # TODO move / delete: This is testing ref parsing, not the selector nor compiler def test__two_models_package_ref(self): self.use_models( { @@ -444,6 +480,7 @@ def test__two_models_package_ref(self): ], ) + # TODO move / delete: This is testing materialization configuration, not the selector nor compiler def test__model_materializations(self): self.use_models( { @@ -480,6 +517,8 @@ def test__model_materializations(self): actual = manifest.nodes[key].config.materialized self.assertEqual(actual, expected) + # TODO move / delete: This is testing materialization configuration, not the selector nor compiler + # The "compiler" testing that is going on here is no different from test_single_model def test__model_incremental(self): self.use_models({"model_one": "select * from events"}) @@ -503,64 +542,7 @@ def test__model_incremental(self): self.assertEqual(manifest.nodes[node].config.materialized, "incremental") - def test__dependency_list(self): - self.use_models( - { - "model_1": "select * from events", - "model_2": 'select * from {{ ref("model_1") }}', - "model_3": """ - select * from {{ ref("model_1") }} - union all - select * from {{ ref("model_2") }} - """, - "model_4": 'select * from {{ ref("model_3") }}', - } - ) - - config = self.get_config() - manifest = self.load_manifest(config) - compiler = self.get_compiler(config) - graph = compiler.compile(manifest) - - models = ("model_1", "model_2", "model_3", "model_4") - model_ids = ["model.test_models_compile.{}".format(m) for m in models] - - manifest = MagicMock( - nodes={ - n: MagicMock( - unique_id=n, - name=n.split(".")[-1], - package_name="test_models_compile", - fqn=["test_models_compile", n], - empty=False, - config=MagicMock(enabled=True), - ) - for n in model_ids - } - ) - manifest.expect.side_effect = lambda n: MagicMock(unique_id=n) - selector = NodeSelector(graph, manifest) - # TODO: The "eager" string below needs to be replaced with programatic access - # to the default value for the indirect selection parameter in - # dbt.cli.params.indirect_selection - # - # Doing that is actually a little tricky, so I'm punting it to a new ticket GH #6397 - queue = selector.get_graph_queue( - parse_difference( - None, - None, - ) - ) - - for model_id in model_ids: - self.assertFalse(queue.empty()) - got = queue.get(block=False) - self.assertEqual(got.unique_id, model_id) - with self.assertRaises(Empty): - queue.get(block=False) - queue.mark_done(got.unique_id) - self.assertTrue(queue.empty()) - + # TODO move / delete: This is testing partial parsing, not the selector nor compiler def test__partial_parse(self): config = self.get_config() From 03fb727c4f40588a6df075871154bde423103d3e Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Fri, 7 Jun 2024 15:53:23 -0700 Subject: [PATCH 6/7] Remove parsing tests that exist in `test_selector.py` We had some tests in `test_selector.py::GraphTest` that didn't add anything ontop of what was already being tested else where in the file except the parsing of models. However, the tests in `test_parser.py::ModelParserTest` cover everything being tested here (and then some). Thus these tests in `test_selector.py::GraphTest` are unnecessary and can be deleted. --- tests/unit/graph/test_selector.py | 94 ------------------------------- 1 file changed, 94 deletions(-) diff --git a/tests/unit/graph/test_selector.py b/tests/unit/graph/test_selector.py index 82f607266df..db5f87a6345 100644 --- a/tests/unit/graph/test_selector.py +++ b/tests/unit/graph/test_selector.py @@ -448,100 +448,6 @@ def load_manifest(self, config): loader.load() return loader.manifest - # TODO move / delete: This is testing ref parsing, not the selector nor compiler - def test__two_models_package_ref(self): - self.use_models( - { - "model_one": "select * from events", - "model_two": "select * from {{ref('test_models_compile', 'model_one')}}", - } - ) - - config = self.get_config() - manifest = self.load_manifest(config) - compiler = self.get_compiler(config) - linker = compiler.compile(manifest) - - self.assertCountEqual( - linker.nodes(), - [ - "model.test_models_compile.model_one", - "model.test_models_compile.model_two", - ], - ) - - self.assertCountEqual( - linker.edges(), - [ - ( - "model.test_models_compile.model_one", - "model.test_models_compile.model_two", - ) - ], - ) - - # TODO move / delete: This is testing materialization configuration, not the selector nor compiler - def test__model_materializations(self): - self.use_models( - { - "model_one": "select * from events", - "model_two": "select * from {{ref('model_one')}}", - "model_three": "select * from events", - "model_four": "select * from events", - } - ) - - cfg = { - "models": { - "materialized": "table", - "test_models_compile": { - "model_one": {"materialized": "table"}, - "model_two": {"materialized": "view"}, - "model_three": {"materialized": "ephemeral"}, - }, - } - } - - config = self.get_config(cfg) - manifest = self.load_manifest(config) - - expected_materialization = { - "model_one": "table", - "model_two": "view", - "model_three": "ephemeral", - "model_four": "table", - } - - for model, expected in expected_materialization.items(): - key = "model.test_models_compile.{}".format(model) - actual = manifest.nodes[key].config.materialized - self.assertEqual(actual, expected) - - # TODO move / delete: This is testing materialization configuration, not the selector nor compiler - # The "compiler" testing that is going on here is no different from test_single_model - def test__model_incremental(self): - self.use_models({"model_one": "select * from events"}) - - cfg = { - "models": { - "test_models_compile": { - "model_one": {"materialized": "incremental", "unique_key": "id"}, - } - } - } - - config = self.get_config(cfg) - manifest = self.load_manifest(config) - compiler = self.get_compiler(config) - linker = compiler.compile(manifest) - - node = "model.test_models_compile.model_one" - - self.assertEqual(list(linker.nodes()), [node]) - self.assertEqual(list(linker.edges()), []) - - self.assertEqual(manifest.nodes[node].config.materialized, "incremental") - # TODO move / delete: This is testing partial parsing, not the selector nor compiler def test__partial_parse(self): config = self.get_config() From 9237332b4b666664fb6ddbc3a690d1947409498d Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Fri, 7 Jun 2024 16:26:25 -0700 Subject: [PATCH 7/7] Move `test__partial_parse` from `test_selector.py` to `test_manifest.py` There was a test `test__partial_parse` in `test_selector.py` which tested the functionality of `is_partial_parsable` of the `ManifestLoader`. This doesn't really make sense to exist in `test_selector.py` where we are testing selectors. We test the `ManifestLoader` class in `test_manifest.py` which seemed like a more appropriate place for the test. Additionally we renamed the test to `test_is_partial_parsable_by_version` to more accurately describe what is being tested. --- tests/unit/graph/test_selector.py | 174 +---------------------------- tests/unit/parser/test_manifest.py | 36 +++++- 2 files changed, 37 insertions(+), 173 deletions(-) diff --git a/tests/unit/graph/test_selector.py b/tests/unit/graph/test_selector.py index db5f87a6345..acbb69dfbba 100644 --- a/tests/unit/graph/test_selector.py +++ b/tests/unit/graph/test_selector.py @@ -1,9 +1,7 @@ -import os import string -import unittest from argparse import Namespace from queue import Empty -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock import networkx as nx import pytest @@ -17,24 +15,10 @@ import dbt.parser.manifest import dbt.utils import dbt_common.exceptions -from dbt import tracking -from dbt.adapters.factory import register_adapter, reset_adapters -from dbt.adapters.postgres import Plugin as PostgresPlugin -from dbt.cli.flags import convert_config from dbt.config.runtime import RuntimeConfig -from dbt.contracts.files import FileHash, FilePath, SourceFile -from dbt.contracts.graph.manifest import MacroManifest, ManifestStateCheck -from dbt.contracts.project import ProjectFlags -from dbt.events.logging import setup_event_logger -from dbt.flags import get_flags, set_from_args +from dbt.flags import set_from_args from dbt.graph import NodeSelector, parse_difference -from dbt.mp_context import get_mp_context from dbt.node_types import NodeType -from tests.unit.utils import ( - config_from_parts_or_dicts, - generate_name_macros, - inject_plugin, -) from tests.unit.utils.manifest import make_manifest, make_model set_from_args(Namespace(WARN_ERROR=False), None) @@ -312,157 +296,3 @@ def test_dependency_list(self, runtime_config: RuntimeConfig): queue.get(block=False) queue.mark_done(got.unique_id) assert queue.empty() - - -class GraphTest(unittest.TestCase): - def tearDown(self): - self.mock_filesystem_search.stop() - self.load_state_check.stop() - self.load_source_file_patcher.stop() - reset_adapters() - - def setUp(self): - # create various attributes - self.graph_result = None - tracking.do_not_track() - self.profile = { - "outputs": { - "test": { - "type": "postgres", - "threads": 4, - "host": "thishostshouldnotexist", - "port": 5432, - "user": "root", - "pass": "password", - "dbname": "dbt", - "schema": "dbt_test", - } - }, - "target": "test", - } - self.macro_manifest = MacroManifest( - {n.unique_id: n for n in generate_name_macros("test_models_compile")} - ) - self.mock_models = [] # used by filesystem_searcher - - # Create file filesystem searcher - self.filesystem_search = patch("dbt.parser.read_files.filesystem_search") - - def mock_filesystem_search(project, relative_dirs, extension, ignore_spec): - if "sql" not in extension: - return [] - if "models" not in relative_dirs: - return [] - return [model.path for model in self.mock_models] - - self.mock_filesystem_search = self.filesystem_search.start() - self.mock_filesystem_search.side_effect = mock_filesystem_search - - # Create the Manifest.state_check patcher - @patch("dbt.parser.manifest.ManifestLoader.build_manifest_state_check") - def _mock_state_check(self): - all_projects = self.all_projects - return ManifestStateCheck( - project_env_vars_hash=FileHash.from_contents(""), - profile_env_vars_hash=FileHash.from_contents(""), - vars_hash=FileHash.from_contents("vars"), - project_hashes={name: FileHash.from_contents(name) for name in all_projects}, - profile_hash=FileHash.from_contents("profile"), - ) - - self.load_state_check = patch( - "dbt.parser.manifest.ManifestLoader.build_manifest_state_check" - ) - self.mock_state_check = self.load_state_check.start() - self.mock_state_check.side_effect = _mock_state_check - - # Create the source file patcher - self.load_source_file_patcher = patch("dbt.parser.read_files.load_source_file") - self.mock_source_file = self.load_source_file_patcher.start() - - def mock_load_source_file(path, parse_file_type, project_name, saved_files): - for sf in self.mock_models: - if sf.path == path: - source_file = sf - source_file.project_name = project_name - source_file.parse_file_type = parse_file_type - return source_file - - self.mock_source_file.side_effect = mock_load_source_file - - # Create hookparser source file patcher - self.load_source_file_manifest_patcher = patch("dbt.parser.manifest.load_source_file") - self.mock_source_file_manifest = self.load_source_file_manifest_patcher.start() - - def mock_load_source_file_manifest(path, parse_file_type, project_name, saved_files): - return [] - - self.mock_source_file_manifest.side_effect = mock_load_source_file_manifest - - def get_config(self, extra_cfg=None): - if extra_cfg is None: - extra_cfg = {} - - cfg = { - "name": "test_models_compile", - "version": "0.1", - "profile": "test", - "project-root": os.path.abspath("."), - "config-version": 2, - } - cfg.update(extra_cfg) - - config = config_from_parts_or_dicts(project=cfg, profile=self.profile) - set_from_args(Namespace(), ProjectFlags()) - flags = get_flags() - setup_event_logger(flags) - object.__setattr__(flags, "PARTIAL_PARSE", False) - for arg_name, args_param_value in vars(flags).items(): - args_param_value = convert_config(arg_name, args_param_value) - object.__setattr__(config.args, arg_name.upper(), args_param_value) - object.__setattr__(config.args, arg_name.lower(), args_param_value) - - return config - - def get_compiler(self, project): - return dbt.compilation.Compiler(project) - - def use_models(self, models): - for k, v in models.items(): - path = FilePath( - searched_path="models", - project_root=os.path.normcase(os.getcwd()), - relative_path="{}.sql".format(k), - modification_time=0.0, - ) - # FileHash can't be empty or 'search_key' will be None - source_file = SourceFile(path=path, checksum=FileHash.from_contents("abc")) - source_file.contents = v - self.mock_models.append(source_file) - - def load_manifest(self, config): - inject_plugin(PostgresPlugin) - register_adapter(config, get_mp_context()) - loader = dbt.parser.manifest.ManifestLoader(config, {config.project_name: config}) - loader.manifest.macros = self.macro_manifest.macros - loader.load() - return loader.manifest - - # TODO move / delete: This is testing partial parsing, not the selector nor compiler - def test__partial_parse(self): - config = self.get_config() - - manifest = self.load_manifest(config) - - # we need a loader to compare the two manifests - loader = dbt.parser.manifest.ManifestLoader(config, {config.project_name: config}) - loader.manifest = manifest.deepcopy() - - is_partial_parsable, _ = loader.is_partial_parsable(manifest) - self.assertTrue(is_partial_parsable) - manifest.metadata.dbt_version = "0.0.1a1" - is_partial_parsable, _ = loader.is_partial_parsable(manifest) - self.assertFalse(is_partial_parsable) - manifest.metadata.dbt_version = "99999.99.99" - is_partial_parsable, _ = loader.is_partial_parsable(manifest) - self.assertFalse(is_partial_parsable) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index 1f10ee04f25..4c887bce5c0 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -4,8 +4,9 @@ import pytest from pytest_mock import MockerFixture +from dbt.artifacts.resources.base import FileHash from dbt.config import RuntimeConfig -from dbt.contracts.graph.manifest import Manifest +from dbt.contracts.graph.manifest import Manifest, ManifestStateCheck from dbt.flags import set_from_args from dbt.parser.manifest import ManifestLoader from dbt.parser.read_files import FileDiff @@ -38,6 +39,39 @@ def test_profile_hash_change(self, mock_project): manifest = ManifestLoader(mock_project, {}) assert manifest.manifest.state_check.profile_hash.checksum != profile_hash + @patch("dbt.parser.manifest.ManifestLoader.build_manifest_state_check") + @patch("dbt.parser.manifest.os.path.exists") + @patch("dbt.parser.manifest.open") + def test_partial_parse_by_version( + self, + patched_open, + patched_os_exist, + patched_state_check, + runtime_config: RuntimeConfig, + manifest: Manifest, + ): + file_hash = FileHash.from_contents("test contests") + manifest.state_check = ManifestStateCheck( + vars_hash=file_hash, + profile_hash=file_hash, + profile_env_vars_hash=file_hash, + project_env_vars_hash=file_hash, + ) + # we need a loader to compare the two manifests + loader = ManifestLoader(runtime_config, {runtime_config.project_name: runtime_config}) + loader.manifest = manifest.deepcopy() + + is_partial_parsable, _ = loader.is_partial_parsable(manifest) + assert is_partial_parsable + + manifest.metadata.dbt_version = "0.0.1a1" + is_partial_parsable, _ = loader.is_partial_parsable(manifest) + assert not is_partial_parsable + + manifest.metadata.dbt_version = "99999.99.99" + is_partial_parsable, _ = loader.is_partial_parsable(manifest) + assert not is_partial_parsable + class TestFailedPartialParse: @patch("dbt.tracking.track_partial_parser")