-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Support unit testing incremental models #8891
Changes from 2 commits
6232097
96bbbfd
adf9f2a
470f977
8d028cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Features | ||
body: Support unit testing incremental models | ||
time: 2023-11-01T10:18:45.341781-04:00 | ||
custom: | ||
Author: michelleark | ||
Issue: "8422" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to test something out to see if they can be used interchangeably though.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this to explicitly include 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 |
||
|
||
|
||
@dataclass | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere "this" is a Relation. Here it's just a model name plus the ephemeral_prefix. Might be confusing. Maybe call it something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromthis
provided in the unit test yaml.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Okay :)