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

Support descriptions on tests #3302

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
## dbt 0.20.0 (Release TBD)

### Features
- Support descriptions defined as a yaml property for tests. ([#3249](https://github.com/fishtown-analytics/dbt/issues/3249), [#3302](https://github.com/fishtown-analytics/dbt/pull/3302))

### Fixes
- Fix compiled sql for ephemeral models ([#3317](https://github.com/fishtown-analytics/dbt/issues/3317), [#3318](https://github.com/fishtown-analytics/dbt/pull/3318))
- Now generating `run_results.json` even when no nodes are selected ([#3313](https://github.com/fishtown-analytics/dbt/issues/3313), [#3315](https://github.com/fishtown-analytics/dbt/pull/3315))
Expand Down
7 changes: 4 additions & 3 deletions core/dbt/config/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,10 @@ def _all_source_paths(
snapshot_paths: List[str],
analysis_paths: List[str],
macro_paths: List[str],
test_paths: List[str],
) -> List[str]:
return list(chain(source_paths, data_paths, snapshot_paths, analysis_paths,
macro_paths))
macro_paths, test_paths))


T = TypeVar('T')
Expand Down Expand Up @@ -333,7 +334,7 @@ def create_project(self, rendered: RenderComponents) -> 'Project':

all_source_paths: List[str] = _all_source_paths(
source_paths, data_paths, snapshot_paths, analysis_paths,
macro_paths
macro_paths, test_paths
)

docs_paths: List[str] = value_or(cfg.docs_paths, all_source_paths)
Expand Down Expand Up @@ -530,7 +531,7 @@ class Project:
def all_source_paths(self) -> List[str]:
return _all_source_paths(
self.source_paths, self.data_paths, self.snapshot_paths,
self.analysis_paths, self.macro_paths
self.analysis_paths, self.macro_paths, self.test_paths
)

def __str__(self):
Expand Down
3 changes: 3 additions & 0 deletions core/dbt/config/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ def should_render_keypath(self, keypath: Keypath) -> bool:
return False
elif self._is_norender_key(keypath[1:]):
return False
elif keypath[0] == NodeType.Test.pluralize():
if keypath[2] == 'description':
return False
else: # keypath[0] in self.DOCUMENTABLE_NODES:
if self._is_norender_key(keypath[1:]):
return False
Expand Down
19 changes: 13 additions & 6 deletions core/dbt/contracts/graph/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ class Disabled(Generic[D]):

def _update_into(dest: MutableMapping[str, T], new_item: T):
"""Update dest to overwrite whatever is at dest[new_item.unique_id] with
new_itme. There must be an existing value to overwrite, and they two nodes
new_item. There must be an existing value to overwrite, and they two nodes
must have the same original file path.
"""
unique_id = new_item.unique_id
Expand Down Expand Up @@ -729,8 +729,12 @@ def patch_nodes(self) -> None:
# were ok with doing an O(n*m) search (one nodes scan per patch)
# Q: could we save patches by node unique_ids instead, or convert
# between names and node ids?
patch_keys = set(self.patches.keys())
used_patch_keys = set()

for node in self.nodes.values():
patch = self.patches.pop(node.name, None)
patch_lookup_key = node.patch_lookup_key
patch = self.patches.get(patch_lookup_key, None)
if not patch:
continue
Comment on lines 731 to 739
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new/diverging behavior! Previously, a patch can only be applied once to a single node. The new behavior allows patches to be applied to multiple nodes to support applying the same patch to multiple instances of the same generic test.


Expand All @@ -745,13 +749,16 @@ def patch_nodes(self) -> None:
raise_invalid_patch(
node, patch.yaml_key, patch.original_file_path
)

node.patch(patch)
used_patch_keys.add(patch_lookup_key)

unused_patch_keys = patch_keys - used_patch_keys

# If anything is left in self.patches, it means that the node for
# if there are any unused patch names, it means that the node for
# that patch wasn't found.
if self.patches:
for patch in self.patches.values():
if unused_patch_keys:
for patch_key in unused_patch_keys:
patch = self.patches[patch_key]
# since patches aren't nodes, we can't use the existing
# target_not_found warning
logger.debug((
Expand Down
4 changes: 4 additions & 0 deletions core/dbt/contracts/graph/parsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,10 @@ class ParsedSchemaTestNode(ParsedNode, HasTestMetadata):
column_name: Optional[str] = None
config: TestConfig = field(default_factory=TestConfig)

@property
def patch_lookup_key(self) -> str:
return self.test_metadata.name

def same_config(self, other) -> bool:
return (
self.unrendered_config.get('severity') ==
Expand Down
6 changes: 5 additions & 1 deletion core/dbt/contracts/graph/unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ class UnparsedNode(UnparsedBaseNode, HasSQL):
]})

@property
def search_name(self):
def patch_lookup_key(self) -> str:
return self.name

@property
def search_name(self) -> str:
return self.name


Expand Down
3 changes: 2 additions & 1 deletion core/dbt/node_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ def documentable(cls) -> List['NodeType']:
cls.Source,
cls.Macro,
cls.Analysis,
cls.Exposure
cls.Exposure,
cls.Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirmed that both types of tests (generic + bespoke) can use doc blocks in their descriptions, nice!

]

def pluralize(self) -> str:
Expand Down
51 changes: 37 additions & 14 deletions core/dbt/parser/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
Manifest, Disabled, MacroManifest, ManifestStateCheck
)
from dbt.contracts.graph.parsed import (
ParsedSourceDefinition, ParsedNode, ParsedMacro, ColumnInfo, ParsedExposure
ParsedSchemaTestNode, ParsedSourceDefinition, ParsedMacro, ColumnInfo, ParsedExposure
)
from dbt.contracts.util import Writable
from dbt.exceptions import (
Expand Down Expand Up @@ -693,12 +693,6 @@ def _get_node_column(node, column_name):
return column


DocsContextCallback = Callable[
[Union[ParsedNode, ParsedSourceDefinition]],
Dict[str, Any]
]


# node and column descriptions
def _process_docs_for_node(
context: Dict[str, Any],
Expand Down Expand Up @@ -749,12 +743,42 @@ def _process_docs_for_exposure(
# exposures: exposure descriptions
def process_docs(manifest: Manifest, config: RuntimeConfig):
for node in manifest.nodes.values():
ctx = generate_runtime_docs(
config,
node,
manifest,
config.project_name,
)
if isinstance(node, ParsedSchemaTestNode):
# Idealing we should be able to put the test kwargs in the context,
# but kwargs have already been processed at this point
# ex: 'model' --> "{{ ref('model') }}"
Comment on lines +747 to +749
Copy link
Contributor

@jtcohen6 jtcohen6 May 10, 2021

Choose a reason for hiding this comment

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

Just to clarify my understanding: The test_metadata that lives on the test node, and contains kwargs as well, is not Jinja-rendered. E.g. from the representation in manifest.json:

"test_metadata": {
    "name": "accepted_values",
    "kwargs": {
        "values": [
            "usd"
        ],
        "column_name": "currency_code",
        "model": "{{ ref('stg_ticket_tailor__orders') }}"
    },
    "namespace": null
},

In order to include kwargs in the test description, we'd need to upgrade the description renderer to use the full Jinja rendering context (refs, macros, all that). Is that right?

ctx = generate_runtime_docs(
config,
node,
manifest,
config.project_name,
)

model = []

if len(node.refs):
model = node.refs[0]
elif len(node.sources):
model = node.sources[0]

if len(model) == 1:
target_model_name = model[0]
elif len(model) == 2:
_, target_model_name = model
else:
raise dbt.exceptions.InternalException(
f'Refs and sources should always be 1 or 2 arguments - got {len(model)}'
)

ctx['column_name'] = node.column_name
ctx['model'] = target_model_name
Comment on lines +757 to +774
Copy link
Contributor Author

Choose a reason for hiding this comment

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

blah hardcoded values set directly on the context, I tried to avoid this without success

Copy link
Contributor

Choose a reason for hiding this comment

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

This means that model and column_name are the only additions to the test description rendering context, right? I think that's totally ok for the v1 of this! In the future, it would be neat if we could grab other args and include them in the rendering context, but that should 100% not block us from merging this as an amazing first cut.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh I see your comment above. Okay, that makes sense!

else:
ctx = generate_runtime_docs(
config,
node,
manifest,
config.project_name,
)
_process_docs_for_node(ctx, node)
for source in manifest.sources.values():
ctx = generate_runtime_docs(
Expand Down Expand Up @@ -856,7 +880,6 @@ def _process_refs_for_node(
node, target_model_name, target_model_package,
disabled=(isinstance(target_model, Disabled))
)

continue

target_model_id = target_model.unique_id
Expand Down
27 changes: 15 additions & 12 deletions core/dbt/parser/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,14 @@ def _yaml_from_file(
)
return None

def parse_column_tests(
def parse_column_test_instances(
self, block: TestBlock, column: UnparsedColumn
) -> None:
if not column.tests:
return

for test in column.tests:
self.parse_test(block, test, column)
self.parse_test_instance(block, test, column)

def _generate_source_config(self, fqn: List[str], rendered: bool):
generator: BaseContextConfigGenerator
Expand Down Expand Up @@ -393,7 +393,6 @@ def _parse_generic_test(
tags: List[str],
column_name: Optional[str],
) -> ParsedSchemaTestNode:

render_ctx = generate_target_context(
self.root_project, self.root_project.cli_vars
)
Expand Down Expand Up @@ -564,7 +563,7 @@ def render_with_context(
node.raw_sql, context, node, capture_macros=True
)

def parse_test(
def parse_test_instance(
self,
target_block: TestBlock,
test: TestDef,
Expand Down Expand Up @@ -594,12 +593,12 @@ def parse_test(
)
self.parse_node(block)

def parse_tests(self, block: TestBlock) -> None:
def parse_test_instances(self, block: TestBlock) -> None:
for column in block.columns:
self.parse_column_tests(block, column)
self.parse_column_test_instances(block, column)

for test in block.tests:
self.parse_test(block, test, None)
self.parse_test_instance(block, test, None)

def parse_exposures(self, block: YamlBlock) -> None:
parser = ExposureParser(self, block)
Expand Down Expand Up @@ -638,19 +637,19 @@ def parse_file(self, block: FileBlock) -> None:
if 'models' in dct:
parser = TestablePatchParser(self, yaml_block, 'models')
for test_block in parser.parse():
self.parse_tests(test_block)
self.parse_test_instances(test_block)

# NonSourceParser.parse()
if 'seeds' in dct:
parser = TestablePatchParser(self, yaml_block, 'seeds')
for test_block in parser.parse():
self.parse_tests(test_block)
self.parse_test_instances(test_block)

# NonSourceParser.parse()
if 'snapshots' in dct:
parser = TestablePatchParser(self, yaml_block, 'snapshots')
for test_block in parser.parse():
self.parse_tests(test_block)
self.parse_test_instances(test_block)

# This parser uses SourceParser.parse() which doesn't return
# any test blocks. Source tests are handled at a later point
Expand All @@ -663,13 +662,17 @@ def parse_file(self, block: FileBlock) -> None:
if 'macros' in dct:
parser = MacroPatchParser(self, yaml_block, 'macros')
for test_block in parser.parse():
self.parse_tests(test_block)
self.parse_test_instances(test_block)

if 'tests' in dct:
parser = TestablePatchParser(self, yaml_block, 'tests')
parser.parse()

# NonSourceParser.parse()
if 'analyses' in dct:
parser = AnalysisPatchParser(self, yaml_block, 'analyses')
for test_block in parser.parse():
self.parse_tests(test_block)
self.parse_test_instances(test_block)

# parse exposures
if 'exposures' in dct:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
version: 2

models:
- name: table_copy
description: "A copy of the table"
columns:
- name: id
description: "The ID"
tests:
- not_null
- unique
- name: first_name
description: "The user's first name"
- name: ip_address
description: "The user's IP address"
tests:
- unique
- name: updated_at
description: "The update time of the user"
- name: email
description: "The user's email address"
- name: favorite_color
description: "The user's favorite color"
- name: fav_number
description: "The user's favorite number"

tests:
- name: not_null
description: "test not null description for column {{ column_name }} on model {{ model }}"
- name: unique
description: "test unique description for column {{ column_name }} on model {{ model }}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

{{
config(
materialized='table'
)
}}

select * from {{ this.schema }}.seed
35 changes: 33 additions & 2 deletions test/integration/008_schema_tests_test/test_schema_v2_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,6 @@ def test_postgres_test_context_tests(self):

results = self.run_dbt(['test'], expect_pass=False)
self.assertEqual(len(results), 2)
result0 = results[0]
result1 = results[1]
for result in results:
if result.node.name == 'type_two_model_a_':
# This will be WARN if the test macro was rendered correctly
Expand All @@ -377,3 +375,36 @@ def test_postgres_test_context_tests(self):
# was rendered correctly
self.assertRegex(result.node.compiled_sql, r'union all')


class TestSchemaTestProperties(DBTIntegrationTest):
def setUp(self):
DBTIntegrationTest.setUp(self)
self.run_sql_file("seed.sql")

@property
def schema(self):
return "schema_tests_008"

@property
def models(self):
return "models-v2/test-properties"

@use_profile('postgres')
def test_postgres_test_description(self):
results = self.run_dbt()
self.assertEqual(len(results), 1)
results = self.run_dbt(['test'])
self.assertEqual(len(results), 3)

test_descriptions = [
'test not null description for column id on model table_copy',
'test unique description for column id on model table_copy',
'test unique description for column ip_address on model table_copy'
]

test_result_descriptions = []

for result in results:
test_result_descriptions.append(result.node.description)

self.assertListEqual(sorted(test_result_descriptions), sorted(test_descriptions))
Loading