Reorganize fixtures and implement a happy path test for semantic_manifest #9930

ChenyuLInx · 2024-04-12T23:23:19Z

resolves #9665
Still might move things around a bit but would love some eyes on it about fixtures structure etc.

Should we have a semantic manifest fixture directly?

This patch moved the existing fixtures to build a manifest around and make it easier to overwrite what nodes are included, while keeping the existing default.
We then used the fixtures to generate a semantic manifest that has both metrics and semantic models test more things in the validate function.

A screenshot of the new unit test coverage for Semantic manifest

Problem

Solution

Checklist

I have read the contributing guide and understand what's expected of me
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
[x]This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
This PR includes type annotations for new and modified functions

…fest.

github-actions · 2024-04-12T23:23:33Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

codecov · 2024-04-12T23:25:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base (ebc22fa) to head (661f45c).
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9930      +/-   ##
==========================================
- Coverage   88.13%   88.06%   -0.07%     
==========================================
  Files         178      180       +2     
  Lines       22449    22546      +97     
==========================================
+ Hits        19785    19856      +71     
- Misses       2664     2690      +26

Flag	Coverage Δ
integration	`85.37% <100.00%> (-0.19%)`	⬇️
unit	`62.13% <100.00%> (+0.23%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aranke · 2024-04-15T13:35:24Z

core/dbt/artifacts/resources/v1/semantic_model.py

@@ -42,7 +42,7 @@ class NodeRelation(dbtClassMixin):
    alias: str
    schema_name: str  # TODO: Could this be called simply "schema" so we could reuse StateRelation?
    database: Optional[str] = None
-    relation_name: Optional[str] = None


Are we using this field anywhere?
If not, might be OK to drop similar to #9794

I will look up but @QMalcolm if you know anything about it

It's used in a couple places in Metricflow:

convert_semantic_model.py::create_sql_source_data_set

data_warehouse_model_validator.py::gen_semantic_model_tasks

Thus it is important for us to power it, and we should not drop it.

gshank

Looks like a big improvement! Good to see that manifest construction code moved out into a reusable fixture.

QMalcolm

This looks great! I had small question on the change in semantic_model.py and some small cleanup requests 🙂

QMalcolm · 2024-04-16T21:48:36Z

tests/unit/conftest.py

@@ -4,6 +4,9 @@
 from dbt.artifacts.resources.types import NodeType
 from dbt.contracts.graph.nodes import SourceDefinition

+# All manifest related fixtures.
+from tests.unit.utils.manifest import *  # noqa


QMalcolm · 2024-04-16T21:51:22Z

core/dbt/artifacts/resources/v1/semantic_model.py

@@ -42,7 +42,7 @@ class NodeRelation(dbtClassMixin):
    alias: str
    schema_name: str  # TODO: Could this be called simply "schema" so we could reuse StateRelation?
    database: Optional[str] = None
-    relation_name: Optional[str] = None
+    relation_name: Optional[str] = ""


This PR is focused on tests and fixtures for tests. Was the default value of relation_name changed to make testing easier, or was there a functional purpose for the change?

So after this got fixed we started seeing test failures that take the form of

______________________ TestSemanticManifest.test_validate ______________________ self = <tests.unit.contracts.graph.test_semantic_manifest.TestSemanticManifest object at 0x7f52df26c040> manifest = Manifest(nodes={'model.test.metricflow_time_spine': ModelNode(database='dbt', schema='analytics', name='metricflow_tim...d_path_count=0, static_analysis_path_count=0), _lock=<Lock(owner=None)>, _macros_by_name=None, _macros_by_package=None) def test_validate(self, manifest): sm_manifest = SemanticManifest(manifest) > assert sm_manifest.validate() tests/unit/contracts/graph/test_semantic_manifest.py:28: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ core/dbt/contracts/graph/semantic_manifest.py:42: in validate semantic_manifest = self._get_pydantic_semantic_manifest() core/dbt/contracts/graph/semantic_manifest.py:69: in _get_pydantic_semantic_manifest PydanticSemanticModel.parse_obj(semantic_model.to_dict()) .tox/unit/lib/python3.8/site-packages/pydantic/v1/main.py:526: in parse_obj return cls(**obj) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ __pydantic_self__ = PydanticSemanticModel() data = {'config': {'enabled': True, 'group': None, 'meta': {}}, 'created_at': 1713305259.7969077, 'defaults': None, 'depends_on': {'macros': [], 'nodes': []}, ...} values = {'defaults': None, 'description': 'Customer entity', 'dimensions': [], 'entities': [], ...} fields_set = {'defaults', 'description', 'dimensions', 'entities', 'label', 'measures', ...} validation_error = ValidationError(model='PydanticSemanticModel', errors=[{'loc': ('node_relation', 'relation_name'), 'msg': 'none is not an allowed value', 'type': 'type_error.none.not_allowed'}]) def __init__(__pydantic_self__, **data: Any) -> None: """ Create a new model by parsing and validating input data from keyword arguments. Raises ValidationError if the input data cannot be parsed to form a valid model. """ # Uses something other than `self` the first arg to allow "self" as a settable attribute values, fields_set, validation_error = validate_model(__pydantic_self__.__class__, data) if validation_error: > raise validation_error E pydantic.v1.error_wrappers.ValidationError: 1 validation error for PydanticSemanticModel E node_relation -> relation_name E none is not an allowed value (type=type_error.none.not_allowed) .tox/unit/lib/python3.8/site-packages/pydantic/v1/main.py:341: ValidationError =============================== warnings summary =============================== tests/unit/test_query_headers.py:44 /home/runner/work/dbt-core/dbt-core/tests/unit/test_query_headers.py:44: DeprecationWarning: invalid escape sequence \/ self.assertTrue(re.match(f"^\/\*.*\*\/\n{self.query}$", sql)) # noqa: [W605] -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ---------- coverage: platform linux, python 3.8.18-final-0 ----------- Coverage XML written to file coverage.xml =========================== short test summary info ============================ FAILED tests/unit/contracts/graph/test_semantic_manifest.py::TestSemanticManifest::test_validate

The test that failed, TestSemanticManifest.test_validate, is a new test added by this PR. The question becomes, is this an error in the code or the test.

It looks like the protocol for NodeRelation does have node_relation as a required string. The PydanticNodeRelation has a specific pydantic validator for handling node_relation 🤔 So why do we in core have it as optional? I should be the person that knows this 🙃

Haha yes, maybe this does deserve a separate PR?

Updating the default value of relation_name to "" in the class definition should be fine. The NodeRelation class is only instantiated on a SemanticModel in one place, and it passes in a value for relation_name.

QMalcolm · 2024-04-16T21:52:27Z

tests/unit/contracts/graph/test_semantic_manifest.py

+from dbt.contracts.graph.semantic_manifest import SemanticManifest
+
+# Request fixtures for manifest, this is simialr to import * in the file.
+# pytest_plugins = ("tests.unit.utils.manifest",)


Now that we've moved the import to conftest.py, can we drop these comments?

QMalcolm · 2024-04-16T21:53:33Z

tests/unit/test_graph_selector_methods.py

+# Request fixtures for manifest, this is simialr to import * in the file.
+pytest_plugins = ("tests.unit.utils.manifest",)


I believe with the update to conftest.py this is no longer needed 🤔

ChenyuLInx · 2024-04-16T22:01:13Z

@QMalcolm Done

QMalcolm

Shweet! Let's get it shipped 🚢

QMalcolm · 2024-04-16T22:27:09Z

More discussion on current test failure in my previous comment questioning the change

ChenyuLInx added 2 commits April 12, 2024 16:11

Reorganize fixtures and implement a happy path test for semantic_mani…

b96cdcf

…fest.

actual add the file, Pycharm was new to me

55f73ae

cla-bot bot added the cla:yes label Apr 12, 2024

ChenyuLInx requested a review from QMalcolm April 12, 2024 23:23

aranke reviewed Apr 15, 2024

View reviewed changes

gshank approved these changes Apr 15, 2024

View reviewed changes

move location

46712cc

ChenyuLInx marked this pull request as ready for review April 16, 2024 21:04

ChenyuLInx requested a review from a team as a code owner April 16, 2024 21:04

QMalcolm requested changes Apr 16, 2024

View reviewed changes

ChenyuLInx added 2 commits April 16, 2024 14:59

clean up

46edee7

nits

e4c52b1

ChenyuLInx requested a review from QMalcolm April 16, 2024 22:00

nits

e82c2c5

QMalcolm approved these changes Apr 16, 2024

View reviewed changes

ChenyuLInx added 3 commits April 16, 2024 15:39

nits

3cdd4fb

nits

bccee25

nits

661f45c

ChenyuLInx added the artifact_minor_upgrade To bypass the CI check by confirming that the change is not breaking label Apr 16, 2024

ChenyuLInx merged commit a70024f into main Apr 16, 2024
65 of 66 checks passed

ChenyuLInx deleted the cl/semantic-manifest-unittest branch April 16, 2024 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize fixtures and implement a happy path test for semantic_manifest #9930

Reorganize fixtures and implement a happy path test for semantic_manifest #9930

ChenyuLInx commented Apr 12, 2024

github-actions bot commented Apr 12, 2024

codecov bot commented Apr 12, 2024 •

edited

Loading

aranke Apr 15, 2024 •

edited

Loading

ChenyuLInx Apr 15, 2024

QMalcolm Apr 16, 2024 •

edited

Loading

gshank left a comment

QMalcolm left a comment

QMalcolm Apr 16, 2024

QMalcolm Apr 16, 2024

QMalcolm Apr 16, 2024

QMalcolm Apr 16, 2024

QMalcolm Apr 16, 2024 •

edited

Loading

ChenyuLInx Apr 16, 2024

QMalcolm Apr 16, 2024

QMalcolm Apr 16, 2024

QMalcolm Apr 16, 2024

ChenyuLInx commented Apr 16, 2024

QMalcolm left a comment

QMalcolm commented Apr 16, 2024

		# Request fixtures for manifest, this is simialr to import * in the file.
		pytest_plugins = ("tests.unit.utils.manifest",)

Reorganize fixtures and implement a happy path test for semantic_manifest #9930

Reorganize fixtures and implement a happy path test for semantic_manifest #9930

Conversation

ChenyuLInx commented Apr 12, 2024

Problem

Solution

Checklist

github-actions bot commented Apr 12, 2024

codecov bot commented Apr 12, 2024 • edited Loading

Codecov Report

aranke Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QMalcolm Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

gshank left a comment

Choose a reason for hiding this comment

QMalcolm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QMalcolm Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx commented Apr 16, 2024

QMalcolm left a comment

Choose a reason for hiding this comment

QMalcolm commented Apr 16, 2024

codecov bot commented Apr 12, 2024 •

edited

Loading

aranke Apr 15, 2024 •

edited

Loading

QMalcolm Apr 16, 2024 •

edited

Loading

QMalcolm Apr 16, 2024 •

edited

Loading