-
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
Convert docs_generate_tests to new framework #5058
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. |
I am going to split this test into two parts, a 'docs_generate' part in the adapter zone, and an 'artifacts' tests in dbt-core standard tests. |
Is the docs generate part going to be mainly related to catalog generate? |
Yes, I'll remove the manifest and run_results varification from the adapter zone test, and remove the catalog verification from the artifacts tests. In the artifacts tests I will try to switch to compile instead of 'docs generate". |
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.
This looks great! Love that adapter one only have catalog related
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.
This looks good! Left two questions for question about thinking process, not blocker.
@@ -120,6 +120,11 @@ def dbt_profile_target(): | |||
} | |||
|
|||
|
|||
@pytest.fixture(scope="class") | |||
def profile_user(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.
Which adapter need this change and why?
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.
We need a way to get the "owner" in the catalog. It's different for different adapters. Some of them use the "user" in the profile, some use other things. So this is special fixture to help with that. We didn't have a standard way to get the profile's user before.
"""Any string. Use this in assert calls""" | ||
|
||
def __eq__(self, other): | ||
return isinstance(other, str) |
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 three classes here looks very similar, why we are doing it as 3 individual instead of having one that you can maybe pass in a specific type? Also, think we can achieve AnyString
by not passing in contains for AnyStringWith
?
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.
They're separate classes because they were copied from existing classes in the integration test suite. You do have a point that a single class that takes a type would work here.
resolves #5035
Description
Convert the 029_docs_generate_tests to new pytest framework.
Pull request opened for initial review, but will also be splitting up these tests into adapter zone and non-adapter-zone tests.
Checklist