fix: extract test owner from primary model only (CORE-196) #905
Conversation
Previously, the flatten_test macro collected owners from ALL parent models that a test references. This was incorrect for multi-parent tests like relationship tests, where only the primary model's owner should be attributed. Changes: - Remove owner collection from the parent models loop - Add owner extraction from the primary model inside the tested_model_node block - Keep tag collection from all parent models unchanged This ensures that: - Single parent tests: Owner attribution unchanged (backward compatible) - Multi-parent tests: Only primary model's owner shown - Relationship tests: Only the 'tested' model's owner attributed - Override scenarios: Respect override_primary_test_model_id configuration Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
WalkthroughOwner collection for dbt tests now extracts owners only from the primary/tested model node (determined by Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
macros/edr/dbt_artifacts/upload_dbt_tests.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: integration_tests/tests/dbt_project.py:191-201
Timestamp: 2025-08-10T11:29:19.004Z
Learning: In the Elementary dbt package integration tests, BigQuery works correctly with the default `("database", "schema")` property mapping in the `get_database_and_schema_properties` function. When using `target.database` and `target.schema` in source definitions, BigQuery's dbt adapter handles these references appropriately without requiring special mapping to `project` and `dataset`.
📚 Learning: 2025-09-04T09:14:40.621Z
Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 825
File: macros/edr/tests/test_dimension_anomalies.sql:73-75
Timestamp: 2025-09-04T09:14:40.621Z
Learning: In the dbt-data-reliability codebase, `flatten_model` is a valid macro defined in `macros/edr/dbt_artifacts/upload_dbt_models.sql` and can be used as a parameter to `elementary.get_anomaly_query()` even though other test files use the `flattened_test` variable instead.
Applied to files:
macros/edr/dbt_artifacts/upload_dbt_tests.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (latest_official, dremio) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (latest_official, bigquery) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (latest_official, snowflake) / test
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (latest_official, postgres) / test
- GitHub Check: test (latest_official, redshift) / test
- GitHub Check: test (latest_official, clickhouse) / test
🔇 Additional comments (1)
macros/edr/dbt_artifacts/upload_dbt_tests.sql (1)
84-91: LGTM – Tag collection preserved, owner collection correctly removed.The loop now only collects tags from all parent models, which aligns with the PR objective. Owner collection has been appropriately removed from this section and moved to the primary model extraction block.
Address CodeRabbit review comment: restore the unique filter for test_models_owners to ensure no duplicate owners appear in model_owners, maintaining backward compatibility with the original behavior. Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (5)
44-44: Remove unusedtmp_pathparameter.The
tmp_pathfixture is declared but unused in this test function. The test usesdbt_project.models_dir_pathfor file operations instead.-def test_single_parent_test_owner_attribution(dbt_project: DbtProject, tmp_path): +def test_single_parent_test_owner_attribution(dbt_project: DbtProject):
100-102: Remove unusedtmp_pathparameter.Same issue as the previous test—
tmp_pathis unused.def test_relationship_test_uses_primary_model_owner_only( - dbt_project: DbtProject, tmp_path + dbt_project: DbtProject, ):
192-192: Remove unusedtmp_pathparameter.-def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject, tmp_path): +def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject):
275-275: Remove unusedtmp_pathparameter.-def test_owner_deduplication(dbt_project: DbtProject, tmp_path): +def test_owner_deduplication(dbt_project: DbtProject):
320-328: Consider simplifying with set comparison.The current assertions work correctly but could be consolidated using set comparison for clarity:
- assert ( - len(model_owners) == 2 - ), f"Expected 2 unique owners, got {len(model_owners)}: {model_owners}" - assert ( - "Alice" in model_owners - ), f"Expected 'Alice' in model_owners, got {model_owners}" - assert ( - "Bob" in model_owners - ), f"Expected 'Bob' in model_owners, got {model_owners}" + assert set(model_owners) == { + "Alice", + "Bob", + }, f"Expected {{'Alice', 'Bob'}}, got {model_owners}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: integration_tests/tests/dbt_project.py:191-201
Timestamp: 2025-08-10T11:29:19.004Z
Learning: In the Elementary dbt package integration tests, BigQuery works correctly with the default `("database", "schema")` property mapping in the `get_database_and_schema_properties` function. When using `target.database` and `target.schema` in source definitions, BigQuery's dbt adapter handles these references appropriately without requiring special mapping to `project` and `dataset`.
🧬 Code graph analysis (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
integration_tests/tests/conftest.py (1)
dbt_project(144-147)integration_tests/tests/dbt_project.py (3)
DbtProject(46-334)write_yaml(325-334)read_table(89-107)
🪛 Ruff (0.14.8)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
44-44: Unused function argument: tmp_path
(ARG001)
101-101: Unused function argument: tmp_path
(ARG001)
192-192: Unused function argument: tmp_path
(ARG001)
275-275: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (latest_official, snowflake) / test
- GitHub Check: test (latest_official, redshift) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (latest_official, dremio) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (latest_official, postgres) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (latest_official, clickhouse) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (latest_official, bigquery) / test
🔇 Additional comments (5)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (5)
14-21: LGTM!The
cleanup_filecontext manager correctly ensures file cleanup after the test block completes, even if an exception occurs.
24-41: LGTM!The helper robustly handles the various formats that
model_ownersmight be stored as across different database adapters (JSON string, native list, None).
49-97: LGTM!Solid baseline test that verifies single-parent test owner attribution. Good use of unique identifiers to prevent test collisions and clear assertion messages.
103-189: LGTM!This is the key test for CORE-196. It correctly validates that relationship tests only inherit the owner from the primary model (
"Alice"), not from the referenced model ("Bob"). The assertion message is particularly helpful in explaining the expected behavior.
193-273: LGTM!Important edge case that validates the primary model's owner is used—when the primary model has no owner, the result should be empty rather than inheriting from the referenced model.
Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (5)
24-41: Consider clarifying line 38 behavior for non-list JSON.The function correctly handles the expected formats (None, list, JSON array string). However, if JSON parsing succeeds but yields a non-list (e.g.,
{"key": "value"}), line 38 returns[model_owners_value](the original string) rather than handling the parsed value explicitly. While this defensive behavior is acceptable for a test helper, you might consider making this edge case more explicit:try: parsed = json.loads(model_owners_value) - return parsed if isinstance(parsed, list) else [model_owners_value] + if isinstance(parsed, list): + return parsed + # Unexpected JSON structure; treat as single owner string + return [model_owners_value] except json.JSONDecodeError:This makes the fallback intent clearer without changing behavior.
44-96: Remove unusedtmp_pathparameter.The test logic correctly validates single-parent owner attribution. However, the
tmp_pathparameter is declared but never used.Apply this diff:
-def test_single_parent_test_owner_attribution(dbt_project: DbtProject, tmp_path): +def test_single_parent_test_owner_attribution(dbt_project: DbtProject):Otherwise, the test is well-structured with clear assertions and proper cleanup.
99-191: Remove unusedtmp_pathparameter.This is the key test for CORE-196 and correctly validates that relationship tests use only the primary model's owner. The test setup, assertion, and error messages clearly demonstrate the expected behavior.
Apply this diff:
@pytest.mark.skip_targets(["dremio"]) def test_relationship_test_uses_primary_model_owner_only( - dbt_project: DbtProject, tmp_path + dbt_project: DbtProject ):
193-277: Remove unusedtmp_pathparameter.This test correctly validates the edge case where the primary model has no owner and ensures that the referenced model's owner is not inherited. This is important coverage for the CORE-196 requirements.
Apply this diff:
@pytest.mark.skip_targets(["dremio"]) -def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject, tmp_path): +def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject):
279-332: Remove unusedtmp_pathparameter.This test correctly validates owner deduplication from comma-separated owner strings. The assertions appropriately check both the count and membership without assuming a specific order.
Apply this diff:
-def test_owner_deduplication(dbt_project: DbtProject, tmp_path): +def test_owner_deduplication(dbt_project: DbtProject):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: integration_tests/tests/dbt_project.py:191-201
Timestamp: 2025-08-10T11:29:19.004Z
Learning: In the Elementary dbt package integration tests, BigQuery works correctly with the default `("database", "schema")` property mapping in the `get_database_and_schema_properties` function. When using `target.database` and `target.schema` in source definitions, BigQuery's dbt adapter handles these references appropriately without requiring special mapping to `project` and `dataset`.
🧬 Code graph analysis (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
integration_tests/tests/conftest.py (1)
dbt_project(144-147)integration_tests/tests/dbt_project.py (3)
DbtProject(46-334)write_yaml(325-334)read_table(89-107)
🪛 Ruff (0.14.8)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
44-44: Unused function argument: tmp_path
(ARG001)
101-101: Unused function argument: tmp_path
(ARG001)
194-194: Unused function argument: tmp_path
(ARG001)
279-279: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (latest_official, postgres) / test
- GitHub Check: test (latest_official, clickhouse) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (latest_official, bigquery) / test
- GitHub Check: test (latest_official, snowflake) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (latest_official, redshift) / test
- GitHub Check: test (latest_official, dremio) / test
🔇 Additional comments (2)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
1-12: LGTM! Clear module documentation and appropriate imports.The docstring provides excellent context for the test suite's purpose, and all imports are relevant to the test implementation.
14-21: LGTM! Clean context manager pattern.The
cleanup_filehelper correctly ensures test files are removed after each test, complementing the existingwrite_yamlcleanup for SQL model files.
Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (4)
44-97: Remove unused parameter.The
tmp_pathparameter is declared but never used in the function body. The test usesdbt_project.models_dir_pathdirectly instead.Apply this diff:
-def test_single_parent_test_owner_attribution(dbt_project: DbtProject, tmp_path): +def test_single_parent_test_owner_attribution(dbt_project: DbtProject):
99-204: Excellent test coverage for the core requirement.This test correctly validates the key CORE-196 behavior: relationship tests between models with different owners should only include the primary model's owner, not the referenced model's owner. The defensive filtering logic (lines 187-193) appropriately handles variations across dbt versions.
However, the
tmp_pathparameter is unused.Apply this diff to remove the unused parameter:
@pytest.mark.skip_targets(["dremio"]) def test_relationship_test_uses_primary_model_owner_only( - dbt_project: DbtProject, tmp_path + dbt_project: DbtProject ):
206-303: Good edge case coverage.This test correctly validates that when the primary model has no owner, the test should have empty
model_ownerseven when the referenced model has an owner. This ensures referenced model owners are never inherited.However, the
tmp_pathparameter is unused.Apply this diff to remove the unused parameter:
@pytest.mark.skip_targets(["dremio"]) -def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject, tmp_path): +def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject):
305-358: Effective deduplication test.This test validates that duplicate owners in comma-separated values are properly deduplicated. The assertion logic correctly verifies exactly 2 unique owners are present.
However, the
tmp_pathparameter is unused.Apply this diff to remove the unused parameter:
-def test_owner_deduplication(dbt_project: DbtProject, tmp_path): +def test_owner_deduplication(dbt_project: DbtProject):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: integration_tests/tests/dbt_project.py:191-201
Timestamp: 2025-08-10T11:29:19.004Z
Learning: In the Elementary dbt package integration tests, BigQuery works correctly with the default `("database", "schema")` property mapping in the `get_database_and_schema_properties` function. When using `target.database` and `target.schema` in source definitions, BigQuery's dbt adapter handles these references appropriately without requiring special mapping to `project` and `dataset`.
🧬 Code graph analysis (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
integration_tests/tests/conftest.py (1)
dbt_project(144-147)integration_tests/tests/dbt_project.py (3)
DbtProject(46-334)write_yaml(325-334)read_table(89-107)
🪛 Ruff (0.14.8)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
44-44: Unused function argument: tmp_path
(ARG001)
101-101: Unused function argument: tmp_path
(ARG001)
207-207: Unused function argument: tmp_path
(ARG001)
305-305: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (latest_official, redshift) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (latest_official, clickhouse) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (latest_official, postgres) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (latest_official, dremio) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (latest_official, snowflake) / test
- GitHub Check: test (latest_official, bigquery) / test
🔇 Additional comments (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (1)
1-22: LGTM: Clean module setup and helper.The module docstring clearly states the purpose, imports are appropriate, and the
cleanup_filecontext manager correctly handles file cleanup with proper existence checks.
Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (1)
24-42: Fix the JSON parsing logic.When
json.loads()successfully parses a non-list value (e.g., a JSON string"Alice"), line 38 returns[model_owners_value](the original JSON-encoded string) instead of[parsed](the decoded value). This causes JSON-encoded strings to retain their quotes.Example:
- Input:
'"Alice"'json.loads('"Alice"')→'Alice'- Current: returns
['"Alice"'](with quotes)- Expected: returns
['Alice'](decoded)Apply this diff:
try: parsed = json.loads(model_owners_value) - return parsed if isinstance(parsed, list) else [model_owners_value] + return parsed if isinstance(parsed, list) else [parsed] except json.JSONDecodeError:
🧹 Nitpick comments (4)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (4)
44-97: Consider removing unusedtmp_pathparameter.The
tmp_pathparameter is declared but never used in the function body. If it's not required by pytest fixtures, consider removing it.Apply this diff if the parameter is not required:
-def test_single_parent_test_owner_attribution(dbt_project: DbtProject, tmp_path): +def test_single_parent_test_owner_attribution(dbt_project: DbtProject):
99-195: Excellent validation of CORE-196, but consider removing unused parameter.The test logic correctly validates that relationship tests only use the primary model's owner (Alice) and not the referenced model's owner (Bob). The explicit test name approach and clear assertion messages make this test robust.
However, the
tmp_pathparameter is unused.Apply this diff to remove the unused parameter if not required:
@pytest.mark.skip_targets(["dremio"]) def test_relationship_test_uses_primary_model_owner_only( - dbt_project: DbtProject, tmp_path + dbt_project: DbtProject ):
197-285: Good edge case coverage, but consider removing unused parameter.The test correctly validates that when the primary model has no owner, the test doesn't inherit from the referenced model (Bob). This edge case is important for CORE-196.
However, the
tmp_pathparameter is unused.Apply this diff to remove the unused parameter if not required:
@pytest.mark.skip_targets(["dremio"]) -def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject, tmp_path): +def test_relationship_test_no_owner_on_primary_model(dbt_project: DbtProject):
287-340: Solid deduplication test, but consider removing unused parameter.The test correctly validates that duplicate owners are deduplicated (Alice,Bob,Alice → 2 unique owners). The order-agnostic assertions are appropriate for this use case.
However, the
tmp_pathparameter is unused.Apply this diff to remove the unused parameter if not required:
-def test_owner_deduplication(dbt_project: DbtProject, tmp_path): +def test_owner_deduplication(dbt_project: DbtProject):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T11:29:19.004Z
Learnt from: GuyEshdat
Repo: elementary-data/dbt-data-reliability PR: 838
File: integration_tests/tests/dbt_project.py:191-201
Timestamp: 2025-08-10T11:29:19.004Z
Learning: In the Elementary dbt package integration tests, BigQuery works correctly with the default `("database", "schema")` property mapping in the `get_database_and_schema_properties` function. When using `target.database` and `target.schema` in source definitions, BigQuery's dbt adapter handles these references appropriately without requiring special mapping to `project` and `dataset`.
Applied to files:
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
🧬 Code graph analysis (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
integration_tests/tests/conftest.py (1)
dbt_project(144-147)integration_tests/tests/dbt_project.py (3)
DbtProject(46-334)write_yaml(325-334)read_table(89-107)
🪛 Ruff (0.14.8)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
44-44: Unused function argument: tmp_path
(ARG001)
101-101: Unused function argument: tmp_path
(ARG001)
198-198: Unused function argument: tmp_path
(ARG001)
287-287: Unused function argument: tmp_path
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (latest_official, redshift) / test
- GitHub Check: test (latest_official, dremio) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (latest_official, bigquery) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (latest_official, postgres) / test
- GitHub Check: test (latest_official, clickhouse) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (latest_official, snowflake) / test
🔇 Additional comments (2)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
1-12: LGTM!The module docstring clearly explains the purpose and references CORE-196, and all imports are appropriate for the integration tests.
14-22: LGTM!The cleanup_file context manager is correctly implemented with proper try/finally pattern and existence check before unlinking.
- Fix JSON parsing logic in _parse_model_owners to return [parsed] instead of [model_owners_value] when json.loads succeeds with a non-list value - Remove unused tmp_path parameter from all 4 test functions Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
|
Addressed the CodeRabbit review comments in commit 2b638fa:
|
Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (1)
48-66: LGTM! Consider escaping owner values.The utility function successfully reduces duplication across tests. Type hints have been properly added.
One minor consideration: if an owner value ever contains single quotes (e.g.,
"Alice's Team"), the f-string interpolation at line 61 could produce invalid Jinja syntax. However, since this is a test utility with controlled inputs (current tests only use simple names like "Alice" and "Bob"), this is unlikely to be an issue in practice.🔎 Optional: Add escaping for special characters
If you want to make the utility more robust against special characters, you could use
json.dumpsto safely escape the owner value:+import json + def _create_model_sql(owner: Optional[str] = None, columns: str = "1 as id") -> str: if owner is not None: + escaped_owner = json.dumps(owner) return f""" - {{{{ config(meta={{'owner': '{owner}'}}) }}}} + {{{{ config(meta={{'owner': {escaped_owner}}}) }}}} select {columns} """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (2)
integration_tests/tests/conftest.py (1)
dbt_project(144-147)integration_tests/tests/dbt_project.py (3)
DbtProject(46-334)write_yaml(325-334)read_table(89-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (latest_official, clickhouse) / test
- GitHub Check: test (latest_official, dremio) / test
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (latest_official, postgres) / test
- GitHub Check: test (latest_official, bigquery) / test
- GitHub Check: test (latest_official, snowflake) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (latest_official, redshift) / test
🔇 Additional comments (7)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (7)
1-13: LGTM!The imports and module docstring are clear and well-organized. The docstring effectively explains the purpose of these tests in relation to CORE-196.
16-23: LGTM!The context manager is properly implemented with correct type hints and defensive checks before file deletion.
26-45: LGTM!The JSON parsing logic is now correct. Line 42 properly returns
[parsed]instead of[model_owners_value], ensuring that decoded values are used without JSON quotes. Type hints have been added as requested in past reviews.
69-114: LGTM!This baseline test is well-structured and correctly validates that a single-parent test inherits the owner from its model. The test properly:
- Uses unique identifiers to avoid conflicts
- Cleans up resources with context managers
- Provides clear assertion messages
- Uses the
_create_model_sqlutility to reduce duplication
117-209: LGTM!This is the key test validating CORE-196's core objective. The test correctly verifies that relationship tests only extract the owner from the primary model (Alice), not from the referenced model (Bob). Excellent practices:
- Explicit test name for robust querying across dbt versions
- Clear assertion message explaining expected behavior
- Proper skip marker for dremio target
- Comprehensive cleanup of resources
212-295: LGTM!This test validates the important edge case where the primary model has no owner but the referenced model does. It correctly verifies that ownership is not inherited from the referenced model, confirming the "primary model only" behavior introduced by CORE-196.
298-348: LGTM!This test correctly validates the deduplication behavior mentioned in the PR objectives. The test uses order-independent assertions (checking length and membership rather than exact list equality), which is a robust approach that won't break if the macro changes the order of deduplicated owners.
…differs) Co-Authored-By: Yosef Arbiv <yosef.arbiv@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (1)
300-350: Verify comma-separated owner parsing behavior.The test passes
"Alice,Bob,Alice"as a single string to theownerparameter, which embeds it into the dbt config asmeta={'owner': 'Alice,Bob,Alice'}. The test then expects this to be parsed into separate owners and deduplicated.Please confirm that the dbt artifacts system is designed to split comma-separated values in the
meta.ownerfield. This is non-standard for dbt meta fields, which typically use lists in YAML.Optional: Consider set-based assertion for clarity
The current assertions check length and membership separately. A set-based assertion might be slightly clearer:
assert set(model_owners) == {"Alice", "Bob"}, \ f"Expected unique owners {{'Alice', 'Bob'}}, got {set(model_owners)}"This is optional—the current approach is perfectly valid and explicit.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (latest_official, trino) / test
- GitHub Check: test (latest_official, dremio) / test
- GitHub Check: test (latest_pre, postgres) / test
- GitHub Check: test (fusion, redshift) / test
- GitHub Check: test (fusion, bigquery) / test
- GitHub Check: test (fusion, databricks_catalog) / test
- GitHub Check: test (fusion, snowflake) / test
- GitHub Check: test (latest_official, databricks_catalog) / test
- GitHub Check: test (latest_official, redshift) / test
- GitHub Check: test (latest_official, athena) / test
- GitHub Check: test (latest_official, bigquery) / test
- GitHub Check: test (latest_official, snowflake) / test
- GitHub Check: test (latest_official, clickhouse) / test
- GitHub Check: test (latest_official, postgres) / test
🔇 Additional comments (4)
integration_tests/tests/test_dbt_artifacts/test_test_owners.py (4)
16-66: LGTM! Helper functions are well-implemented.The helper functions properly address past review comments:
- Type hints added to all function arguments and return types
_create_model_sqlutility created to reduce code duplication- JSON parsing bug fixed (line 42 correctly returns
[parsed])The implementations are clean, well-documented, and handle edge cases appropriately.
69-114: LGTM! Single parent test is well-structured.The test properly validates the baseline case where a test on a single model inherits the owner from that model. Good use of the
_create_model_sqlutility and proper cleanup handling.
117-210: LGTM! Core CORE-196 test validates primary model owner behavior.This test correctly validates that relationship tests only use the owner from the primary model (the one being tested), not from referenced models. The explicit test naming strategy and detailed assertion messages make this test robust and maintainable.
213-297: LGTM! Edge case test validates no owner inheritance from referenced models.This test properly validates that when the primary model has no owner, the test's
model_ownersremains empty even if the referenced model has an owner. This confirms owners don't leak from non-primary models.
Summary
Fixes test ownership attribution to only collect owners from the primary model being tested, rather than aggregating owners from ALL parent models that a test references.
Before: A relationship test between
model_a(owner: Alice) andmodel_b(owner: Bob) would show both owners.After: The same test only shows the primary model's owner (the model specified in the test's
modelparameter).Changes
if tested_model_node:block, using the same parsing pattern fromflatten_model| unique | listdeduplication filter to ensure no duplicate owners appear inmodel_ownerstest_models_ownersremains emptyUpdates since last revision
test_test_owners.pycovering:| unique | listfix)_parse_model_owners: now returns[parsed]instead of[model_owners_value]whenjson.loads()succeeds with a non-list valuetmp_pathparameter from all 4 test functionscleanup_file,_parse_model_owners) and test functions_create_model_sqlutility function to reduce code duplicationargumentsformat for relationship test YAML definitions@pytest.mark.skip_for_dbt_fusionto relationship tests (fusion handles primary model detection differently, causing emptymodel_owners)Review & Testing Checklist for Human
tested_model_nodeonly, not from alltest_model_nodes. Confirm this matches the intended CORE-196 behavior.latest_officialcoverage is sufficient.dbt_tests.model_owners.Notes
test_schema_changeson fusion (pre-existing, unrelated). Alllatest_officialwarehouse tests pass.Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.