Fix unit tests for incremental models with alias #10755

katsugeneration · 2024-09-22T15:33:56Z

Resolves #10754
Resolves #10763

Problem

Unit tests fail due to wrong input cte referrence name on incremental this models with alias
Because cte name is based on alias name on ephemeral model. This change on dbt-adapters 1.4.1 by Use model alias for the CTE identifier generated during ephemeral materialization dbt-adapters#236

Solution

Fix this referrence name in UnitTestContext to based on alias name on incremental model

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 or relevant for this PR.
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.

cla-bot · 2024-09-22T15:34:01Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @katsugeneration

katsugeneration · 2024-09-22T15:46:16Z

@cla-bot check

cla-bot · 2024-09-22T15:46:21Z

The cla-bot has been summoned, and re-checked this pull request!

codecov · 2024-09-23T12:10:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.98%. Comparing base (db69473) to head (79d650b).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10755      +/-   ##
==========================================
+ Coverage   88.97%   88.98%   +0.01%     
==========================================
  Files         181      181              
  Lines       22956    22977      +21     
==========================================
+ Hits        20424    20447      +23     
+ Misses       2532     2530       -2

Flag	Coverage Δ
integration	`86.16% <100.00%> (+0.02%)`	⬆️
unit	`62.39% <100.00%> (-0.06%)`	⬇️

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

Components	Coverage Δ
Unit Tests	`62.39% <100.00%> (-0.06%)`	⬇️
Integration Tests	`86.16% <100.00%> (+0.02%)`	⬆️

dbeatty10

Thanks for the targeted solution and the nice functional test @katsugeneration 🤩

I can confirm that running the following test fails without the fix, but succeeds with the test in place:

