-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
mv on_schema_change
tests -> "adapter zone"
#6618
Conversation
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. |
abd8d00
to
6716836
Compare
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.
LGTM!
# Tests | ||
# | ||
|
||
_TESTS__SELECT_FROM_INCREMENTAL_IGNORE = """ |
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.
Clearly from my more naive days of test conversions :p
That said, I do recall thinking "should we keep these?" I know they live in the code history and don't necessary need to exist in current code. More a "principles" question: if there are seemingly extraneously models in tests, do we nix them with cautious abandon or seek to add tests to include them? Of course, we merged this without getting much of answer at the time, and I'd love to hear your thoughts if you have any now. 🙇♀️
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.
As a general rule: If it's not being actively used in one of our existing tests, we probably don't need it. (There are rare exceptions, where we've added a new fixture/combination with the intent of testing, but completely forgot! I just found an instance where I'd made this mistake a ~while ago: dbt-labs/dbt-bigquery#41)
Longer answer: Intent is a useful input, to the extent we can infer it. In this case, I remember the history as being: our colleague Matt W added this feature (on_schema_change
) back in 2021, and he added integration tests for it by copying + adapting some other recent integration tests (for --store-failures
). The "tests" component was important for the original tests, but not necessary for these adapted ones.
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.
Appreciate the history lesson! That makes much more sense and I'll run with your heuristic in the future.
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.
Barring the merge conflicts, and my lil' comment, looks good.
Thanks for introducing me to that neat dbt.type_string()
.
6716836
to
096e520
Compare
* Mv incremental on_schema_change tests to 'adapter zone' * Use type_string() * Cleanup
* Mv incremental on_schema_change tests to 'adapter zone' * Use type_string() * Cleanup
* mv `on_schema_change` tests -> "adapter zone" (#6618) * Mv incremental on_schema_change tests to 'adapter zone' * Use type_string() * Cleanup * mv `on_schema_change` tests -> "adapter zone" (#6618) * Mv incremental on_schema_change tests to 'adapter zone' * Use type_string() * Cleanup Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
follow-up to #6330
Description
In the previous conversion PR, @VersusFacit made a reasonable point:
As it stands, that these tests have already been living in those other repos, and they were originally written (in #3387) to be cross-adapter compatible. Then, they were copy-pasted everywhere, as part of the Great Adapter Split, in fall 2021 / the lead-up to v1.0.
By moving this test into the "adapter zone" (a.k.a.
dbt-tests-adapter
), we can inherit it for use in all our adapter repos, and delete a LOT of copy-pasted code.While here:
string_type
withdbt.type_string()
(added in v1.2!)dbt-bigquery
), without having to run existing test functions multiple timesdbt-tests-adapter
(which is v1 distributed software!)Adapter PRs
Checklist
I have runchangie new
to create a changelog entry