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: where, limit, warn_if, error_if, fail_calc #3336

Closed
wants to merge 12 commits into from

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented May 11, 2021

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. (I realize now this should probably be !=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

TODO

  • Add integration tests
  • Cleanup: Could we wrap the where logic in a special internal macro, so long as it's included in the schema test parsing context?
  • Cleanup: Any way to DRY up the manual-set configs within unique + not_null shortcut?

For the future

I got stuck trying to implement relative/percentage warn_if and error_if. That's ok! That piece, while compelling, can very easily come in a later issue.

  • If the user supplies error_if: >5%, it's easy enough to error_if.replace('%', '/100') and compare against >5/100 instead.
  • The issue was, I couldn't figure out how to grab the denominator, i.e. the count of the "full model" (ideally with the where condition included). At materialization time, we can access test_metadata.kwargs.model, but this is an unrendered Jinja string, and even with the as_native filter, I couldn't figure out how to "extra-render" it.

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
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label May 11, 2021
@jtcohen6 jtcohen6 mentioned this pull request May 17, 2021
@kwigley kwigley linked an issue May 19, 2021 that may be closed by this pull request
@kwigley kwigley force-pushed the feature/net-new-test-configs branch from 84bca9c to 86649b7 Compare May 19, 2021 21:48
Copy link
Contributor Author

@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.

This is looking really good! I haven't had the chance for an exhaustive review, so I just dropped some quick comments where things are standing out to me.

core/dbt/parser/schemas.py Outdated Show resolved Hide resolved
core/dbt/parser/schema_test_builders.py Outdated Show resolved Hide resolved
core/dbt/task/test.py Show resolved Hide resolved
core/dbt/contracts/graph/compiled.py Outdated Show resolved Hide resolved
Comment on lines +415 to +416
'test.test.not_null_base_extension_id.4a9d96018d',
'test.test.not_null_base_extension_id.60bbea9027'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know why the unique_ids have changed? We shouldn't be including any configs in the hash, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

The model kwarg changed with the where config. The hash includes kwargs.

core/dbt/task/test.py Outdated Show resolved Hide resolved
core/dbt/task/test.py Show resolved Hide resolved
Copy link
Contributor Author

@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.

Looking good! And looks like a lot of failing tests that remain are checking result.message against an integer, so just need to switch over to result.failures

core/dbt/task/printer.py Outdated Show resolved Hide resolved
@kwigley kwigley force-pushed the feature/net-new-test-configs branch from 87ed860 to 64b4d3f Compare May 21, 2021 16:01
@kwigley kwigley force-pushed the feature/net-new-test-configs branch from cd9f221 to 39c61f0 Compare May 21, 2021 20:56
@kwigley kwigley changed the title New test configs: where, limit, warn_if, error_if New test configs: where, limit, warn_if, error_if, fail_calc May 24, 2021
Copy link
Contributor Author

@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.

Took this for a ride, it's looking really good!

  • I found one thing we need to switch for config inheritance, which I didn't realize back in Feature: test config parity #3257.
  • I was able to reimplement the tests in dbt_utils through a combination of where + fail_calc.
  • I realize I originally opened this PR... so I'm going to mark it as review-ready, hah. Feel free to close and reopen in a new PR if you'd like.

@@ -334,10 +351,13 @@ def build_raw_sql(self) -> str:
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to reorder "{config}{{{{ {macro}(**{kwargs_name}) }}}}" to be {{{{ {macro}(**{kwargs_name}) }}}}{config}, so that specific configs (supplied by the user in the modifiers) override the generic configs set in the test macro.

  # this is the 'raw_sql' that's used in 'render_update' and execution
  # of the test macro
  # config needs to be rendered last to take precedence over default configs
  # set in the macro (test definition)
  def build_raw_sql(self) -> str:
      return (
          "{{{{ {macro}(**{kwargs_name}) }}}}{config}"
      ).format(
          macro=self.macro_name(),
          config=self.construct_config(),
          kwargs_name=SCHEMA_TEST_KWARGS_NAME,
      )

other.unrendered_config.get('severity')
)

# TODO: this is unused
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes... indeed it is. I confirmed that dbt test -m state:modified works as expected, leveraging the unrendered_config.

error_if: str = "!= 0"

@classmethod
def same_contents(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I see now that this is the proper place for it. Nice work, this logic is much clearer.

@jtcohen6 jtcohen6 marked this pull request as ready for review May 24, 2021 23:54
@kwigley kwigley closed this May 26, 2021
@kwigley kwigley mentioned this pull request May 26, 2021
4 tasks
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.

User-supplied fail_calc for tests Net-new test configs
3 participants