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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Apr 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b96cdcf
Reorganize fixtures and implement a happy path test for semantic_mani…
ChenyuLInx Apr 12, 2024
55f73ae
actual add the file, Pycharm was new to me
ChenyuLInx Apr 12, 2024
46712cc
move location
ChenyuLInx Apr 16, 2024
46edee7
clean up
ChenyuLInx Apr 16, 2024
e4c52b1
nits
ChenyuLInx Apr 16, 2024
e82c2c5
nits
ChenyuLInx Apr 16, 2024
3cdd4fb
nits
ChenyuLInx Apr 16, 2024
bccee25
nits
ChenyuLInx Apr 16, 2024
661f45c
nits
ChenyuLInx Apr 16, 2024
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
2 changes: 1 addition & 1 deletion core/dbt/artifacts/resources/v1/semantic_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@aranke aranke Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@QMalcolm QMalcolm Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's used in a couple places in Metricflow:

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

relation_name: Optional[str] = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@QMalcolm QMalcolm Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha yes, maybe this does deserve a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.



# ====================================
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀



@pytest.fixture
def basic_parsed_source_definition_object():
Expand Down
Empty file added tests/unit/contracts/__init__.py
View file
Open in desktop
Empty file.
Empty file added tests/unit/contracts/graph/__init__.py
View file
Open in desktop
Empty file.
32 changes: 32 additions & 0 deletions tests/unit/contracts/graph/test_semantic_manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import pytest
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",)
Copy link
Contributor

Choose a reason for hiding this comment

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

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


# Overwrite the default nods to construct the manifest


@pytest.fixture
def nodes(metricflow_time_spine_model):
return [metricflow_time_spine_model]


@pytest.fixture
def semantic_models(
semantic_model,
) -> list:
return [semantic_model]


@pytest.fixture
def metrics(
metric,
) -> list:
return [metric]


class TestSemanticManifest:
def test_validate(self, manifest):
sm_manifest = SemanticManifest(manifest)
assert sm_manifest.validate()
Loading
Loading