From 15f27b6896d6ad5b938c7a791a994502d1d3ce4a Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 8 May 2024 16:12:44 -0700 Subject: [PATCH 1/6] Add test for different `write_perf_info` values to `get_full_manifest` --- tests/unit/parser/test_manifest.py | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index 9a84414a99e..a5c02d47947 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -2,11 +2,15 @@ from unittest.mock import MagicMock, patch import pytest +from pytest_mock import MockerFixture +from dbt.adapters.postgres import PostgresAdapter +from dbt.adapters.sql import SQLConnectionManager from dbt.config import RuntimeConfig from dbt.contracts.graph.manifest import Manifest from dbt.flags import set_from_args from dbt.parser.manifest import ManifestLoader +from dbt.tracking import User @pytest.fixture @@ -23,6 +27,20 @@ def mock_project(): return mock_project +@pytest.fixture +def mock_connection_manager(): + mock_connection_manager = MagicMock(SQLConnectionManager) + mock_connection_manager.set_query_header = lambda query_header_context: None + return mock_connection_manager + + +@pytest.fixture +def mock_adapter(mock_connection_manager: MagicMock): + mock_adapter = MagicMock(PostgresAdapter) + mock_adapter.connections = mock_connection_manager + return mock_adapter + + class TestPartialParse: @patch("dbt.parser.manifest.ManifestLoader.build_manifest_state_check") @patch("dbt.parser.manifest.os.path.exists") @@ -91,3 +109,33 @@ 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 TestGetFullManifest: + def test_write_perf_info( + self, + manifest: Manifest, + mock_project: MagicMock, + mock_adapter: MagicMock, + mocker: MockerFixture, + ) -> None: + mocker.patch("dbt.parser.manifest.get_adapter").return_value = mock_adapter + mocker.patch("dbt.parser.manifest.ManifestLoader.load").return_value = manifest + mocker.patch("dbt.parser.manifest._check_manifest").return_value = None + mocker.patch( + "dbt.parser.manifest.ManifestLoader.save_macros_to_adapter" + ).return_value = None + mocker.patch("dbt.tracking.active_user").return_value = User(None) + write_perf_info = mocker.patch("dbt.parser.manifest.ManifestLoader.write_perf_info") + + ManifestLoader.get_full_manifest( + config=mock_project, + # write_perf_info=False let it default instead + ) + assert not write_perf_info.called + + ManifestLoader.get_full_manifest(config=mock_project, write_perf_info=False) + assert not write_perf_info.called + + ManifestLoader.get_full_manifest(config=mock_project, write_perf_info=True) + assert write_perf_info.called From a54d73490fba9e4966d4c85b461a80dd2c939825 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 9 May 2024 10:21:17 -0700 Subject: [PATCH 2/6] Add test for different `reset` values to `get_full_manifest` --- tests/unit/parser/test_manifest.py | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index a5c02d47947..66f400fb9a1 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -24,6 +24,7 @@ def mock_project(): mock_project.profile_env_vars = {} mock_project.project_target_path = "mock_target_path" mock_project.credentials = MagicMock() + mock_project.clear_dependencies = MagicMock() return mock_project @@ -38,6 +39,7 @@ def mock_connection_manager(): def mock_adapter(mock_connection_manager: MagicMock): mock_adapter = MagicMock(PostgresAdapter) mock_adapter.connections = mock_connection_manager + mock_adapter.clear_macro_resolver = MagicMock() return mock_adapter @@ -139,3 +141,33 @@ def test_write_perf_info( ManifestLoader.get_full_manifest(config=mock_project, write_perf_info=True) assert write_perf_info.called + + def test_reset( + self, + manifest: Manifest, + mock_project: MagicMock, + mock_adapter: MagicMock, + mocker: MockerFixture, + ) -> None: + mocker.patch("dbt.parser.manifest.get_adapter").return_value = mock_adapter + mocker.patch("dbt.parser.manifest.ManifestLoader.load").return_value = manifest + mocker.patch("dbt.parser.manifest._check_manifest").return_value = None + mocker.patch( + "dbt.parser.manifest.ManifestLoader.save_macros_to_adapter" + ).return_value = None + mocker.patch("dbt.tracking.active_user").return_value = User(None) + + ManifestLoader.get_full_manifest( + config=mock_project, + # reset=False let it default instead + ) + assert not mock_project.clear_dependencies.called + assert not mock_adapter.clear_macro_resolver.called + + ManifestLoader.get_full_manifest(config=mock_project, reset=False) + assert not mock_project.clear_dependencies.called + assert not mock_adapter.clear_macro_resolver.called + + ManifestLoader.get_full_manifest(config=mock_project, reset=True) + assert mock_project.clear_dependencies.called + assert mock_adapter.clear_macro_resolver.called From 791ec1d64f569a59d817a9d5e0fb4b95a4b291cf Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 9 May 2024 10:42:40 -0700 Subject: [PATCH 3/6] Abstract required mocks for `get_full_manifest` tests to reduce duplication There are a set of required mocks that `get_full_manifest` unit tests need. Instead of doing these mocks in each test, we've abstracted these mocks into a reusable function. I did try to do this as a fixture, but for some reaosn the mocks didn't actually propagate when I did that. --- tests/unit/parser/test_manifest.py | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index 66f400fb9a1..f5b4c411b4e 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -114,13 +114,9 @@ def test_partial_parse_safe_update_project_parser_files_partially( class TestGetFullManifest: - def test_write_perf_info( - self, - manifest: Manifest, - mock_project: MagicMock, - mock_adapter: MagicMock, - mocker: MockerFixture, - ) -> None: + def set_required_mocks( + self, mocker: MockerFixture, manifest: Manifest, mock_adapter: MagicMock + ): mocker.patch("dbt.parser.manifest.get_adapter").return_value = mock_adapter mocker.patch("dbt.parser.manifest.ManifestLoader.load").return_value = manifest mocker.patch("dbt.parser.manifest._check_manifest").return_value = None @@ -128,6 +124,15 @@ def test_write_perf_info( "dbt.parser.manifest.ManifestLoader.save_macros_to_adapter" ).return_value = None mocker.patch("dbt.tracking.active_user").return_value = User(None) + + def test_write_perf_info( + self, + manifest: Manifest, + mock_project: MagicMock, + mock_adapter: MagicMock, + mocker: MockerFixture, + ) -> None: + self.set_required_mocks(mocker, manifest, mock_adapter) write_perf_info = mocker.patch("dbt.parser.manifest.ManifestLoader.write_perf_info") ManifestLoader.get_full_manifest( @@ -149,13 +154,7 @@ def test_reset( mock_adapter: MagicMock, mocker: MockerFixture, ) -> None: - mocker.patch("dbt.parser.manifest.get_adapter").return_value = mock_adapter - mocker.patch("dbt.parser.manifest.ManifestLoader.load").return_value = manifest - mocker.patch("dbt.parser.manifest._check_manifest").return_value = None - mocker.patch( - "dbt.parser.manifest.ManifestLoader.save_macros_to_adapter" - ).return_value = None - mocker.patch("dbt.tracking.active_user").return_value = User(None) + self.set_required_mocks(mocker, manifest, mock_adapter) ManifestLoader.get_full_manifest( config=mock_project, From 8dc4228fb926c86441cd24d2abd3a6ddb3f24bdc Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 9 May 2024 17:08:21 -0700 Subject: [PATCH 4/6] Add test for different `PARTIAL_PARSE_FILE_DIFF` values to `get_full_manifest` --- tests/unit/parser/test_manifest.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index f5b4c411b4e..72a2db5b6b6 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -10,6 +10,7 @@ from dbt.contracts.graph.manifest import Manifest from dbt.flags import set_from_args from dbt.parser.manifest import ManifestLoader +from dbt.parser.read_files import FileDiff from dbt.tracking import User @@ -170,3 +171,30 @@ def test_reset( ManifestLoader.get_full_manifest(config=mock_project, reset=True) assert mock_project.clear_dependencies.called assert mock_adapter.clear_macro_resolver.called + + def test_partial_parse_file_diff_flag( + self, + manifest: Manifest, + mock_project: MagicMock, + mock_adapter: MagicMock, + mocker: MockerFixture, + ) -> None: + self.set_required_mocks(mocker, manifest, mock_adapter) + + # FileDiff.from_dict is only called if PARTIAL_PARSE_FILE_DIFF == False + # So we can track this function call to check if setting PARTIAL_PARSE_FILE_DIFF + # works appropriately + mock_file_diff = mocker.patch("dbt.parser.read_files.FileDiff.from_dict") + mock_file_diff.return_value = FileDiff([], [], []) + + set_from_args(Namespace(), {}) + ManifestLoader.get_full_manifest(config=mock_project) + assert not mock_file_diff.called + + set_from_args(Namespace(PARTIAL_PARSE_FILE_DIFF=True), {}) + ManifestLoader.get_full_manifest(config=mock_project) + assert not mock_file_diff.called + + set_from_args(Namespace(PARTIAL_PARSE_FILE_DIFF=False), {}) + ManifestLoader.get_full_manifest(config=mock_project) + assert mock_file_diff.called From 184fe9a750ec449ae2c94860ad394fa31c175d58 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 14 May 2024 16:56:49 -0700 Subject: [PATCH 5/6] Refactor mock fixtures in `test_manifest.py` to make them more widely available --- tests/unit/conftest.py | 1 + tests/unit/parser/test_manifest.py | 33 ------------------------------ tests/unit/utils/adapter.py | 21 +++++++++++++++++++ tests/unit/utils/project.py | 18 ++++++++++++++++ 4 files changed, 40 insertions(+), 33 deletions(-) create mode 100644 tests/unit/utils/adapter.py diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index a3c42f9f8e8..b06dd39981e 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -5,6 +5,7 @@ from dbt.contracts.graph.nodes import SourceDefinition # All manifest related fixtures. +from tests.unit.utils.adapter import * # noqa from tests.unit.utils.manifest import * # noqa from tests.unit.utils.project import * # noqa diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index 72a2db5b6b6..7da3f3aeece 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -1,11 +1,8 @@ from argparse import Namespace from unittest.mock import MagicMock, patch -import pytest from pytest_mock import MockerFixture -from dbt.adapters.postgres import PostgresAdapter -from dbt.adapters.sql import SQLConnectionManager from dbt.config import RuntimeConfig from dbt.contracts.graph.manifest import Manifest from dbt.flags import set_from_args @@ -14,36 +11,6 @@ from dbt.tracking import User -@pytest.fixture -def mock_project(): - mock_project = MagicMock(RuntimeConfig) - mock_project.cli_vars = {} - mock_project.args = MagicMock() - mock_project.args.profile = "test" - mock_project.args.target = "test" - mock_project.project_env_vars = {} - mock_project.profile_env_vars = {} - mock_project.project_target_path = "mock_target_path" - mock_project.credentials = MagicMock() - mock_project.clear_dependencies = MagicMock() - return mock_project - - -@pytest.fixture -def mock_connection_manager(): - mock_connection_manager = MagicMock(SQLConnectionManager) - mock_connection_manager.set_query_header = lambda query_header_context: None - return mock_connection_manager - - -@pytest.fixture -def mock_adapter(mock_connection_manager: MagicMock): - mock_adapter = MagicMock(PostgresAdapter) - mock_adapter.connections = mock_connection_manager - mock_adapter.clear_macro_resolver = MagicMock() - return mock_adapter - - class TestPartialParse: @patch("dbt.parser.manifest.ManifestLoader.build_manifest_state_check") @patch("dbt.parser.manifest.os.path.exists") diff --git a/tests/unit/utils/adapter.py b/tests/unit/utils/adapter.py new file mode 100644 index 00000000000..06555b0e400 --- /dev/null +++ b/tests/unit/utils/adapter.py @@ -0,0 +1,21 @@ +from unittest.mock import MagicMock + +import pytest + +from dbt.adapters.postgres import PostgresAdapter +from dbt.adapters.sql import SQLConnectionManager + + +@pytest.fixture +def mock_connection_manager() -> MagicMock: + mock_connection_manager = MagicMock(SQLConnectionManager) + mock_connection_manager.set_query_header = lambda query_header_context: None + return mock_connection_manager + + +@pytest.fixture +def mock_adapter(mock_connection_manager: MagicMock) -> MagicMock: + mock_adapter = MagicMock(PostgresAdapter) + mock_adapter.connections = mock_connection_manager + mock_adapter.clear_macro_resolver = MagicMock() + return mock_adapter diff --git a/tests/unit/utils/project.py b/tests/unit/utils/project.py index b34e03da494..c7215990e6d 100644 --- a/tests/unit/utils/project.py +++ b/tests/unit/utils/project.py @@ -1,6 +1,9 @@ +from unittest.mock import MagicMock + import pytest from dbt.adapters.contracts.connection import QueryComment +from dbt.config import RuntimeConfig from dbt.config.project import Project, RenderComponents, VarProvider from dbt.config.selectors import SelectorConfig from dbt.contracts.project import PackageConfig @@ -68,3 +71,18 @@ def project(selector_config: SelectorConfig) -> Project: restrict_access=False, dbt_cloud={}, ) + + +@pytest.fixture +def mock_project(): + mock_project = MagicMock(RuntimeConfig) + mock_project.cli_vars = {} + mock_project.args = MagicMock() + mock_project.args.profile = "test" + mock_project.args.target = "test" + mock_project.project_env_vars = {} + mock_project.profile_env_vars = {} + mock_project.project_target_path = "mock_target_path" + mock_project.credentials = MagicMock() + mock_project.clear_dependencies = MagicMock() + return mock_project From 16edcbb8f8d973e7cea9cceea23f7e8d58c61165 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 14 May 2024 17:05:53 -0700 Subject: [PATCH 6/6] Convert `set_required_mocks` of `TestGetFullManifest` into a fixture This wasn't working before, but it does now. Not sure why. --- tests/unit/parser/test_manifest.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tests/unit/parser/test_manifest.py b/tests/unit/parser/test_manifest.py index 7da3f3aeece..b7d470a3552 100644 --- a/tests/unit/parser/test_manifest.py +++ b/tests/unit/parser/test_manifest.py @@ -1,6 +1,7 @@ from argparse import Namespace from unittest.mock import MagicMock, patch +import pytest from pytest_mock import MockerFixture from dbt.config import RuntimeConfig @@ -82,6 +83,7 @@ def test_partial_parse_safe_update_project_parser_files_partially( class TestGetFullManifest: + @pytest.fixture def set_required_mocks( self, mocker: MockerFixture, manifest: Manifest, mock_adapter: MagicMock ): @@ -95,12 +97,10 @@ def set_required_mocks( def test_write_perf_info( self, - manifest: Manifest, mock_project: MagicMock, - mock_adapter: MagicMock, mocker: MockerFixture, + set_required_mocks, ) -> None: - self.set_required_mocks(mocker, manifest, mock_adapter) write_perf_info = mocker.patch("dbt.parser.manifest.ManifestLoader.write_perf_info") ManifestLoader.get_full_manifest( @@ -117,12 +117,10 @@ def test_write_perf_info( def test_reset( self, - manifest: Manifest, mock_project: MagicMock, mock_adapter: MagicMock, - mocker: MockerFixture, + set_required_mocks, ) -> None: - self.set_required_mocks(mocker, manifest, mock_adapter) ManifestLoader.get_full_manifest( config=mock_project, @@ -141,12 +139,10 @@ def test_reset( def test_partial_parse_file_diff_flag( self, - manifest: Manifest, mock_project: MagicMock, - mock_adapter: MagicMock, mocker: MockerFixture, + set_required_mocks, ) -> None: - self.set_required_mocks(mocker, manifest, mock_adapter) # FileDiff.from_dict is only called if PARTIAL_PARSE_FILE_DIFF == False # So we can track this function call to check if setting PARTIAL_PARSE_FILE_DIFF