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

Refactor use_profile #3193

Closed
JCZuurmond opened this issue Mar 24, 2021 · 6 comments
Closed

Refactor use_profile #3193

JCZuurmond opened this issue Mar 24, 2021 · 6 comments
Labels
repo ci/cd Testing and continuous integration for dbt-core + adapter plugins stale Issues that have gone stale tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality

Comments

@JCZuurmond
Copy link
Contributor

JCZuurmond commented Mar 24, 2021

During my work on pr #3176 (@jtcohen6 and @kwigley) I broke my head over the use_profile decorator. As I understand it does two things:

  1. Set a pytest mark
  2. Test if the test name contains one of the profiles as defined here. This happens in the _profile_from_test_name

IMHO both are unexpected, especially if you are new to dbt. What is the goal of this decorator?

My assumption is that we intend to do two things:

  1. Limit the allowed markers. Pytest has functionality to do this in the pytest.ini.
  2. Restrict naming functions -> if you have the mark profile_postgres you must have postgres in your name. Why do we want this? I think this is not needed, if we want it we could write a test for this.

I think this would allow us to replace use_profile(<profile name>) with @pytest.mark.profile_<profile name>, which is easier to understand

@jtcohen6
Copy link
Contributor

@JCZuurmond Thanks for opening this, and sharing your thoughts! I appreciate the time you've spent diving into dbt's testing patterns, and the subsequent befuddlement.

Today, dbt integration tests use the @use_profile decorator to specify which databases/adapters a given test should actually run against, and we invoke the test using marks like profile_postgres and profile_snowflake. It sounds like you have a lot of prior experience using pytest, and would prefer if dbt did less special "wrapping" around builtin pytest constructs, and instead exposed them more explicitly. Does that sound right?

I think dbt contributors tend to be a mix of folks with plenty of prior python experience, prior OSS contributing experience, and none of either. In abstract, I like the idea of being less "special," where possible, so that folks can benefit from the vast trove of python/pytest documentation online.

@JCZuurmond
Copy link
Contributor Author

Yes, you said it perfectly. I think we - the dbt project - would benefit from less special wrapping around builtin pytest as it is easier for new contributors to understand what is happening.

I myself was bent over the "random" assert error, I did not easily make the link between the use_profile decorator and my test name (not having the mentioned profile).

@github-actions
Copy link
Contributor

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please remove the stale label or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues that have gone stale label Oct 13, 2021
@jtcohen6 jtcohen6 added tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality and removed stale Issues that have gone stale labels Oct 13, 2021
@jtcohen6
Copy link
Contributor

This one still feels compelling to me. We should aim to rework our integration testing framework so that it is much less "special," and much more accessible to community contributors. Using more pytest builtins would be a reasonable way there.

Alternative proposition: use_profile is a lot less critical in a world where we've split out our adapter plugins.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2022

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please remove the stale label or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the stale Issues that have gone stale label Jul 5, 2022
@jtcohen6 jtcohen6 added repo ci/cd Testing and continuous integration for dbt-core + adapter plugins and removed Repo_testing labels Jul 12, 2022
@github-actions
Copy link
Contributor

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest; add a comment to notify the maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repo ci/cd Testing and continuous integration for dbt-core + adapter plugins stale Issues that have gone stale tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Projects
None yet
Development

No branches or pull requests

3 participants