-
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
Initial pass at switching integration tests to pytest #4691
Conversation
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 is really exciting!! Lots of comments as I read through the draft implementation.
As next steps, I'd be excited to:
- Try porting the two converted integration tests to an adapter plugin, and see how we'd need to reimplement fixtures/methods/etc
- Think about what other modular utilities we'd want, in a similar vein to
get_manifest
. I think @JCZuurmond's work onpytest-dbt-core
could be generative here, especiallymacro_generator
to mock macro input + output. Could that do for "execution" code whatget_manifest
does for "parsing" code?
'advanced_incremental.sql': advanced_incremental_sql, | ||
'compound_sort.sql': compound_sort_sql, | ||
'disabled.sql': disabled_sql, | ||
'empty.sql': empty_sql, | ||
'get_and_ref.sql': get_and_ref_sql, | ||
'incremental.sql': incremental_sql, | ||
'interleaved_sort.sql': interleaved_sort_sql, | ||
'materialized.sql': materialized_sql, | ||
'schema.yml': schema_yml, | ||
'view_model.sql': view_model_sql, |
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.
Should we aim to consolidate these fixtures with the ones used in other tests? It would require more upfront work, but if we assembled a common set of standard fixtures (rather than recreating highly similar ones over and over again), then:
- each test can pick & choose just the ones it needs
- adapter plugins could reimplement specific fixtures (as needed) among the standard reused set, and then be able to get most tests "for free," rather than needing to reimplement similar fixtures in similar ways for each test
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.
Ideally I think it would be nice to have some standard projects that can be reused. We have a lot of tests that create projects with only subtle or minimal differences. That can be done a directory basis, using a test directory conftest.py, with only the different parts overridden. We probably could do that for test_simple_copy.py and test_copy_upperbase, and just override the 'models' fixture for test_copy_upperbase. You can also make individual sets of project fixtures with different names and include them in a test as a plugin.
I think it will be easiest to do that after we've converted the tests though, since we'll then be able to more easily see the differences between test projects. The legacy tests with all the files in directories (often multiple) makes that sort of comparison harder.
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.
Though on second thought, if people see the same setup as they're going through converting, there's no reason to wait. "Oh, I saw this project before. Yank! It's now a plugin."
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.
Hi, super nice you are upgrading the testing framework! I added some comments, I am still thinking about how I would do this myself, view the comments as suggestions, probably some education is needed before I understand the intent of the changes.
It is great to remove all the dbt
subprocess calls as this allows the user to start debugging sessions.
Furthermore, it would be great to have a test suite which can be applied to the core
and all the adapters
. Especially, if it is written in such a way that adapters can easily extended it (for example with adapter specific settings) whilst reusing the test suite.
Pytest recommends two test layouts: outside or inside the package. The most common set-up is having your tests outside of your package: this makes your package smaller. However, some packages don't do this (for example numpy
).
A benefit of having the tests inside your package is that you can run the test suite as a user. This could be helpful for the adapters:
- Install
dbt-core
:pip install dbt-core
- Run
dbt-core
test suite:pytest --pyargs dbt
If then the test suite is set-up to automatically pick up which adapter is installed, you can reuse that test suite for every adapter.
As mentioned in the comments, I don't think fixtures are usually shipped inside the package. If you want to reuse a fixture over different projects, I think the idea is to write a pytest plugin.
2c054b2
to
f552cfa
Compare
63c460b
to
6e48fbf
Compare
3dcea97
to
fc4adda
Compare
0a3987f
to
30b5707
Compare
Add comments, cleanup adapter schema setup
51aad7c
to
e72db2b
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 with the understanding that we'll make changes as needed as we learn more/establish patterns.
resolves #4690
Description
This pull request is for preliminary review of the state of the integration test conversion to pytest.
Checklist
CHANGELOG.md
and added information about my change