From 8b8f9bbf04d297a4da38c9716bf4686a43fd01b1 Mon Sep 17 00:00:00 2001 From: Mike Alfare Date: Mon, 9 Oct 2023 19:38:37 -0400 Subject: [PATCH] add compilation error for invalid setting of store_failures_as --- core/dbt/contracts/graph/model_config.py | 39 +++++++++++-------- .../macros/materializations/tests/test.sql | 7 ++++ .../store_test_failures_tests/_files.py | 8 ++++ .../store_test_failures_tests/basic.py | 23 +++++++++++ .../test_store_test_failures.py | 5 +++ 5 files changed, 65 insertions(+), 17 deletions(-) diff --git a/core/dbt/contracts/graph/model_config.py b/core/dbt/contracts/graph/model_config.py index 4873ec83d19..aea8eb117c9 100644 --- a/core/dbt/contracts/graph/model_config.py +++ b/core/dbt/contracts/graph/model_config.py @@ -575,34 +575,39 @@ def __post_init__(self): is to include "ephemeral" as a value for `store_failures_as`, which effectively sets `store_failures=False`. + The exception handling for this is tricky. If we raise an exception here, the entire run fails at + parse time. We would rather well-formed models run successfully, leaving only exceptions to be rerun + if necessary. Hence, the exception needs to be raised in the test materialization. In order to do so, + we need to make sure that we go down the `store_failures = True` route with the invalid setting for + `store_failures_as`. This results in the `.get()` defaulted to `True` below, instead of a normal + dictionary lookup as is done in the `if` block. Refer to the test materialization for the + exception that is raise as a result of an invalid value. + The intention of this block is to behave as if `store_failures_as` is the only setting, but still allow for backwards compatibility for `store_failures`. See https://github.com/dbt-labs/dbt-core/issues/6914 for more information. """ - # if `store_failures_as` is set, it dictates what `store_failures` gets set to - store_failures_map = { - "ephemeral": False, - "table": True, - "view": True, - } - # if `store_failures_as` is not set, it gets set by `store_failures` - store_failures_as_map = { + # the settings below mimic existing behavior prior to `store_failures_as` + get_store_failures_as_map = { True: "table", False: "ephemeral", None: None, } - if self.store_failures_as in store_failures_map: - self.store_failures = store_failures_map[self.store_failures_as] - elif self.store_failures_as is None: - self.store_failures_as = store_failures_as_map[self.store_failures] - else: # `store_failures_as` is set to an unsupported value - raise CompilationError( - f"""{self.store_failures_as} is not a valid value for `store_failures_as`. """ - f"""Accepted values are: {str(list(store_failures_map.keys()))}""" - ) + # if `store_failures_as` is set, it dictates what `store_failures` gets set to + # the settings below overrides whatever `store_failures` is set to by the user + get_store_failures_map = { + "ephemeral": False, + "table": True, + "view": True, + } + + if self.store_failures_as is None: + self.store_failures_as = get_store_failures_as_map[self.store_failures] + else: + self.store_failures = get_store_failures_map.get(self.store_failures_as, True) @classmethod def same_contents(cls, unrendered: Dict[str, Any], other: Dict[str, Any]) -> bool: diff --git a/core/dbt/include/global_project/macros/materializations/tests/test.sql b/core/dbt/include/global_project/macros/materializations/tests/test.sql index 238faf3fd1c..ba205a9b295 100644 --- a/core/dbt/include/global_project/macros/materializations/tests/test.sql +++ b/core/dbt/include/global_project/macros/materializations/tests/test.sql @@ -11,6 +11,13 @@ -- if `--store-failures` is invoked via command line and `store_failures_as` is not set, -- config.get('store_failures_as', 'table') returns None, not 'table' {% if store_failures_as == none %}{% set store_failures_as = 'table' %}{% endif %} + {% if store_failures_as not in ['table', 'view'] %} + {{ exceptions.raise_compiler_error( + "'" ~ store_failures_as ~ "' is not a valid value for `store_failures_as`. " + "Accepted values are: ['ephemeral', 'table', 'view']" + ) }} + {% endif %} + {% set target_relation = api.Relation.create( identifier=identifier, schema=schema, database=database, type=store_failures_as) -%} %} diff --git a/tests/adapter/dbt/tests/adapter/store_test_failures_tests/_files.py b/tests/adapter/dbt/tests/adapter/store_test_failures_tests/_files.py index d6302ff56c8..b3e296e730a 100644 --- a/tests/adapter/dbt/tests/adapter/store_test_failures_tests/_files.py +++ b/tests/adapter/dbt/tests/adapter/store_test_failures_tests/_files.py @@ -117,6 +117,14 @@ """ +TEST__ERROR_UNSET = """ +{{ config(store_failures_as="error") }} +select * +from {{ ref('chipmunks') }} +where shirt = 'green' +""" + + SCHEMA_YML = """ version: 2 diff --git a/tests/adapter/dbt/tests/adapter/store_test_failures_tests/basic.py b/tests/adapter/dbt/tests/adapter/store_test_failures_tests/basic.py index cfdd59d3c14..e8beb0f1fde 100644 --- a/tests/adapter/dbt/tests/adapter/store_test_failures_tests/basic.py +++ b/tests/adapter/dbt/tests/adapter/store_test_failures_tests/basic.py @@ -280,3 +280,26 @@ def test_tests_run_successfully_and_are_stored_as_expected(self, project): TestResult("not_null_chipmunks_shirt", TestStatus.Fail, "view"), } self.run_and_assert(project, expected_results) + + +class StoreTestFailuresAsExceptions(StoreTestFailuresAsBase): + """ + This tests that `store_failures_as` raises exceptions in appropriate scenarios. + Test Scenarios: + + - If `store_failures_as = "error"`, a helpful exception is raised. + """ + + @pytest.fixture(scope="class") + def tests(self): + return { + "store_failures_as_error.sql": _files.TEST__ERROR_UNSET, + } + + def test_tests_run_unsuccessfully_and_raise_appropriate_exception(self, project): + results = run_dbt(["test"], expect_pass=False) + assert len(results) == 1 + result = results[0] + assert "Compilation Error" in result.message + assert "'error' is not a valid value" in result.message + assert "Accepted values are: ['ephemeral', 'table', 'view']" in result.message diff --git a/tests/functional/store_test_failures/test_store_test_failures.py b/tests/functional/store_test_failures/test_store_test_failures.py index 39d2e08dc9f..8783e1903e3 100644 --- a/tests/functional/store_test_failures/test_store_test_failures.py +++ b/tests/functional/store_test_failures/test_store_test_failures.py @@ -6,6 +6,7 @@ StoreTestFailuresAsProjectLevelView, StoreTestFailuresAsProjectLevelEphemeral, StoreTestFailuresAsGeneric, + StoreTestFailuresAsExceptions, ) @@ -39,3 +40,7 @@ class TestStoreTestFailuresAsProjectLevelEphemeral( class TestStoreTestFailuresAsGeneric(StoreTestFailuresAsGeneric, PostgresMixin): pass + + +class TestStoreTestFailuresAsExceptions(StoreTestFailuresAsExceptions, PostgresMixin): + pass