add base class for merge exclude tests #6700
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. |
There was a problem hiding this comment.
hey I have given all of these PRs a read-through, and they look awesome to me 🤩
yo, is there a single description that can be used across all the body: description_here portions of the changie entries so that all the changelogs will read uniformly? This is just a nit. A related nit is that I think these "family of PRs" are most readable when they all have identical PR names. Not a big deal, but giving you my best by presenting it as an option.
anyway, this is great and glad it's all working! I'll defer to anyone else that wants to review these. Otherwise, I'll be happy to go ahead and do formal review for these.
jtcohen6
added
Team:Adapters
tests/adapter/dbt/tests/adapter/incremental/test_incremental_merge_exclude_columns.py
Show resolved
Hide resolved
tests/adapter/dbt/tests/adapter/incremental/test_incremental_merge_exclude_columns.py
Show resolved
Hide resolved
| seed_count = len(run_dbt(["seed", "--select", seed, "--full-refresh"])) | ||
|
|
||
| model_count = len(run_dbt(["run", "--select", incremental_model, "--full-refresh"])) | ||
| # pass on kwarg |
There was a problem hiding this comment.
What's the keyword argument here exactly? I just see renaming a variable name.
There was a problem hiding this comment.
believe this code was copied from elsewhere, the comments may not be perfectly applicable
There was a problem hiding this comment.
specifically, took Matt's work from in test_incremental_unique_id
tests/adapter/dbt/tests/adapter/incremental/test_incremental_merge_exclude_columns.py
Show resolved
Hide resolved
| project.adapter, [expected_fields.relation, test_case_fields.relation] | ||
| ) | ||
|
|
||
| def get_expected_fields(self, relation, seed_rows, opt_model_count=None): |
There was a problem hiding this comment.
nit: I'd probably make this a class object or a constant and not use a method to obscure it. All that this really does it hide the instantiation, but we don't expect these values to change. I don't see an advantage to this factory-function.
There was a problem hiding this comment.
Hey Dave, this PR is merge-able on a functional level -- thanks a bundle!
But, if you're willing to indulge us, we're making a big push to make our adapter-first test code updated in terms of design/framework use.
If you want to chat with me on some of these design considerations, do reach out and I'm happy to pair up.
tests/adapter/dbt/tests/adapter/incremental/test_incremental_merge_exclude_columns.py
Outdated
Show resolved
Hide resolved
resolves #6699
Description
merge_exclude_columnsadapter testsRelated PRs:
Checklist
changie newto create a changelog entry