From 503592c0ae59963bf32951321e25aba00578fba1 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 22 May 2024 11:20:51 -0700 Subject: [PATCH 1/9] Create `runtime_config` fixture and necessary upstream fixtures --- tests/unit/conftest.py | 1 + tests/unit/utils/config.py | 50 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 tests/unit/utils/config.py diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index f1823fb858f..7c14e8dee5b 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -6,6 +6,7 @@ # All manifest related fixtures. from tests.unit.utils.adapter import * # noqa +from tests.unit.utils.config import * # noqa from tests.unit.utils.event_manager import * # noqa from tests.unit.utils.flags import * # noqa from tests.unit.utils.manifest import * # noqa diff --git a/tests/unit/utils/config.py b/tests/unit/utils/config.py new file mode 100644 index 00000000000..d34c62fa901 --- /dev/null +++ b/tests/unit/utils/config.py @@ -0,0 +1,50 @@ +import pytest + +from dbt.adapters.postgres.connections import PostgresCredentials +from dbt.config.profile import Profile +from dbt.config.project import Project +from dbt.config.renderer import ProfileRenderer +from dbt.config.runtime import RuntimeConfig + + +@pytest.fixture +def credentials() -> PostgresCredentials: + return PostgresCredentials( + database="test_database", + schema="test_schema", + host="test_host", + user="test_user", + port=1337, + password="test_password", + ) + + +@pytest.fixture +def profile() -> Profile: + profile_yaml = { + "target": "postgres", + "outputs": { + "postgres": { + "type": "postgres", + "host": "postgres-db-hostname", + "port": 5555, + "user": "db_user", + "pass": "db_pass", + "dbname": "postgres-db-name", + "schema": "postgres-schema", + "threads": 7, + }, + }, + } + return Profile.from_raw_profile_info( + raw_profile=profile_yaml, profile_name="unit_tests", renderer=ProfileRenderer({}) + ) + + +@pytest.fixture +def runtime_config(project: Project, profile: Profile) -> RuntimeConfig: + return RuntimeConfig.from_parts( + project=project, + profile=profile, + args={}, + ) From 391c99c6a7053ef6ef3fb0b6a6bd69522020883b Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 28 May 2024 15:41:35 -0700 Subject: [PATCH 2/9] Check for better scoped `ProjectContractError` in test_runtime tests Previously in `test_unsupported_version_extra_config` and `test_archive_not_allowed` we were checking for `DbtProjectError`. This worked because `ProjectContractError` is a subclass of `DbtProjectError`. However, if we check for `DbtProjectError` in these tests than, some tangential failure which raises a `DbtProejctError` type error would go undetected. As we plan on modifying these tests to be pytest in the coming commits, we want to ensure that the tests are succeeding for the right reason. --- tests/unit/config/test_runtime.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 6d2b18fd896..1652ca9513a 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -123,7 +123,7 @@ def test_impossible_version_range(self): def test_unsupported_version_extra_config(self): self.default_project_data["some-extra-field-not-allowed"] = True - raised = self.from_parts(dbt.exceptions.DbtProjectError) + raised = self.from_parts(dbt.exceptions.ProjectContractError) self.assertIn("Additional properties are not allowed", str(raised.exception)) def test_archive_not_allowed(self): @@ -141,7 +141,7 @@ def test_archive_not_allowed(self): ], } ] - with self.assertRaises(dbt.exceptions.DbtProjectError): + with self.assertRaises(dbt.exceptions.ProjectContractError): self.get_project() def test__warn_for_unused_resource_config_paths_empty(self): From 4a168315ded7e5a0d09129cfb8210324e9bfc4d1 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 28 May 2024 16:07:54 -0700 Subject: [PATCH 3/9] Convert `test_str` of `TestRuntimeConfig` to a pytest test using fixtures --- tests/unit/config/test_runtime.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 1652ca9513a..2957a66c0cd 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -5,6 +5,8 @@ import dbt.config import dbt.exceptions from dbt import tracking +from dbt.config.profile import Profile +from dbt.config.project import Project from dbt.contracts.project import PackageConfig from dbt.flags import set_from_args from dbt.tests.util import safe_set_invocation_context @@ -16,7 +18,15 @@ ) -class TestRuntimeConfig(BaseConfigTest): +class TestRuntimeConfig: + def test_str(self, profile: Profile, project: Project) -> None: + config = dbt.config.RuntimeConfig.from_parts(project, profile, {}) + + # to make sure nothing terrible happens + str(config) + + +class TestRuntimeConfigOLD(BaseConfigTest): def get_project(self): return project_from_config_norender( self.default_project_data, @@ -62,14 +72,6 @@ def test_from_parts(self): } self.assertEqual(config.to_project_config(), expected_project) - def test_str(self): - project = self.get_project() - profile = self.get_profile() - config = dbt.config.RuntimeConfig.from_parts(project, profile, {}) - - # to make sure nothing terrible happens - str(config) - def test_supported_version(self): self.default_project_data["require-dbt-version"] = ">0.0.0" conf = self.from_parts() From a93765d1320abb8eb05d4b6ddcbca2e8e29bf751 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 28 May 2024 16:21:16 -0700 Subject: [PATCH 4/9] Convert `test_from_parts` of `TestRuntimeConfig` to a pytest test using fixtures While converting `test_from_parts` I noticed the comment > TODO(jeb): Adapters must assert that quoting is populated? This led me to beleive that `quoting` shouldn't be "fully" realized in our project fixture unless we're saying that it's gone through adapter instantiation. Thus I update the `quoting` on our project fixture to be an empty dict. This change affected `test__str__` in `test_project.py` which we thus needed to update accordingly. --- tests/unit/config/test_project.py | 2 +- tests/unit/config/test_runtime.py | 53 +++++++++++++++++++------------ tests/unit/utils/config.py | 2 +- tests/unit/utils/project.py | 2 +- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/tests/unit/config/test_project.py b/tests/unit/config/test_project.py index 7d0006570af..6b9cbfb1ddb 100644 --- a/tests/unit/config/test_project.py +++ b/tests/unit/config/test_project.py @@ -42,7 +42,7 @@ def test_fixture_paths(self, project: Project): def test__str__(self, project: Project): assert ( str(project) - == "{'name': 'test_project', 'version': 1.0, 'project-root': 'doesnt/actually/exist', 'profile': 'test_profile', 'model-paths': ['models'], 'macro-paths': ['macros'], 'seed-paths': ['seeds'], 'test-paths': ['tests'], 'analysis-paths': ['analyses'], 'docs-paths': ['docs'], 'asset-paths': ['assets'], 'target-path': 'target', 'snapshot-paths': ['snapshots'], 'clean-targets': ['target'], 'log-path': 'path/to/project/logs', 'quoting': {'database': True, 'schema': True, 'identifier': True}, 'models': {}, 'on-run-start': [], 'on-run-end': [], 'dispatch': [{'macro_namespace': 'dbt_utils', 'search_order': ['test_project', 'dbt_utils']}], 'seeds': {}, 'snapshots': {}, 'sources': {}, 'data_tests': {}, 'unit_tests': {}, 'metrics': {}, 'semantic-models': {}, 'saved-queries': {}, 'exposures': {}, 'vars': {}, 'require-dbt-version': ['=0.0.0'], 'restrict-access': False, 'dbt-cloud': {}, 'query-comment': {'comment': \"\\n{%- set comment_dict = {} -%}\\n{%- do comment_dict.update(\\n app='dbt',\\n dbt_version=dbt_version,\\n profile_name=target.get('profile_name'),\\n target_name=target.get('target_name'),\\n) -%}\\n{%- if node is not none -%}\\n {%- do comment_dict.update(\\n node_id=node.unique_id,\\n ) -%}\\n{% else %}\\n {# in the node context, the connection name is the node_id #}\\n {%- do comment_dict.update(connection_name=connection_name) -%}\\n{%- endif -%}\\n{{ return(tojson(comment_dict)) }}\\n\", 'append': False, 'job-label': False}, 'packages': []}" + == "{'name': 'test_project', 'version': 1.0, 'project-root': 'doesnt/actually/exist', 'profile': 'test_profile', 'model-paths': ['models'], 'macro-paths': ['macros'], 'seed-paths': ['seeds'], 'test-paths': ['tests'], 'analysis-paths': ['analyses'], 'docs-paths': ['docs'], 'asset-paths': ['assets'], 'target-path': 'target', 'snapshot-paths': ['snapshots'], 'clean-targets': ['target'], 'log-path': 'path/to/project/logs', 'quoting': {}, 'models': {}, 'on-run-start': [], 'on-run-end': [], 'dispatch': [{'macro_namespace': 'dbt_utils', 'search_order': ['test_project', 'dbt_utils']}], 'seeds': {}, 'snapshots': {}, 'sources': {}, 'data_tests': {}, 'unit_tests': {}, 'metrics': {}, 'semantic-models': {}, 'saved-queries': {}, 'exposures': {}, 'vars': {}, 'require-dbt-version': ['=0.0.0'], 'restrict-access': False, 'dbt-cloud': {}, 'query-comment': {'comment': \"\\n{%- set comment_dict = {} -%}\\n{%- do comment_dict.update(\\n app='dbt',\\n dbt_version=dbt_version,\\n profile_name=target.get('profile_name'),\\n target_name=target.get('target_name'),\\n) -%}\\n{%- if node is not none -%}\\n {%- do comment_dict.update(\\n node_id=node.unique_id,\\n ) -%}\\n{% else %}\\n {# in the node context, the connection name is the node_id #}\\n {%- do comment_dict.update(connection_name=connection_name) -%}\\n{%- endif -%}\\n{{ return(tojson(comment_dict)) }}\\n\", 'append': False, 'job-label': False}, 'packages': []}" ) def test_get_selector(self, project: Project): diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 2957a66c0cd..54124426400 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -1,7 +1,10 @@ import os +import tempfile from argparse import Namespace from unittest import mock +import pytest + import dbt.config import dbt.exceptions from dbt import tracking @@ -19,12 +22,42 @@ class TestRuntimeConfig: + @pytest.fixture + def args(self) -> Namespace: + return Namespace( + profiles_dir=tempfile.mkdtemp(), + cli_vars={}, + version_check=True, + project_dir=tempfile.mkdtemp(), + target=None, + threads=None, + profile=None, + ) + def test_str(self, profile: Profile, project: Project) -> None: config = dbt.config.RuntimeConfig.from_parts(project, profile, {}) # to make sure nothing terrible happens str(config) + def test_from_parts(self, args: Namespace, profile: Profile, project: Project): + config = dbt.config.RuntimeConfig.from_parts(project, profile, args) + + assert config.cli_vars == {} + assert config.to_profile_info() == profile.to_profile_info() + # we should have the default quoting set in the full config, but not in + # the project + # TODO(jeb): Adapters must assert that quoting is populated? + expected_project = project.to_project_config() + assert expected_project["quoting"] == {} + + expected_project["quoting"] = { + "database": True, + "identifier": True, + "schema": True, + } + assert config.to_project_config() == expected_project + class TestRuntimeConfigOLD(BaseConfigTest): def get_project(self): @@ -52,26 +85,6 @@ def from_parts(self, exc=None): else: return err - def test_from_parts(self): - project = self.get_project() - profile = self.get_profile() - config = dbt.config.RuntimeConfig.from_parts(project, profile, self.args) - - self.assertEqual(config.cli_vars, {}) - self.assertEqual(config.to_profile_info(), profile.to_profile_info()) - # we should have the default quoting set in the full config, but not in - # the project - # TODO(jeb): Adapters must assert that quoting is populated? - expected_project = project.to_project_config() - self.assertEqual(expected_project["quoting"], {}) - - expected_project["quoting"] = { - "database": True, - "identifier": True, - "schema": True, - } - self.assertEqual(config.to_project_config(), expected_project) - def test_supported_version(self): self.default_project_data["require-dbt-version"] = ">0.0.0" conf = self.from_parts() diff --git a/tests/unit/utils/config.py b/tests/unit/utils/config.py index d34c62fa901..72cb4fa024c 100644 --- a/tests/unit/utils/config.py +++ b/tests/unit/utils/config.py @@ -37,7 +37,7 @@ def profile() -> Profile: }, } return Profile.from_raw_profile_info( - raw_profile=profile_yaml, profile_name="unit_tests", renderer=ProfileRenderer({}) + raw_profile=profile_yaml, profile_name="test_profile", renderer=ProfileRenderer({}) ) diff --git a/tests/unit/utils/project.py b/tests/unit/utils/project.py index c7215990e6d..2e374b82fac 100644 --- a/tests/unit/utils/project.py +++ b/tests/unit/utils/project.py @@ -45,7 +45,7 @@ def project(selector_config: SelectorConfig) -> Project: log_path="path/to/project/logs", packages_install_path="dbt_packages", packages_specified_path="packages.yml", - quoting={"database": True, "schema": True, "identifier": True}, + quoting={}, models={}, on_run_start=[], on_run_end=[], From aabe0ae7da57c3c4df58caaa3c96220ddd4db35e Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 28 May 2024 18:02:33 -0700 Subject: [PATCH 5/9] Convert runtime version specifier tests to pytest tests and move to test_project We've done two things in this commit, which arguably _should_ have been done in two commits. First we moved the version specifier tests from `test_runtime.py::TestRuntimeConfig` to `test_project.py::TestGetRequiredVersion` this is because what is really being tested is the `_get_required_version` method. Doing it via `RuntimeConfig.from_parts` method made actually testing it a lot harder as it requires setting up more of the world and running with a _full_ project config dict. The second thing we did was convert it from the old unittest implementation to a pytest implementation. This saves us from having to create most of the world as we were doing previously in these tests. Of note, I did not move the test `test_unsupported_version_range_bad_config`. This test is a bit different from the rest of the version specifier tests. It was introduced in [1eb585781192f706d485f4a4d3802cc17366f713](https://github.com/dbt-labs/dbt-core/commit/1eb585781192f706d485f4a4d3802cc17366f713) of [#2726](https://github.com/dbt-labs/dbt-core/pull/2726) to resolve [#2638](https://github.com/dbt-labs/dbt-core/issues/2638). The focus of #2726 was to ensure the version specifier checks were run _before_ the validation of the `dbt_project.yml`. Thus what this test is actually testing for is order of operations at parse time. As such, this is really more a _functional_ test than a unit test. In the next commit we'll get this test moved (and renamed) --- tests/unit/config/test_project.py | 53 ++++++++++++++++++++++++++++++- tests/unit/config/test_runtime.py | 45 -------------------------- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/tests/unit/config/test_project.py b/tests/unit/config/test_project.py index 6b9cbfb1ddb..ae0ae3928dc 100644 --- a/tests/unit/config/test_project.py +++ b/tests/unit/config/test_project.py @@ -2,6 +2,7 @@ import os import unittest from copy import deepcopy +from typing import Any, Dict from unittest import mock import pytest @@ -10,7 +11,7 @@ import dbt.exceptions from dbt.adapters.contracts.connection import DEFAULT_QUERY_COMMENT, QueryComment from dbt.adapters.factory import load_plugin -from dbt.config.project import Project +from dbt.config.project import Project, _get_required_version from dbt.constants import DEPENDENCIES_FILE_NAME from dbt.contracts.project import GitPackage, LocalPackage, PackageConfig from dbt.flags import set_from_args @@ -534,3 +535,53 @@ def setUp(self): def test_setting_multiple_flags(self): with pytest.raises(dbt.exceptions.DbtProjectError): set_from_args(self.args, None) + + +class TestGetRequiredVersion: + @pytest.fixture + def project_dict(self) -> Dict[str, Any]: + return { + "name": "test_project", + "require-dbt-version": ">0.0.0", + } + + def test_supported_version(self, project_dict: Dict[str, Any]) -> None: + specifiers = _get_required_version(project_dict=project_dict, verify_version=True) + assert set(x.to_version_string() for x in specifiers) == {">0.0.0"} + + def test_unsupported_version(self, project_dict: Dict[str, Any]) -> None: + project_dict["require-dbt-version"] = ">99999.0.0" + with pytest.raises( + dbt.exceptions.DbtProjectError, match="This version of dbt is not supported" + ): + _get_required_version(project_dict=project_dict, verify_version=True) + + def test_unsupported_version_no_check(self, project_dict: Dict[str, Any]) -> None: + project_dict["require-dbt-version"] = ">99999.0.0" + specifiers = _get_required_version(project_dict=project_dict, verify_version=False) + assert set(x.to_version_string() for x in specifiers) == {">99999.0.0"} + + def test_supported_version_range(self, project_dict: Dict[str, Any]) -> None: + project_dict["require-dbt-version"] = [">0.0.0", "<=99999.0.0"] + specifiers = _get_required_version(project_dict=project_dict, verify_version=True) + assert set(x.to_version_string() for x in specifiers) == {">0.0.0", "<=99999.0.0"} + + def test_unsupported_version_range(self, project_dict: Dict[str, Any]) -> None: + project_dict["require-dbt-version"] = [">0.0.0", "<=0.0.1"] + with pytest.raises( + dbt.exceptions.DbtProjectError, match="This version of dbt is not supported" + ): + _get_required_version(project_dict=project_dict, verify_version=True) + + def test_unsupported_version_range_no_check(self, project_dict: Dict[str, Any]) -> None: + project_dict["require-dbt-version"] = [">0.0.0", "<=0.0.1"] + specifiers = _get_required_version(project_dict=project_dict, verify_version=False) + assert set(x.to_version_string() for x in specifiers) == {">0.0.0", "<=0.0.1"} + + def test_impossible_version_range(self, project_dict: Dict[str, Any]) -> None: + project_dict["require-dbt-version"] = [">99999.0.0", "<=0.0.1"] + with pytest.raises( + dbt.exceptions.DbtProjectError, + match="The package version requirement can never be satisfied", + ): + _get_required_version(project_dict=project_dict, verify_version=True) diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 54124426400..108a2c0ea10 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -85,57 +85,12 @@ def from_parts(self, exc=None): else: return err - def test_supported_version(self): - self.default_project_data["require-dbt-version"] = ">0.0.0" - conf = self.from_parts() - self.assertEqual(set(x.to_version_string() for x in conf.dbt_version), {">0.0.0"}) - - def test_unsupported_version(self): - self.default_project_data["require-dbt-version"] = ">99999.0.0" - raised = self.from_parts(dbt.exceptions.DbtProjectError) - self.assertIn("This version of dbt is not supported", str(raised.exception)) - - def test_unsupported_version_no_check(self): - self.default_project_data["require-dbt-version"] = ">99999.0.0" - self.args.version_check = False - set_from_args(self.args, None) - conf = self.from_parts() - self.assertEqual(set(x.to_version_string() for x in conf.dbt_version), {">99999.0.0"}) - - def test_supported_version_range(self): - self.default_project_data["require-dbt-version"] = [">0.0.0", "<=99999.0.0"] - conf = self.from_parts() - self.assertEqual( - set(x.to_version_string() for x in conf.dbt_version), {">0.0.0", "<=99999.0.0"} - ) - - def test_unsupported_version_range(self): - self.default_project_data["require-dbt-version"] = [">0.0.0", "<=0.0.1"] - raised = self.from_parts(dbt.exceptions.DbtProjectError) - self.assertIn("This version of dbt is not supported", str(raised.exception)) - def test_unsupported_version_range_bad_config(self): self.default_project_data["require-dbt-version"] = [">0.0.0", "<=0.0.1"] self.default_project_data["some-extra-field-not-allowed"] = True raised = self.from_parts(dbt.exceptions.DbtProjectError) self.assertIn("This version of dbt is not supported", str(raised.exception)) - def test_unsupported_version_range_no_check(self): - self.default_project_data["require-dbt-version"] = [">0.0.0", "<=0.0.1"] - self.args.version_check = False - set_from_args(self.args, None) - conf = self.from_parts() - self.assertEqual( - set(x.to_version_string() for x in conf.dbt_version), {">0.0.0", "<=0.0.1"} - ) - - def test_impossible_version_range(self): - self.default_project_data["require-dbt-version"] = [">99999.0.0", "<=0.0.1"] - raised = self.from_parts(dbt.exceptions.DbtProjectError) - self.assertIn( - "The package version requirement can never be satisfied", str(raised.exception) - ) - def test_unsupported_version_extra_config(self): self.default_project_data["some-extra-field-not-allowed"] = True raised = self.from_parts(dbt.exceptions.ProjectContractError) From 67e1bbec3dfe56db53d8a7d86a6a367a2baf7c42 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 29 May 2024 10:50:02 -0700 Subject: [PATCH 6/9] Create a better test for checking that version checks come before project schema validation --- tests/functional/basic/test_project.py | 23 ++++++++++++++++++++++- tests/unit/config/test_runtime.py | 11 ----------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/tests/functional/basic/test_project.py b/tests/functional/basic/test_project.py index 2bdb101c913..a09dd3ad982 100644 --- a/tests/functional/basic/test_project.py +++ b/tests/functional/basic/test_project.py @@ -4,7 +4,8 @@ import pytest import yaml -from dbt.exceptions import ProjectContractError +from dbt.cli.main import dbtRunner +from dbt.exceptions import DbtProjectError, ProjectContractError from dbt.tests.util import run_dbt, update_config_file, write_config_file simple_model_sql = """ @@ -118,3 +119,23 @@ def test_dbt_cloud_invalid(self, project): with pytest.raises(ProjectContractError) as excinfo: run_dbt() assert expected_err in str(excinfo.value) + + +class TestVersionSpecifierChecksComeBeforeYamlValidation: + def test_version_specifier_checks_before_yaml_validation(self, project) -> None: + runner = dbtRunner() + + # if no version specifier error, we should get a yaml validation error + config_update = {"this-is-not-a-valid-key": "my-value-for-invalid-key"} + update_config_file(config_update, "dbt_project.yml") + result = runner.invoke(["parse"]) + assert result.exception is not None + assert isinstance(result.exception, ProjectContractError) + assert "Additional properties are not allowed" in str(result.exception) + + # add bad version specifier, and assert we get the error for that + update_config_file({"require-dbt-version": [">0.0.0", "<=0.0.1"]}, "dbt_project.yml") + result = runner.invoke(["parse"]) + assert result.exception is not None + assert isinstance(result.exception, DbtProjectError) + assert "This version of dbt is not supported" diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 108a2c0ea10..72e5da4c698 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -85,17 +85,6 @@ def from_parts(self, exc=None): else: return err - def test_unsupported_version_range_bad_config(self): - self.default_project_data["require-dbt-version"] = [">0.0.0", "<=0.0.1"] - self.default_project_data["some-extra-field-not-allowed"] = True - raised = self.from_parts(dbt.exceptions.DbtProjectError) - self.assertIn("This version of dbt is not supported", str(raised.exception)) - - def test_unsupported_version_extra_config(self): - self.default_project_data["some-extra-field-not-allowed"] = True - raised = self.from_parts(dbt.exceptions.ProjectContractError) - self.assertIn("Additional properties are not allowed", str(raised.exception)) - def test_archive_not_allowed(self): self.default_project_data["archive"] = [ { From cbe377e1034584bc2a0725dfb5633b9712e49123 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 29 May 2024 12:29:23 -0700 Subject: [PATCH 7/9] Convert `test_get_metadata` to pytest test --- tests/unit/config/test_runtime.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 72e5da4c698..45e32a9877a 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -4,12 +4,14 @@ from unittest import mock import pytest +from pytest_mock import MockerFixture import dbt.config import dbt.exceptions from dbt import tracking from dbt.config.profile import Profile from dbt.config.project import Project +from dbt.config.runtime import RuntimeConfig from dbt.contracts.project import PackageConfig from dbt.flags import set_from_args from dbt.tests.util import safe_set_invocation_context @@ -58,6 +60,16 @@ def test_from_parts(self, args: Namespace, profile: Profile, project: Project): } assert config.to_project_config() == expected_project + def test_get_metadata(self, mocker: MockerFixture, runtime_config: RuntimeConfig) -> None: + mock_user = mocker.patch.object(tracking, "active_user") + mock_user.id = "cfc9500f-dc7f-4c83-9ea7-2c581c1b38cf" + set_from_args(Namespace(SEND_ANONYMOUS_USAGE_STATS=False), None) + + metadata = runtime_config.get_metadata() + # ensure user_id and send_anonymous_usage_stats are set correctly + assert metadata.user_id == mock_user.id + assert not metadata.send_anonymous_usage_stats + class TestRuntimeConfigOLD(BaseConfigTest): def get_project(self): @@ -121,20 +133,6 @@ def test__warn_for_unused_resource_config_paths_empty(self): finally: dbt.flags.WARN_ERROR = False - @mock.patch.object(tracking, "active_user") - def test_get_metadata(self, mock_user): - project = self.get_project() - profile = self.get_profile() - config = dbt.config.RuntimeConfig.from_parts(project, profile, self.args) - - mock_user.id = "cfc9500f-dc7f-4c83-9ea7-2c581c1b38cf" - set_from_args(Namespace(SEND_ANONYMOUS_USAGE_STATS=False), None) - - metadata = config.get_metadata() - # ensure user_id and send_anonymous_usage_stats are set correctly - self.assertEqual(metadata.user_id, mock_user.id) - self.assertFalse(metadata.send_anonymous_usage_stats) - class TestRuntimeConfigWithConfigs(BaseConfigTest): def setUp(self): From 106111e9e0be2bc071a17d0a6a2f9f36e4336d3e Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 29 May 2024 13:56:10 -0700 Subject: [PATCH 8/9] Refactor `test_archive_not_allowed` to functional test We do already have tests that ensure "extra" keys aren't allowed in the dbt_project.yaml. This test is different because it's checking that a specific key, `archive`, isn't allowed. We do this because at one point in time `archive` _was_ an allowed key. Specifically, we stopped allowing `archive` in dbt-core 0.15.0 via commit [f26948dd](https://github.com/dbt-labs/dbt-core/commit/f26948dde20e04032c58a751019ebe6a1a1c8358). Given that it's been 5 years and a major version, we could probably remove this test, but let's keep it around unless we start supporting `archive` again. --- tests/functional/basic/test_project.py | 28 ++++++++++++++++++++++++++ tests/unit/config/test_runtime.py | 18 ----------------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/tests/functional/basic/test_project.py b/tests/functional/basic/test_project.py index a09dd3ad982..7a4cb9fd1da 100644 --- a/tests/functional/basic/test_project.py +++ b/tests/functional/basic/test_project.py @@ -139,3 +139,31 @@ def test_version_specifier_checks_before_yaml_validation(self, project) -> None: assert result.exception is not None assert isinstance(result.exception, DbtProjectError) assert "This version of dbt is not supported" + + +class TestArchiveNotAllowed: + """At one point in time we supported an 'archive' key in projects, but no longer""" + + def test_archive_not_allowed(self, project): + runner = dbtRunner() + + config_update = { + "archive": { + "source_schema": "a", + "target_schema": "b", + "tables": [ + { + "source_table": "seed", + "target_table": "archive_actual", + "updated_at": "updated_at", + "unique_key": """id || '-' || first_name""", + }, + ], + } + } + update_config_file(config_update, "dbt_project.yml") + + result = runner.invoke(["parse"]) + assert result.exception is not None + assert isinstance(result.exception, ProjectContractError) + assert "Additional properties are not allowed" in str(result.exception) diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 45e32a9877a..1415c974998 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -97,24 +97,6 @@ def from_parts(self, exc=None): else: return err - def test_archive_not_allowed(self): - self.default_project_data["archive"] = [ - { - "source_schema": "a", - "target_schema": "b", - "tables": [ - { - "source_table": "seed", - "target_table": "archive_actual", - "updated_at": "updated_at", - "unique_key": """id || '-' || first_name""", - }, - ], - } - ] - with self.assertRaises(dbt.exceptions.ProjectContractError): - self.get_project() - def test__warn_for_unused_resource_config_paths_empty(self): project = self.from_parts() dbt.flags.WARN_ERROR = True From ce46f02340f84b439dfee10e98d66ef0a26bb7b0 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 29 May 2024 17:10:03 -0700 Subject: [PATCH 9/9] Convert `warn_for_unused_resource_config_paths` tests to use pytest --- tests/unit/config/test_runtime.py | 141 ++++++++---------------------- 1 file changed, 38 insertions(+), 103 deletions(-) diff --git a/tests/unit/config/test_runtime.py b/tests/unit/config/test_runtime.py index 1415c974998..816ec8f98c3 100644 --- a/tests/unit/config/test_runtime.py +++ b/tests/unit/config/test_runtime.py @@ -1,6 +1,7 @@ import os import tempfile from argparse import Namespace +from typing import Any, Dict from unittest import mock import pytest @@ -13,14 +14,12 @@ from dbt.config.project import Project from dbt.config.runtime import RuntimeConfig from dbt.contracts.project import PackageConfig +from dbt.events.types import UnusedResourceConfigPath from dbt.flags import set_from_args from dbt.tests.util import safe_set_invocation_context -from tests.unit.config import ( - BaseConfigTest, - empty_profile_renderer, - project_from_config_norender, - temp_cd, -) +from dbt_common.events.event_manager_client import add_callback_to_manager +from tests.unit.config import BaseConfigTest, temp_cd +from tests.utils import EventCatcher class TestRuntimeConfig: @@ -70,114 +69,50 @@ def test_get_metadata(self, mocker: MockerFixture, runtime_config: RuntimeConfig assert metadata.user_id == mock_user.id assert not metadata.send_anonymous_usage_stats - -class TestRuntimeConfigOLD(BaseConfigTest): - def get_project(self): - return project_from_config_norender( - self.default_project_data, - project_root=self.project_dir, - verify_version=self.args.version_check, - ) - - def get_profile(self): - renderer = empty_profile_renderer() - return dbt.config.Profile.from_raw_profiles( - self.default_profile_data, self.default_project_data["profile"], renderer - ) - - def from_parts(self, exc=None): - with self.assertRaisesOrReturns(exc) as err: - project = self.get_project() - profile = self.get_profile() - - result = dbt.config.RuntimeConfig.from_parts(project, profile, self.args) - - if exc is None: - return result - else: - return err - - def test__warn_for_unused_resource_config_paths_empty(self): - project = self.from_parts() - dbt.flags.WARN_ERROR = True - try: - project.warn_for_unused_resource_config_paths( - { - "models": frozenset( - ( - ("my_test_project", "foo", "bar"), - ("my_test_project", "foo", "baz"), - ) - ) - }, - [], - ) - finally: - dbt.flags.WARN_ERROR = False - - -class TestRuntimeConfigWithConfigs(BaseConfigTest): - def setUp(self): - self.profiles_dir = "/invalid-profiles-path" - self.project_dir = "/invalid-root-path" - super().setUp() - self.default_project_data["project-root"] = self.project_dir - self.default_project_data["models"] = { - "enabled": True, + @pytest.fixture + def used_fqns(self) -> Dict[str, Any]: + return {"models": frozenset((("my_test_project", "foo", "bar"),))} + + def test_warn_for_unused_resource_config_paths( + self, + runtime_config: RuntimeConfig, + used_fqns: Dict[str, Any], + ): + catcher = EventCatcher(event_to_catch=UnusedResourceConfigPath) + add_callback_to_manager(catcher.catch) + + runtime_config.models = { "my_test_project": { "foo": { "materialized": "view", "bar": { "materialized": "table", }, - }, - "baz": { - "materialized": "table", - }, - }, - } - self.used = { - "models": frozenset( - ( - ("my_test_project", "foo", "bar"), - ("my_test_project", "foo", "baz"), - ) - ) + "baz": { + "materialized": "table", + }, + } + } } - def get_project(self): - return project_from_config_norender( - self.default_project_data, project_root=self.project_dir, verify_version=True - ) - - def get_profile(self): - renderer = empty_profile_renderer() - return dbt.config.Profile.from_raw_profiles( - self.default_profile_data, self.default_project_data["profile"], renderer - ) - - def from_parts(self, exc=None): - with self.assertRaisesOrReturns(exc) as err: - project = self.get_project() - profile = self.get_profile() + runtime_config.warn_for_unused_resource_config_paths(used_fqns, []) + len(catcher.caught_events) == 1 + expected_msg = "models.my_test_project.foo.baz" + assert expected_msg in str(catcher.caught_events[0].data) - result = dbt.config.RuntimeConfig.from_parts(project, profile, self.args) + def test_warn_for_unused_resource_config_paths_empty_models( + self, + runtime_config: RuntimeConfig, + used_fqns: Dict[str, Any], + ) -> None: + catcher = EventCatcher(event_to_catch=UnusedResourceConfigPath) + add_callback_to_manager(catcher.catch) - if exc is None: - return result - else: - return err + # models should already be empty, but lets ensure it + runtime_config.models = {} - def test__warn_for_unused_resource_config_paths(self): - project = self.from_parts() - with mock.patch("dbt.config.runtime.warn_or_error") as warn_or_error_patch: - project.warn_for_unused_resource_config_paths(self.used, []) - warn_or_error_patch.assert_called_once() - event = warn_or_error_patch.call_args[0][0] - assert type(event).__name__ == "UnusedResourceConfigPath" - msg = event.message() - expected_msg = "- models.my_test_project.baz" - assert expected_msg in msg + runtime_config.warn_for_unused_resource_config_paths(used_fqns, ()) + assert len(catcher.caught_events) == 0 class TestRuntimeConfigFiles(BaseConfigTest):