Experiment with functional testing #577
Conversation
|
If you run tests with -s you should get a line that lists the temporary project directory, like: === Test project_root: /private/var/folders/qt/vw8wqdgx4w381wh14b9y25m40000gn/T/pytest-of-gerda/pytest-487/project0. You can use that look at the project directory. We were also thinking about a pytest custom option to copy that somewhere, but so far haven't needed it. We have a number of tests that look at things in the target directory. If you grep for "get_artifact" and "get_manifest" you'll find some of them. |
There was a problem hiding this comment.
Thanks for the diligent work here! And thanks especially for calling out what's odd & left out. I think you managed to get something workable in just about all cases.
The three types of cross-database macros which you've called out as being most difficult to test, are also the ones that might want to be adapter methods (Python), IMO:
_is_relation,_is_ephemeral: These should absolutely become Python context methods—not evenadaptermethods, just good old standard base/provider context stuff. They'll be able to perform real Pythonisinstance, instead of the nonsense they have to do right now, checking side-effect class attributes of the available object.type_*macros: These feel duplicative with theColumnobject methods (overridable by adapter), and the adapterconvert_*_typemacros, which are used during type inference while loading seeds. These feel like the right places to define them. If we can find a reasonable way to use them as one-offs, I bet we can plumb this logic back intodbt_utils(for backwards compatibility) from theColumnobject or those adapter methods. They're not perfectly tested today, but I think it better to test them "closer to the metal," by hooking into the adapter's Python client (if available/supported). E.g. we useColumn.convert_typehere to translate between BigQuery's schema object and dbt's Column objects.current_timestamp: I'm on the fence about this one, and how best to test it. There's already a macro for this (snapshot_get_time) and an (unused, untested) adapter methoddate_function. Let's consolidate them together?current_timestamp_in_utcis a bit trickier, and I definitely see the value. I'm actually surprised we don't have aconvert_timezonemacro here — the snowplow package does.
The trade-offs with all of these: If these are defined as Python methods, users can still use them, but they can't override them, and the source code is less accessible / trickier to find. That said, the whole initiative here with cross-database macros is recognizing there exists some unopinionated low-level functionality that is best left to the adapter plugin maintainer, not the package / project maintainer.
Not mentioned in our issues / PRs so far, but worth calling out in a similar vein:
There's tremendous overlap between:
- the
equality,cardinality_equality, andequal_rowcountcustom generic tests indbt-utils - the theurgical query we use to power the
check_relations_equalutility in the New Core Testing Framework
This code does more visible work, so I think it's good to have it visible, rather than hidden away in adapter code. But I also think it's tremendously valuable to end users (unit testing SQL transformations!), and we should aim to consolidate between the logic that's living in both places right now. Right now, all the testing framework logic is Python-only methods and f-stringified SQL, inaccessible to user-space code. See: recent slack thread with Marius from Trino/Starburst
| } | ||
|
|
||
|
|
||
| @pytest.mark.skip_profile("bigquery", reason="TODO - need to figure out timestamp vs. datetime behavior!") |
| @@ -0,0 +1,26 @@ | |||
|
|
|||
| # escape_single_quotes | |||
There was a problem hiding this comment.
I like the way you've factored this one. Ideally, we'd have a single fixture that works across the board. The fact that it's hard to do, justifies the existence of the cross-db macro :)
I think it might make sense to define two "variants" of this test. Those could be two different test cases:
models__test_escape_single_quotes_quote→BaseEscapeSingleQuotesQuotemodels__test_escape_single_quotes_backslash→BaseEscapeSingleQuotesBackslash
Or a single test case with a new fixture, which subclasses can override:
@pytest.fixture(scope="class")
def escape_character():
return "quote" # or "backslash"
@pytest.fixture(scope="class")
def models():
if self.escape_character() == "quote":
return ...
elif self.escape_character() == "backslash":
return ...
else: # pseudo code
raise "I don't know that one"Then, each adapter (including Postgres/Redshift/Snowflake/BigQuery) can opt into one of the two standard variants with no changes:
@pytest.mark.only_profile("postgres")
class TestEscapeSingleQuotesPostgres(BaseEscapeSingleQuotesQuote):
pass
@pytest.mark.only_profile("snowflake")
class TestEscapeSingleQuotesSnowflake(BaseEscapeSingleQuotesBackslash):
passOr:
@pytest.mark.only_profile("postgres")
class TestEscapeSingleQuotesPostgres(BaseEscapeSingleQuotes):
pass
@pytest.mark.only_profile("snowflake")
class TestEscapeSingleQuotesBigQuery(BaseEscapeSingleQuotes):
@pytest.fixture(scope="class")
def escape_character():
return "backslash"Of course, if neither of those standard variants work, an adapter can define its own fixture and override the test case from the ground up.
There was a problem hiding this comment.
Decision
I chose the "two different test cases" option. The implementation came out very straightforward.
This allowed me to bypass reasoning about the case of an unknown escape_character value. 😅
There was a problem hiding this comment.
The alternative
However, the single test case with the escape_character definition was really interesting. I wonder if it would be worth adding the escape_character definition within each adapter itself?
An idea
Create a collection of low-level definitions within each adapter:
literal_quote- the symbol used to demarcate a (string) literal value within the database.- ANSI standard is single quote.
- We'd then use this within the default implementation of the
string_literalmacro.
standard_escape_character- The symbol used to interpret a character literally rather than interpret it.- The ANSI standard is the backslash character.
invalid_for_standard_escape- List of symbols that can't be escaped by thestandard_escape_character.- The ANSI standard includes both single and double quotes.
- We'd default this to the
literal_quotevalue (and maybe double quote too).
special_escape_character- The symbol used to escapeinvalid_for_standard_escapecharacters.- The ANSI standard is a single quote (same as
literal_quote). - Use within default implementation of the
escape_single_quotesmacro.
- The ANSI standard is a single quote (same as
These low-level configurations would then inform the intermediate-level macros like the following:
string_literalescape_single_quotes(maybe rename this to something else more abstract)
References
There was a problem hiding this comment.
Sidenote
Some macros assume single quotes to demarcate string literals rather than utilizing string_literal. This is one example. Not sure how common this is or not.
| class TestExcept(BaseExcept): | ||
| def test_build_assert_equal(self, project): |
There was a problem hiding this comment.
This test is cleverly written!
Any reason to have test_build_assert_equal defined on TestExcept, rather than on BaseExcept and then
class TestExcept(BaseExcept):
pass| } | ||
|
|
||
|
|
||
| class TestIntersect(BaseIntersect): |
There was a problem hiding this comment.
Same comment here as for TestExcept — let's move test_build_assert_equal into BaseIntersect, I think
| # TODO how can we test this better? | ||
| models__test_current_timestamp_sql = """ | ||
| select | ||
| {{ dbt_utils.current_timestamp() }} as actual, | ||
| {{ dbt_utils.current_timestamp() }} as expected | ||
| """ |
There was a problem hiding this comment.
Hm. Juice might not be worth the squeeze on this one.
We could use Python/Jinja {{ datetime.datetime.now() }}, and perform a comparison that shaves off seconds? To be clear, I think this is a bad idea:
models__test_current_timestamp_sql = """
select
left(cast({{ dbt_utils.current_timestamp() }} as text), 16) as actual,
'{{ modules.datetime.datetime.now().strftime("%Y-%m-%d %H:%M") }}' as expected
"""Worth calling out that dbt_utils.current_timestamp is actually duplicative with:
- The adapter classmethod
date_now, which is required for all adapters but actually unused / untested (lol): (docs, abstractmethod, Postgres implementation) snapshot_get_time, which is indeed a macro (not method) — not tested directly, but well tested insofar as snapshots are well tested, including their "right now" behavior (the tests for which can be flakey)
This was very helpful -- exactly what I needed. Thank you @gshank ! 🏆 Was able to see that directory as you described when I ran a single test module locally: python3 -m pytest tests/functional/cross_db_utils/test_dateadd.py --profile bigquery -s |
|
@jtcohen6 I think I've responded to all your crucial feedback, so requested another review. Do you see any remaining barriers we should overcome before merging this? Feedback not incorporated hereYou surfaced a few things that we should split out into new issues for either dbt-utils or dbt-core, namely:
|
|
No blockers from me! Let's move forward! I see you managed to get the So, on the whole — it sounds like we didn't actually have to make any breaking changes to these macros, which means that we don't first need a Next stepsI think next up is lift & shift. I think this is the order of operations:
Related work itemsThanks for catching all those other items! Let's open up some issues to keep track of this work. These issues are interrelated, so I'm taking best guesses at where they should live, and which units of work can be pursued independently. To be clear: Not saying you need to go create all these issues. Also not saying you can't, if you feel inspired! New
|
Continuing the work from #575.
This is a:
Checklist
dateaddtest is skipped currently)Description & motivation
Followed the provided pattern for functional testing for all cross-database macros except as noted below.
Questions
targetfolder created by each test?Details
Was mostly able to follow the assert_equal actual/expected pattern for test comparisons via YAML tests. Exceptions noted below.
Created
@pytest.mark.only_profileto support different implementations forescape_single_quotes. Is there a better way to enable different implementations for each adapter?Tests that are not yet implemented are marked as
@pytest.mark.skip_profile.Other tests not yet implemented contain the text
TODOsomewhere within the code (so it can be grep'ed easily).Included
These were a little funky, complicated, or different:
Not included
We'll need to decide how we want to approach testing for these macros
These are either not going to be moved, will be deprecated, or merely haven't been fully discussed yet:
To do
type_*,_is_*, andcurrent_timestamp*macrosdateaddtest to work for BigQueryReflections
DUALtable).from_dualmacro that returns an empty string in the default case. This would allow us to create datasets on the fly in a cross-database manner using CTE for the purposes of testing.check_relations_equalmight rely on the existence ofEXCEPTwithin the database. Once we have theexceptmacro moved from dbt-utils to dbt-core, we might want to consider dispatchingEXCEPTinstead (i.e., to useMINUSwhen applicable for databases like Oracle and MySQL).Approaches for testing cross-db macros
It seems there are 3 types of uses for cross-db macros:
concatanddate_trunc.intersectandexcept.string_literalandcast_bool_to_text.A way to test each:
assert_equalactual/expected pattern for test comparisons via YAML tests.test_build_assert_equal) and utilizecheck_relations_equalto compare the actual/expected relations.union allto separate each case. See discussion regardingfrom_dual. Useassert_equallike the standard case.