Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADAP-850: Support test results as a view #8653

Merged
merged 40 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
718b892
add strategy parameter to TestConfig, default to ephemeral, catch str…
mikealfare Sep 15, 2023
d4dfec8
changie
mikealfare Sep 15, 2023
e999ed2
create test results as views
mikealfare Sep 16, 2023
f5e0797
removed unnecessary formatting changes
mikealfare Sep 16, 2023
8e366bb
removed unnecessary formatting changes
mikealfare Sep 16, 2023
1a3bd81
removed unnecessary formatting changes
mikealfare Sep 16, 2023
7b4be33
removed unnecessary formatting changes
mikealfare Sep 16, 2023
a149d66
removed unnecessary formatting changes
mikealfare Sep 16, 2023
0313904
removed unnecessary formatting changes
mikealfare Sep 16, 2023
ccf199c
removed unnecessary formatting changes
mikealfare Sep 16, 2023
f76fc74
removed unnecessary formatting changes
mikealfare Sep 16, 2023
9a0656f
updated test expected values for new config option
mikealfare Sep 16, 2023
40b646b
break up tests into reusable tests and adapter specific configuration…
mikealfare Sep 16, 2023
661c5f6
use built-in utility to test relation types, reduce configuration to …
mikealfare Sep 21, 2023
3fa0858
move configuration into base test class
mikealfare Sep 21, 2023
49b797e
separate setup and teardown methods, move postgres piece back into db…
mikealfare Sep 21, 2023
069a9b2
shorten inserted record values to avoid data type issues in the model…
mikealfare Sep 21, 2023
61d514b
shorten inserted record values to avoid data type issues in the model…
mikealfare Sep 21, 2023
beb0ed4
add audit schema suffix as class attribute
mikealfare Sep 22, 2023
5aa2031
rename `strategy` parameter to `store_failures_as`; allow `store_fail…
mikealfare Sep 29, 2023
9971be7
updated changelog entry to reflect correct parameters
mikealfare Sep 29, 2023
3ddc7d4
updated changelog entry to reflect correct parameters
mikealfare Sep 29, 2023
4b81e22
revert unexpected formatting changes from black
mikealfare Sep 29, 2023
0833774
revert test debugging artifacts
mikealfare Sep 29, 2023
eb1f3b4
update local variable to be more intuitive
mikealfare Sep 29, 2023
cf86565
update default for store_test_failures_as; rename postgres test to re…
mikealfare Sep 29, 2023
2f1aa91
remove duplicated default for store_failures_as
mikealfare Sep 29, 2023
96d18a9
update expected test config dicts to include the new default value fo…
mikealfare Sep 29, 2023
c214595
Merge branch 'main' into feature/materialized-tests/adap-850
mikealfare Sep 29, 2023
0baf090
Add `store_failures_as` config for generic tests
dbeatty10 Sep 30, 2023
1b1d9c6
revert code to prioritize store_failures_as over store_failures
mikealfare Oct 3, 2023
6c93e34
cover --store-failures on CLI gap
mikealfare Oct 3, 2023
a47e59e
add generic tests test case for store_failures_as
mikealfare Oct 3, 2023
86b2793
Merge branch 'main' into feature/materialized-tests/adap-850
mikealfare Oct 3, 2023
90792e0
Merge branch 'dbeatty/config-test-materializations-2' into feature/ma…
mikealfare Oct 3, 2023
c09b81d
update object names for generic test case tests for store_failures_as
mikealfare Oct 3, 2023
729d939
remove unique generic test, it was not testing `store_failures_as`
mikealfare Oct 4, 2023
9d3fb06
pull generic run and assertion into base test class to turn tests int…
mikealfare Oct 4, 2023
e70dca3
add ephemeral option for store_failures_as, as a way to easily turn o…
mikealfare Oct 9, 2023
8b8f9bb
add compilation error for invalid setting of store_failures_as
mikealfare Oct 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20230915-174428.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Support persisting test results as views
time: 2023-09-15T17:44:28.833877-04:00
custom:
Author: mikealfare
Issue: "6914"
3 changes: 2 additions & 1 deletion core/dbt/contracts/graph/model_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ class Hook(dbtClassMixin, Replaceable):

@dataclass
class BaseConfig(AdditionalPropertiesAllowed, Replaceable):

# enable syntax like: config['key']
def __getitem__(self, key):
return self.get(key)
Expand Down Expand Up @@ -560,6 +559,7 @@ class TestConfig(NodeAndTestConfig):
fail_calc: str = "count(*)"
warn_if: str = "!= 0"
error_if: str = "!= 0"
strategy: Optional[str] = None

@classmethod
def same_contents(cls, unrendered: Dict[str, Any], other: Dict[str, Any]) -> bool:
Expand All @@ -572,6 +572,7 @@ def same_contents(cls, unrendered: Dict[str, Any], other: Dict[str, Any]) -> boo
"warn_if",
"error_if",
"store_failures",
"strategy",
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
]

seen = set()
Expand Down
5 changes: 3 additions & 2 deletions core/dbt/contracts/graph/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,6 @@
or enforced_column_constraint_removed
or materialization_changed
):

breaking_changes = []
if contract_enforced_disabled:
breaking_changes.append(
Expand Down Expand Up @@ -984,7 +983,9 @@
class TestShouldStoreFailures:
@property
def should_store_failures(self):
if self.config.store_failures:
if self.config.strategy in ["view", "table"]:
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
return True

Check warning on line 987 in core/dbt/contracts/graph/nodes.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/contracts/graph/nodes.py#L987

Added line #L987 was not covered by tests
elif self.config.store_failures:
return self.config.store_failures
return get_flags().STORE_FAILURES

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@

{% macro should_store_failures() %}
{% set config_store_failures = config.get('store_failures') %}
{% if config_store_failures is none %}
{% set config_store_failures_strategy = config.get('strategy') %}
{% if config_store_failures_strategy in ['view', 'table'] %}
{% set config_store_failures = True %}
{% elif config_store_failures is none %}
{% set config_store_failures = flags.STORE_FAILURES %}
{% endif %}
{% do return(config_store_failures) %}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,33 @@
{%- materialization test, default -%}

{% set relations = [] %}
{% set relation_type = config.get('strategy') %}

{% if should_store_failures() %}
{% if relation_type in ['view', 'table'] %}

{% set identifier = model['alias'] %}
{% set existing_relation = adapter.get_relation(database=database, schema=schema, identifier=identifier) %}
{% set target_relation = api.Relation.create(
database=database, schema=schema, identifier=identifier, type=relation_type
) %}

{% call statement(auto_begin=True) %}
{% if existing_relation %}
{{ get_replace_sql(existing_relation, target_relation, sql) }}
{% else %}
{{ get_create_sql(target_relation, sql) }}
{% endif %}
{% endcall %}

{% do relations.append(target_relation) %}

{% set main_sql %}
select * from {{ target_relation }}
{% endset %}

{{ adapter.commit() }}

{% elif should_store_failures() %}
Copy link
Contributor

@dataders dataders Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's my read on the logic

  1. if relation_type is configured, then depending on the argument materialize a table or view
  2. else, if should_store_failures do all that jazz.

the first thing reads to me effectively as a sub-materialization, that is independent of that. so might i be able to pass should_store_failures() and relation_type='table'? what happens then?

I'm down with a deprecation in the future, but in the meantime might be deprecate the implementation and make config.get('store_failures') imply relation_type='table'? then there's only one code path for creating a table and storing test results, and the logic within the L30 conditional can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so might i be able to pass should_store_failures() and relation_type='table'? what happens then?

It will create it as a table. This is the same as omitting the 'table' piece entirely.

but in the meantime might be deprecate the implementation and make config.get('store_failures') imply relation_type='table'?

That's the conclusion that @colin-rogers-dbt and I reached as well. That alters the feature request slightly, but it makes much more intuitive sense. The feature request is now effectively:

I know store_failures creates the results as as table, but I want to make it create the results as a view.

Instead of implementing a feature alongside store_failures, we're augmenting stored_failures itself.


{% set identifier = model['alias'] %}
{% set old_relation = adapter.get_relation(database=database, schema=schema, identifier=identifier) %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@

{%- macro default__get_create_sql(relation, sql) -%}

{%- if relation.is_materialized_view -%}
{%- if relation.is_view -%}
{{ get_create_view_as_sql(relation, sql) }}

{%- elif relation.is_table -%}
{{ get_create_table_as_sql(False, relation, sql) }}

{%- elif relation.is_materialized_view -%}
{{ get_create_materialized_view_as_sql(relation, sql) }}

{%- else -%}
Expand Down
44 changes: 44 additions & 0 deletions tests/adapter/dbt/tests/adapter/persist_test_results/_files.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
SEED__CHIPMUNKS = """
name,shirt
alvin,red
simon,blue
theodore,green
""".strip()


MODEL__CHIPMUNKS = """
{{ config(materialized='table') }}
select *
from {{ ref('chipmunks_stage') }}
"""

TEST__FAIL_WITH_VIEW_STRATEGY = """
{{ config(strategy="view") }}
select *
from {{ ref('chipmunks') }}
where shirt = 'green'
"""


TEST__PASS_WITH_VIEW_STRATEGY = """
{{ config(strategy="view") }}
select *
from {{ ref('chipmunks') }}
where shirt = 'grape'
"""


TEST__FAIL_WITH_TABLE_STRATEGY = """
{{ config(strategy="table") }}
select *
from {{ ref('chipmunks') }}
where shirt = 'green'
"""


TEST__PASS_WITH_TABLE_STRATEGY = """
{{ config(strategy="table") }}
select *
from {{ ref('chipmunks') }}
where shirt = 'purple'
"""
154 changes: 154 additions & 0 deletions tests/adapter/dbt/tests/adapter/persist_test_results/basic.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
from collections import namedtuple
from typing import Dict

import pytest

from dbt.contracts.results import TestStatus
from dbt.tests.util import run_dbt, check_relation_types

from dbt.tests.adapter.persist_test_results._files import (
SEED__CHIPMUNKS,
MODEL__CHIPMUNKS,
TEST__FAIL_WITH_VIEW_STRATEGY,
TEST__PASS_WITH_VIEW_STRATEGY,
TEST__FAIL_WITH_TABLE_STRATEGY,
TEST__PASS_WITH_TABLE_STRATEGY,
)


class PersistTestResults:
seed_table: str = "chipmunks_stage"
model_table: str = "chipmunks"
audit_schema_suffix: str = "dbt_test__audit"

audit_schema: str

@pytest.fixture(scope="class", autouse=True)
def setup_class(self, project):
# the seed doesn't get touched, load it once
run_dbt(["seed"])
yield

@pytest.fixture(scope="function", autouse=True)
def setup_method(self, project, setup_class):
# make sure the model is always right
run_dbt(["run"])

# the name of the audit schema doesn't change in a class, but this doesn't run at the class level
self.audit_schema = f"{project.test_schema}_{self.audit_schema_suffix}"
yield

@pytest.fixture(scope="function", autouse=True)
def teardown_method(self, project):
yield

# clear out the audit schema after each test case
with project.adapter.connection_named("__test"):
audit_schema = project.adapter.Relation.create(
database=project.database, schema=self.audit_schema
)
project.adapter.drop_schema(audit_schema)

@pytest.fixture(scope="class")
def seeds(self):
return {f"{self.seed_table}.csv": SEED__CHIPMUNKS}

@pytest.fixture(scope="class")
def models(self):
return {f"{self.model_table}.sql": MODEL__CHIPMUNKS}

@pytest.fixture(scope="class")
def tests(self):
return {
"fail_with_view_strategy.sql": TEST__FAIL_WITH_VIEW_STRATEGY,
"pass_with_view_strategy.sql": TEST__PASS_WITH_VIEW_STRATEGY,
"fail_with_table_strategy.sql": TEST__FAIL_WITH_TABLE_STRATEGY,
"pass_with_table_strategy.sql": TEST__PASS_WITH_TABLE_STRATEGY,
}

def row_count(self, project, relation_name: str) -> int:
"""
Return the row count for the relation.

Args:
project: the project fixture
relation_name: the name of the relation

Returns:
the row count as an integer
"""
sql = f"select count(*) from {self.audit_schema}.{relation_name}"
return project.run_sql(sql, fetch="one")[0]

def insert_record(self, project, record: Dict[str, str]):
field_names, field_values = [], []
for field_name, field_value in record.items():
field_names.append(field_name)
field_values.append(f"'{field_value}'")
field_name_clause = ", ".join(field_names)
field_value_clause = ", ".join(field_values)

sql = f"""
insert into {project.test_schema}.{self.model_table} ({field_name_clause})
values ({field_value_clause})
"""
project.run_sql(sql)

def delete_record(self, project, record: Dict[str, str]):
where_clause = " and ".join(
[f"{field_name} = '{field_value}'" for field_name, field_value in record.items()]
)
sql = f"""
delete from {project.test_schema}.{self.model_table}
where {where_clause}
"""
project.run_sql(sql)

def test_tests_run_successfully_and_are_persisted_correctly(self, project):
# set up the expected results
TestResult = namedtuple("TestResult", ["name", "status", "type", "row_count"])
expected_results = {
TestResult("pass_with_view_strategy", TestStatus.Pass, "view", 0),
TestResult("fail_with_view_strategy", TestStatus.Fail, "view", 1),
TestResult("pass_with_table_strategy", TestStatus.Pass, "table", 0),
TestResult("fail_with_table_strategy", TestStatus.Fail, "table", 1),
}

# run the tests once
results = run_dbt(["test"], expect_pass=False)

# show that the statuses are what we expect
actual = {(result.node.name, result.status) for result in results}
expected = {(result.name, result.status) for result in expected_results}
assert actual == expected

# show that the results are persisted in the correct database objects
check_relation_types(
project.adapter, {result.name: result.type for result in expected_results}
)

# show that only the failed records show up
actual = {
(result.name, self.row_count(project, result.name)) for result in expected_results
}
expected = {(result.name, result.row_count) for result in expected_results}
assert actual == expected

# insert a new record in the model that fails the "pass" tests
# show that the view updates, but not the table
self.insert_record(project, {"name": "dave", "shirt": "grape"})
expected_results.remove(TestResult("pass_with_view_strategy", TestStatus.Pass, "view", 0))
expected_results.add(TestResult("pass_with_view_strategy", TestStatus.Pass, "view", 1))

# delete the original record from the model that failed the "fail" tests
# show that the view updates, but not the table
self.delete_record(project, {"name": "theodore", "shirt": "green"})
expected_results.remove(TestResult("fail_with_view_strategy", TestStatus.Fail, "view", 1))
expected_results.add(TestResult("fail_with_view_strategy", TestStatus.Fail, "view", 0))

# show that the views update without needing to run dbt, but the tables do not update
actual = {
(result.name, self.row_count(project, result.name)) for result in expected_results
}
expected = {(result.name, result.row_count) for result in expected_results}
assert actual == expected
2 changes: 1 addition & 1 deletion tests/functional/artifacts/expected_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ def get_rendered_tst_config(**updates):
"tags": [],
"severity": "ERROR",
"store_failures": None,
"strategy": None,
"warn_if": "!= 0",
"error_if": "!= 0",
"fail_calc": "count(*)",
Expand Down Expand Up @@ -201,7 +202,6 @@ def __str__(self):


def expected_seeded_manifest(project, model_database=None, quote_model=False):

model_sql_path = os.path.join("models", "model.sql")
second_model_sql_path = os.path.join("models", "second_model.sql")
model_schema_yml_path = os.path.join("models", "schema.yml")
Expand Down
3 changes: 3 additions & 0 deletions tests/functional/list/test_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ def expect_test_output(self):
"materialized": "test",
"severity": "ERROR",
"store_failures": None,
"strategy": None,
"warn_if": "!= 0",
"error_if": "!= 0",
"fail_calc": "count(*)",
Expand Down Expand Up @@ -470,6 +471,7 @@ def expect_test_output(self):
"materialized": "test",
"severity": "ERROR",
"store_failures": None,
"strategy": None,
"warn_if": "!= 0",
"error_if": "!= 0",
"fail_calc": "count(*)",
Expand Down Expand Up @@ -500,6 +502,7 @@ def expect_test_output(self):
"materialized": "test",
"severity": "ERROR",
"store_failures": None,
"strategy": None,
"warn_if": "!= 0",
"error_if": "!= 0",
"fail_calc": "count(*)",
Expand Down
11 changes: 11 additions & 0 deletions tests/functional/persist_test_results/test_persist_test_results.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import pytest

from dbt.tests.adapter.persist_test_results.basic import PersistTestResults


class TestPersistTestResults(PersistTestResults):
@pytest.fixture(scope="function", autouse=True)
def setup_audit_schema(self, project, setup_method):
# postgres only supports schema names of 63 characters
# a schema with a longer name still gets created, but the name gets truncated
self.audit_schema = self.audit_schema[:63]