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

New test configs #3392

Merged
merged 11 commits into from
Jun 2, 2021
Merged

New test configs #3392

merged 11 commits into from
Jun 2, 2021

Conversation

kwigley
Copy link
Contributor

@kwigley kwigley commented May 26, 2021

reopened from #3336! (thanks @jtcohen6 )

resolves #3258, #3321

Description

tests:
  +warn_if: ">10"
  +error_if: ">100"
  +where: "date_col = current_date"
  +limit: 10
  +fail_calc: "count(*)"

Adds four five new test configs:

  • limit: Simple enough, templated out in the materialization. This is mostly a complement for dbt test --store-failures #3316
  • where: The way I did this was unbelievably hacky—see the test cases for yourself—but it works, and in a way that should be backwards compatible with all existing schema/generic tests, without them needing to change any part of their SQL definition. (This config doesn't make sense for one-off tests.)
  • warn_if, error_if: The user supplies a python-evaluable string (e.g. >=3, <5, ==0, !=0) and dbt will compare the fail count/calc against it.
    • By default, set to !=0
    • Interaction with severity: A little tricky, but I actually think they work reasonably together. By default, severity: error, and dbt checks the error_if condition first; if not error, then check the warn_if condition; if the result meets neither, it passes the test. If the user sets severity: warn, dbt will skip over the error_if condition entirely and jump straight to warn_if.
  • fail_calc: User-supplied fail_calc for tests #3321

Possible follow up work

  • Cleanup: Could we wrap the where logic in a special internal macro, so long as it's included in the schema test parsing context?

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
    • Need to add some integration tests, non-blocking for review
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@kwigley kwigley requested review from gshank and jtcohen6 May 26, 2021 12:45
@cla-bot cla-bot bot added the cla:yes label May 26, 2021
@kwigley kwigley requested a review from iknox-fa May 26, 2021 12:45
@kwigley kwigley self-assigned this May 26, 2021
@gshank
Copy link
Contributor

gshank commented May 26, 2021

Git is terminally confused by this branch. Somehow a commit of mine (b952b70) seems to have gotten into this. I tried rebasing on develop because I like to look at a squashed commit to see all the changes and git couldn't do it. Could you make a clean branch and force push it?

@kwigley kwigley force-pushed the feature/net-new-test-configs branch from 7c136b6 to 9e876cd Compare May 26, 2021 13:40
@kwigley
Copy link
Contributor Author

kwigley commented May 26, 2021

@gshank good call! Looks like I totally messed up a rebase at some point, should be in good shape now!

Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

LGTM!

node.config['error_if'] = builder.error_if()
if builder.fail_calc() is not None:
node.unrendered_config['fail_calc'] = builder.fail_calc()
node.config['fail_calc'] = builder.fail_calc()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to see this in the builder as a method that just returns the configs. But we can leave that for later.

@kwigley kwigley force-pushed the feature/net-new-test-configs branch from 7da8c9f to 1ed23b2 Compare May 27, 2021 13:04
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

lgtm!

Happy to merge #3316 before or after, in either case there will be some conflicts.

@iknox-fa
Copy link
Contributor

A few super minor details.. otherwise LGTM!

Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

👍

@kwigley kwigley force-pushed the feature/net-new-test-configs branch 2 times, most recently from cae0941 to 4157d02 Compare June 1, 2021 12:53
@kwigley kwigley force-pushed the feature/net-new-test-configs branch from 4157d02 to 2adeb76 Compare June 1, 2021 12:54
@kwigley kwigley force-pushed the feature/net-new-test-configs branch from 2adeb76 to 86637c8 Compare June 1, 2021 17:16
@kwigley kwigley merged commit 2c40530 into develop Jun 2, 2021
@kwigley kwigley deleted the feature/net-new-test-configs branch June 2, 2021 14:28
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
* New test configs: where, limit, warn_if, error_if

* update test task and tests

* fix flake8

* Update core/dbt/parser/schemas.py

Co-authored-by: Jeremy Cohen <[email protected]>

* Update core/dbt/parser/schema_test_builders.py

Co-authored-by: Jeremy Cohen <[email protected]>

* respond to some feedback

* add failures field

* add failures to results

* more feedback

* add some test coverage

* dev review feedback

Co-authored-by: Jeremy Cohen <[email protected]>

automatic commit by git-black, original commits:
  2c40530
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
* New test configs: where, limit, warn_if, error_if

* update test task and tests

* fix flake8

* Update core/dbt/parser/schemas.py

Co-authored-by: Jeremy Cohen <[email protected]>

* Update core/dbt/parser/schema_test_builders.py

Co-authored-by: Jeremy Cohen <[email protected]>

* respond to some feedback

* add failures field

* add failures to results

* more feedback

* add some test coverage

* dev review feedback

Co-authored-by: Jeremy Cohen <[email protected]>

automatic commit by git-black, original commits:
  2388330
  2c40530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Net-new test configs
4 participants