python -m pytest tests/functional/unit_testing/test_unit_testing.py::TestUnitTestIncrementalModelWithAlias```

…sion

dbeatty10 · 2024-09-23T21:07:33Z

tests/functional/unit_testing/test_unit_testing.py

+class TestUnitTestIncrementalModelWithAlias:
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {
+            "my_incremental_model.sql": my_incremental_model_with_alias_sql,
+            "events.sql": event_sql,
+            "schema.yml": test_my_model_incremental_yml_basic,
+        }
+
+    def test_basic(self, project):
+        results = run_dbt(["run"])
+        assert len(results) == 2
+
+        # Select by model name
+        results = run_dbt(["test", "--select", "my_incremental_model"], expect_pass=True)
+        assert len(results) == 2


This tests the regression reported in #10754.

I confirmed this test fails without the fix but succeeds with the fix in place:

python -m pytest tests/functional/unit_testing/test_unit_testing.py::TestUnitTestIncrementalModelWithAlias

dbeatty10 · 2024-09-23T21:07:54Z

tests/functional/unit_testing/test_unit_testing.py

+class TestUnitTestIncrementalModelWithVersion:
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {
+            "my_incremental_model.sql": my_incremental_model_sql,
+            "events.sql": event_sql,
+            "schema.yml": my_incremental_model_versioned_yml + test_my_model_incremental_yml_basic,
+        }
+
+    def test_basic(self, project):
+        results = run_dbt(["run"])
+        assert len(results) == 2
+
+        # Select by model name
+        results = run_dbt(["test", "--select", "my_incremental_model"], expect_pass=True)
+        assert len(results) == 2


This tests the regression reported in #10763.

I confirmed this test fails without the fix but succeeds with the fix in place:

python -m pytest tests/functional/unit_testing/test_unit_testing.py::TestUnitTestIncrementalModelWithVersion

dbeatty10 · 2024-09-24T15:52:25Z

core/dbt/context/providers.py

@@ -1636,7 +1636,7 @@ def this(self) -> Optional[str]:
        if self.model.this_input_node_unique_id:
            this_node = self.manifest.expect(self.model.this_input_node_unique_id)
            self.model.set_cte(this_node.unique_id, None)  # type: ignore
-            return self.adapter.Relation.add_ephemeral_prefix(this_node.name)
+            return self.adapter.Relation.add_ephemeral_prefix(this_node.identifier)  # type: ignore


The root causes of the regressions reported in #10754 and #10763 was that:

the update from this_node.name to this_node.identifier was accidentally overlooked during the implementation and code review of https://github.com/dbt-labs/dbt-core/pull/10290/files, and

there were no functional tests to catch those particular cases

This PR addresses both of those root causes ✅ ✅

(cherry picked from commit a8d4ba2)

(cherry picked from commit a8d4ba2) Co-authored-by: Katsuya Shimabukuro <katsu.generation.888@gmail.com> Co-authored-by: Gerda Shank <gerda@dbtlabs.com>

rmbborges · 2024-10-21T15:20:38Z

Hey @dbeatty10 , will this fix be included in 1.8.8?

katsugeneration added 3 commits September 22, 2024 13:24

Fix unittest incremental model with alias

67726a7

Add changie changelog

4ce3aa7

Change issue referrence

f4e46a7

katsugeneration requested a review from a team as a code owner September 22, 2024 15:33

github-actions bot added the community This PR is from a community member label Sep 22, 2024

cla-bot bot added the cla:yes label Sep 22, 2024

dbeatty10 mentioned this pull request Sep 23, 2024

[Regression] Unit tests generate an sql with wrong CTE this referrence on incremental model with alias #10754

Closed

2 tasks

dbeatty10 requested a review from MichelleArk September 23, 2024 13:47

dbeatty10 assigned MichelleArk and dbeatty10 Sep 23, 2024

dbeatty10 reviewed Sep 23, 2024

View reviewed changes

dbeatty10 mentioned this pull request Sep 23, 2024

[Regression] Incorrect CTE name in unit tests using incremental mode for versioned incremental models #10763

Closed

2 tasks

Add functional tests for unit testing an incremental model with a ver…

79d650b

…sion

dbeatty10 reviewed Sep 23, 2024

View reviewed changes

dbeatty10 reviewed Sep 24, 2024

View reviewed changes

MichelleArk approved these changes Sep 24, 2024

View reviewed changes

MichelleArk merged commit a8d4ba2 into dbt-labs:main Sep 24, 2024
60 checks passed

dbeatty10 mentioned this pull request Sep 24, 2024

[Bug] Bump the lower bound of dbt-adapters within 1.8.latest #10768

Open

2 tasks

dbeatty10 added the backport 1.8.latest label Sep 24, 2024

dbeatty10 pushed a commit that referenced this pull request Sep 24, 2024

Fix unit tests for incremental models with alias (#10755)

51455ca

(cherry picked from commit a8d4ba2)

dbeatty10 mentioned this pull request Sep 24, 2024

[Backport 1.8.latest] Fix unit tests for incremental models with alias #10769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unit tests for incremental models with alias #10755

Fix unit tests for incremental models with alias #10755

katsugeneration commented Sep 22, 2024 •

edited by dbeatty10

Loading

cla-bot bot commented Sep 22, 2024

katsugeneration commented Sep 22, 2024

cla-bot bot commented Sep 22, 2024

codecov bot commented Sep 23, 2024 •

edited

Loading

dbeatty10 left a comment

dbeatty10 Sep 23, 2024

dbeatty10 Sep 23, 2024

dbeatty10 Sep 24, 2024

rmbborges commented Oct 21, 2024

Fix unit tests for incremental models with alias #10755

Fix unit tests for incremental models with alias #10755

Conversation

katsugeneration commented Sep 22, 2024 • edited by dbeatty10 Loading

Problem

Solution

Checklist

cla-bot bot commented Sep 22, 2024

katsugeneration commented Sep 22, 2024

cla-bot bot commented Sep 22, 2024

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

dbeatty10 left a comment

Choose a reason for hiding this comment

dbeatty10 Sep 23, 2024

Choose a reason for hiding this comment

dbeatty10 Sep 23, 2024

Choose a reason for hiding this comment

dbeatty10 Sep 24, 2024

Choose a reason for hiding this comment

rmbborges commented Oct 21, 2024

katsugeneration commented Sep 22, 2024 •

edited by dbeatty10

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading