Support unit testing incremental models #8891

MichelleArk · 2023-10-25T00:40:03Z

resolves #8422

Problem

Unit testing incremental models requires support for both:

overriding the is_incremental macro to return True or False (depending on which scenario is being tested) and provide (already done)
the ability to specify an input fixture for {{ this }} in order to mock out the previous state of the tested incremental model (this PR!)

Solution

accept this as a given input yml name, in addition to ref and source calls
storing the model node of on a UnitTestNode when this is provided for reference downstream in UnitTestContext.this
overriding this in UnitTestContext to add an ephemeral prefix to the unit test sql if this is specified

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
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

github-actions · 2023-10-25T00:40:23Z

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 · 2023-10-25T00:42:38Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (df4e4ed) 86.47% compared to head (8d028cb) 86.63%.
Report is 11 commits behind head on unit_testing_feature_branch.

Additional details and impacted files

@@                       Coverage Diff                       @@
##           unit_testing_feature_branch    #8891      +/-   ##
===============================================================
+ Coverage                        86.47%   86.63%   +0.15%     
===============================================================
  Files                              179      181       +2     
  Lines                            26836    26976     +140     
===============================================================
+ Hits                             23207    23371     +164     
+ Misses                            3629     3605      -24

Flag	Coverage Δ
integration	`83.51% <79.48%> (+0.19%)`	⬆️
unit	`64.65% <25.64%> (+0.18%)`	⬆️

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

Files	Coverage Δ
core/dbt/context/providers.py	`89.48% <100.00%> (+0.09%)`	⬆️
core/dbt/contracts/graph/nodes.py	`93.85% <100.00%> (-0.49%)`	⬇️
core/dbt/exceptions.py	`68.97% <50.00%> (-0.06%)`	⬇️
core/dbt/parser/unit_tests.py	`93.51% <76.92%> (-1.58%)`	⬇️

... and 16 files with indirect coverage changes

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

gshank

Overall I think this is very clever. I'm not convinced that using "this" as the name in several places is the best choice though. Not only do the two places differ from each other, they're not the same as the original "this"...

gshank · 2023-11-01T16:13:54Z

core/dbt/context/providers.py

@@ -1541,6 +1541,14 @@ def env_var(self, var: str, default: Optional[str] = None) -> str:
        else:
            return super().env_var(var, default)

+    @contextproperty()
+    def this(self) -> Optional[str]:


Elsewhere "this" is a Relation. Here it's just a model name plus the ephemeral_prefix. Might be confusing. Maybe call it something else?

I don't think we can we can call it anything else in the provider specifically, because need the {{ this }} call in the jinja context to resolve to the CTE name of the fixture we create from this provided in the unit test yaml.

Ah. Okay :)

gshank · 2023-11-01T16:14:04Z

core/dbt/contracts/graph/nodes.py

@@ -1068,6 +1068,7 @@ class UnitTestNode(CompiledNode):
    attached_node: Optional[str] = None
    overrides: Optional[UnitTestOverrides] = None
    config: UnitTestNodeConfig = field(default_factory=UnitTestNodeConfig)
+    this: Optional[ModelNode] = None


I'm wondering if using "this" in this way might not be confusing. I think "this" in the rest of the context is actually a Relation, right? What do you think of calling it "original_input_node"?

Hmm... this is a little different than the original input node - its the actual fixture node as specified in the unit test yaml and inserted in the unit test manifest.

