Skip to content
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

Port testing framework changes. #70

Merged
merged 3 commits into from
Apr 1, 2022
Merged

Conversation

ueshin
Copy link
Collaborator

@ueshin ueshin commented Mar 31, 2022

Description

Port testing framework changes from dbt-labs/dbt-spark#299 and dbt-labs/dbt-spark#314.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 1, 2022

@ueshin You beat me to it! I was planning to open an issue for the testing framework migration in the dbt-databricks repo today :)

Draft docs (dbt-labs/docs.getdbt.com#1263) for using the new framework: https://deploy-preview-1263--docs-getdbt-com.netlify.app/docs/contributing/testing-a-new-adapter

We've also got work in progress (dbt-labs/dbt-core#4958) to convert and consolidate the test case that you ported in #62, so that you can inherit + run these test cases with a lot less copy-pasted code.

@ueshin
Copy link
Collaborator Author

ueshin commented Apr 1, 2022

@jtcohen6 Yes, we are keeping an eye on dbt-spark changes so that we don't loose the compatibility. :)

We've also got work in progress (dbt-labs/dbt-core#4958) to convert and consolidate the test case that you ported in #62, so that you can inherit + run these test cases with a lot less copy-pasted code.

That's really good to know! Looking forward to it being merged.

@ueshin ueshin marked this pull request as ready for review April 1, 2022 18:23
@@ -29,7 +29,7 @@ deps =

[testenv:integration-databricks-cluster]
basepython = python3
commands = /bin/bash -c '{envpython} -m pytest -v tests/specs/databricks-cluster.dbtspec {posargs}; ret=$?; [ $ret = 5 ] && exit 0 || exit $ret'
commands = /bin/bash -c '{envpython} -m pytest -v --profile databricks_cluster tests/functional/adapter/test_basic.py {posargs}; ret=$?; [ $ret = 5 ] && exit 0 || exit $ret'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the --profile option! I am curious if we can change the integration tests to use this too.

Copy link
Collaborator Author

@ueshin ueshin Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can, in a separate PR, and/or we can contribute to dbt-spark if they want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or dbt-labs/dbt-core#4958 will provide the testing framework even for custom integration tests?

Copy link
Contributor

@jtcohen6 jtcohen6 Apr 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ueshin Correct, our thinking is that this framework will provide the foundation for:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as the --profile option in particular: It looks like this is something we added in dbt-spark (https://github.com/dbt-labs/dbt-spark/blob/main/tests/conftest.py), rather than the test utilities built into dbt-core (since the tests there only use a single Postgres profile). @gshank Do you see merit in standardizing this for all plugin maintainers, by moving pytest_addoption and dbt_profile_target into dbt-core? Or must they be defined in each plugin's own conftest.py?

@ueshin
Copy link
Collaborator Author

ueshin commented Apr 1, 2022

Thanks! merging.

@ueshin ueshin merged commit 5d44dc6 into databricks:main Apr 1, 2022
@ueshin ueshin deleted the new_test_framework branch April 1, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants