-
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
Set up adapter testing framework for use by adapter test repos #4846
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool! I took a quick look, and left a few quick comments, including small nits around (integration) test names, given that (dbt) tests have been renamed.
There's an important question we need to answer: Which tests should land in this framework?
- Goal: These "basic" tests should be the initial proof of capability for a new dbt adapter plugin. If it passes these tests, it has successfully extended the lowest common denominator of dbt behavior.
- What's missing? Currently, these tests don't include newer features, such as
persist_docs
, that require some implementation across each database. It also doesn't include standard permutations of incremental models, such asunique_key
(Test for incremental model with uniqueness constraint dbt-adapter-tests#15) +on_schema_change
configurations. I think that's ok, as long as we most those tests available via easy "opt in." - Many of the tests that are landing in
tests/functional
have also been copy-pasted intodbt-redshift
,dbt-snowflake
,dbt-bigquery
. I think it will be critical to make those inheritable for adapters, so we can remove the duplicated code in those repos, without cluttering up the "basic" adapter testing suite.
c59b12c
to
48b07d3
Compare
Just FYI, I've decided not to use git-subrepo for now. It's not necessary for internal use, and we can make a decision on how to make the adapter test plugin available later. |
The integration test seems failing |
4d762c3
to
0c71b44
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.
Looks good! We can keep adjusting small changes if needed
@@ -63,6 +63,19 @@ def test_data_dir(request): | |||
return os.path.join(request.fspath.dirname, "data") | |||
|
|||
|
|||
@pytest.fixture(scope="class") | |||
def dbt_profile_target(): |
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 added profiles_config_update
to do similar stuff, do we want to consider merging these two functions?
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.
The profile_target is for just the important bits and is consistent with the way adapter tests ran on dbt-adapter-tests before. The profiles_config_update is for when you need to replace more than the "target" portion. So I think they serve different needs. There's overlap, certainly... If it turns out to be confusing or a headache we can revisit.
check_relation_rows(project, "ts_snapshot", 30) | ||
|
||
|
||
class TestSnapshotTimestamp(BaseSnapshotTimestamp): |
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.
Why all of the tests have a base class then inherent?
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.
Because otherwise when a test is inherited in an adapter test suite, pytest will run all the tests in the base class PLUS all of the tests in the subclass. We don't want that. So we need to have base class names that don't start with "Test", and inherit from them in classes that do start with "Test".
resolves #4730
Description
This moves the adapter tests to tests/adapter, set them up as a plugin installable at dbt.tests.adapter. It sets up the tests in tests/adapter/dbt/tests/adapter/basic so that they can be imported into adapter-specific test suites. It uses git-subrepo to push and pull tests from a new dbt-tests-adapter repository.
Checklist