Add test Jinja tag
#3261
Merged
Add test Jinja tag
#3261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kwigley
requested review from
a team,
nathaniel-may and
iknox-fa
and removed request for
a team
kwigley
commented
Apr 13, 2021
| @@ -250,9 +250,6 @@ def _inject_ctes_into_sql(self, sql: str, ctes: List[InjectedCTE]) -> str: | |||
|
|
|||
| return str(parsed) | |||
|
|
|||
| def _get_dbt_test_name(self) -> str: | |||
There was a problem hiding this comment.
Wee, since we don't prepend tests with count(*) when "compiling" this can go away!
iknox-fa
approved these changes
Apr 13, 2021
iknox-fa
reviewed
Apr 13, 2021
core/dbt/utils.py
Outdated
| def get_test_macro_name(test_name, with_prefix=True): | ||
| name = f'test_{test_name}' | ||
|
|
||
| if with_prefix: |
iknox-fa
approved these changes
Apr 13, 2021
jtcohen6
approved these changes
Apr 13, 2021
There was a problem hiding this comment.
This is backwards compatible in a very clever way, and I had to spend a few minutes thinking about it to really get it. Nice work!
A few things worth calling out, either because I think we should eventually document them, or because it reorients my own thinking:
- Test blocks are still technically macro nodes, just like materializations. As long as that's true, some of our other work around "configuring" generic tests (e.g. from
dbt_project.yml) will have to work a bit differently. I actually think that's ok! - A test node named
my_testcannot coexist with a macro namedtest_my_test. I suppose the same is true of a materialization namedmy_mtznand a macro namedmaterialization_my_mtzn. - If users are commonly calling other macros from within a test block, it's more likely that they will encounter the v0.19.1 regression (
config()block orrefnot properly accounted for), which @gshank is fixing by sticking the top-level test macro into the capture context. Once more, I think that's ok: We can tell users they need to hoist ref/source/config within the main test block to achieve the functionality they're after.
gshank
approved these changes
Apr 13, 2021
kwigley
force-pushed
the
feature/test-jinja-block
branch
2 times, most recently
from
04ba591
to
a651a69
Compare
kwigley
force-pushed
the
feature/test-jinja-block
branch
from
a651a69
to
3860d91
Compare
nathaniel-may
approved these changes
Apr 16, 2021
This was referenced Oct 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
addresses part of #1173
Description
This PR adds a
testJinja tag. This gives us the ability to treat tests differently than very specifically defined macros. There is no change in behavior with this PR.Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.