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

[CT-281] Make obsolete the test compare tables code now in core/dbt/tests/tables.py #4778

Closed
gshank opened this issue Feb 24, 2022 · 5 comments · Fixed by #4986
Closed

[CT-281] Make obsolete the test compare tables code now in core/dbt/tests/tables.py #4778

gshank opened this issue Feb 24, 2022 · 5 comments · Fixed by #4986
Labels
adapter_plugins Issues relating to third-party adapter plugins repo ci/cd Testing and continuous integration for dbt-core + adapter plugins tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality

Comments

@gshank
Copy link
Contributor

gshank commented Feb 24, 2022

As part of the conversion to switching from unittest to pytest, the code in test/integration/base.py that implements the five table comparison assertions was pulled into a separate file, core/dbt/tests/tables.py. There are better ways to do this, including using macros and data tests.

Implement the table comparisons in a more dbt-like way, update the new pytest tests to use it, and remove dbt.tests.tables.

@github-actions github-actions bot changed the title Make obsolete the test compare tables code now in core/dbt/tests/tables.py [CT-281] Make obsolete the test compare tables code now in core/dbt/tests/tables.py Feb 24, 2022
@jtcohen6 jtcohen6 added repo ci/cd Testing and continuous integration for dbt-core + adapter plugins adapter_plugins Issues relating to third-party adapter plugins tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality labels Feb 25, 2022
@jtcohen6
Copy link
Contributor

In particular, there's a lot of overlap between what's in tables.py, and what's been implemented to power the (relatively newer) suite of basic adapter tests here. Where possible, we should opt for:

  • defining this functionality on the adapter
  • wrapping all SQL in Jinja macros

Other current sources of jankiness, worth a second pass:

@nathaniel-may
Copy link
Contributor

work items:

  • identify duplicate functionality in table.py vs macros + adapter methods
  • convert to using macro and adapter methods where possible
  • identify and create missing macro and adapter methods
  • update existing tests if necessary

BigQuery will be a bit of a challenge

@gshank
Copy link
Contributor Author

gshank commented Apr 1, 2022

Comment in wrong pr.

@nathaniel-may
Copy link
Contributor

@gshank, my comment from estimation? where does it belong?

@gshank
Copy link
Contributor Author

gshank commented Apr 11, 2022

Oh, you're asking about my "Comment in wrong pr." comment? That was my mistake. I put a comment here that belonged in a different PR, but github wouldn't let me remove all the text, so I just put that in... Had nothing to do with your comment :)

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

Successfully merging a pull request may close this issue.

3 participants