attached_node above (should maybe be renamed to tested_node_unique_id(I can go ahead and rename that here) already represents that but this (no pun intended) is actually different.

I'm going to test something out to see if they can be used interchangeably though..

I've updated this to explicitly include this_input_node_unique_id and updated attached_node to be tested_node_unique_id: https://github.com/dbt-labs/dbt-core/pull/8891/files#diff-42cfc99b2fcd89ab75957b8999bee2b1af822f7d39953ca99f1f285c170557c1R1068-R1069

the provider.this implementation needs to do a lookup now but the benefit is that we don't store the whole node on the UnitTestNode which may want to make its way to a serialized artifact and lead to bloat

gshank

Looks good!

* Initial implementation of unit testing (from pr #2911) Co-authored-by: Michelle Ark <[email protected]> * 8295 unit testing artifacts (#8477) * unit test config: tags & meta (#8565) * Add additional functional test for unit testing selection, artifacts, etc (#8639) * Enable inline csv format in unit testing (#8743) * Support unit testing incremental models (#8891) * update unit test key: unit -> unit-tests (#8988) * convert to use unit test name at top level key (#8966) * csv file fixtures (#9044) * Unit test support for `state:modified` and `--defer` (#9032) Co-authored-by: Michelle Ark <[email protected]> * Allow use of sources as unit testing inputs (#9059) * Use daff for diff formatting in unit testing (#8984) * Fix #8652: Use seed file from disk for unit testing if rows not specified in YAML config (#9064) Co-authored-by: Michelle Ark <[email protected]> Fix #8652: Use seed value if rows not specified * Move unit testing to test and build commands (#9108) * Enable unit testing in non-root packages (#9184) * convert test to data_test (#9201) * Make fixtures files full-fledged members of manifest and enable partial parsing (#9225) * In build command run unit tests before models (#9273) --------- Co-authored-by: Michelle Ark <[email protected]> Co-authored-by: Michelle Ark <[email protected]> Co-authored-by: Emily Rockman <[email protected]> Co-authored-by: Jeremy Cohen <[email protected]> Co-authored-by: Kshitij Aranke <[email protected]>

first pass

6232097

cla-bot bot added the cla:yes label Oct 25, 2023

MichelleArk changed the title ~~first pass~~ Support unit testing incremental models Oct 25, 2023

changelog entry

96bbbfd

MichelleArk marked this pull request as ready for review November 1, 2023 15:19

MichelleArk requested a review from a team as a code owner November 1, 2023 15:19

MichelleArk requested review from peterallenwebb and gshank November 1, 2023 15:19

gshank approved these changes Nov 1, 2023

View reviewed changes

MichelleArk added 3 commits November 1, 2023 16:56

remove this from UnitTestNode, rename attached_node to tested_node

adf9f2a

store this_input_node_unique_id explicitly on UnitTestNode

470f977

improve readability in UnitTestContext.this

8d028cb

gshank approved these changes Nov 1, 2023

View reviewed changes

MichelleArk merged commit aa91ea4 into unit_testing_feature_branch Nov 2, 2023
49 checks passed

MichelleArk deleted the unit-testing-for-incremental-models branch November 2, 2023 01:08

aranke mentioned this pull request Nov 13, 2023

Fix #8652: Use seed file from disk for unit testing if rows not specified in YAML config #9064

Merged

5 tasks

dbeatty10 mentioned this pull request Oct 29, 2024

[Feature] Enable unit testing for models that use adapter.get_relation #10891

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support unit testing incremental models #8891

Support unit testing incremental models #8891

MichelleArk commented Oct 25, 2023 •

edited by dbeatty10

Loading

github-actions bot commented Oct 25, 2023

codecov bot commented Oct 25, 2023 •

edited

Loading

gshank left a comment

gshank Nov 1, 2023

MichelleArk Nov 1, 2023

gshank Nov 1, 2023

gshank Nov 1, 2023

MichelleArk Nov 1, 2023

MichelleArk Nov 1, 2023

MichelleArk Nov 1, 2023

MichelleArk Nov 1, 2023 •

edited

Loading

gshank left a comment

Support unit testing incremental models #8891

Support unit testing incremental models #8891

Conversation

MichelleArk commented Oct 25, 2023 • edited by dbeatty10 Loading

Problem

Solution

Checklist

github-actions bot commented Oct 25, 2023

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

gshank left a comment

Choose a reason for hiding this comment

gshank Nov 1, 2023

Choose a reason for hiding this comment

MichelleArk Nov 1, 2023

Choose a reason for hiding this comment

gshank Nov 1, 2023

Choose a reason for hiding this comment

gshank Nov 1, 2023

Choose a reason for hiding this comment

MichelleArk Nov 1, 2023

Choose a reason for hiding this comment

MichelleArk Nov 1, 2023

Choose a reason for hiding this comment

MichelleArk Nov 1, 2023

Choose a reason for hiding this comment

MichelleArk Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

gshank left a comment

Choose a reason for hiding this comment

MichelleArk commented Oct 25, 2023 •

edited by dbeatty10

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

MichelleArk Nov 1, 2023 •

edited

Loading