-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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-2627] [Bug] --warn-error-options doesn't support configuration of turning test warnings into failures #7761
Comments
Thanks for discovering and raising this @joellabes and @epapineau 🤩 I was able to reproduce what you reported ✅ Root causeI suspect it might be the difference between the logic in the test task and warn_or_error. Potential solutionNot the most elegant solution (and completely untested!), but updating this: dbt-core/core/dbt/task/test.py Line 160 in 2d23782
to this might do the trick 🤷: if get_flags().WARN_ERROR or get_flags().WARN_ERROR_OPTIONS.includes(type(event).__name__): |
Hmm! Failures in tests and source freshness checks are a bit different from our other warnings-to-exceptions, because dbt handles "error" results gracefully, rather than interrupting its entire process. As a way for users to configure if get_flags().WARN_ERROR or get_flags().WARN_ERROR_OPTIONS.includes("LogTestResult"): That event has a "dynamic" log level, which is set conditionally based on the dbt-core/core/dbt/events/types.py Lines 1551 to 1563 in 6f51de4
|
Need to determine if we should fix this or document why we won't fix it. |
If I'm interpreting @jtcohen6's message correctly, the answer is "yes, we should fix this". And the specific way we may do so is: if get_flags().WARN_ERROR or get_flags().WARN_ERROR_OPTIONS.includes("LogTestResult"): That being said, this probably isn't a very high priority (since it didn't actually affect the reporters negatively, they merely noticed and reported it). Is that a fair summary @jtcohen6 ? |
Regardless of the answer ☝️, just want to also flag for the sake of completeness that source freshness tests have the same dynamic capabilities, so I expect are covered by the same issue where |
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 comment on the issue or else it will be closed in 7 days. |
Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest. Just add a comment to notify the maintainers. |
We are not prioritizing this ourselves right now, but labeling as a See implementation hint here. |
@dbeatty10 could i take this issue? |
That would be awesome @jx2lee 👍 |
Is this a new bug in dbt-core?
Current Behavior
I wrote about using the warn-error-options functionality on LI, and @epapineau asked to work with me to implement it in the internal analytics project.
After we got it working, we realised we might have to exclude tests with a warn severity as well as "nothing to do", and went poking around in
types.py
where we found LogTestResult (we assume that's the right one!)Because it extends
DynamicLevel
instead ofWarnLevel
, nothing happened when we included it in the dictionary of include/excludes. However,dbt build --warn-error
would create an error from a warn-level test (or freshness check etc).dbt test --warn-error-options {"include": "all"}
anddbt test --warn-error
should be identical, right?This happens to be convenient bug for us, because we didn't want to include them anyway and were trying to add them to the exclude-list, but a bug nonetheless.
Expected Behavior
dbt test --warn-error-options {"include": "all"}
anddbt test --warn-error
to be identicalSteps To Reproduce
Make a singular test:
Run
dbt --warn-error-options '{"include":"all"}' test -s single_line
and
dbt --warn-error test -s single_line
Relevant log output
Environment
Which database adapter are you using with dbt?
No response
Additional Context
No response
The text was updated successfully, but these errors were encountered: