-
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
[Feature] add generic config for generic tests #10245
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10245 +/- ##
==========================================
- Coverage 88.72% 88.70% -0.02%
==========================================
Files 180 180
Lines 22476 22474 -2
==========================================
- Hits 19942 19936 -6
- Misses 2534 2538 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…out how to best represent same key error, failing correctly though
…dbt parse and inspecting the manifest
…rom earlier to be more dry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach of breaking out the logic into separate methods for readability. And I provided comments that align with that approach as you've implemented it.
However, I also think this could be done even better by dispatching to methods that correspond to stage instead of legacy/preferred. There's a lot of duplication in the two methods, which suggests that pivoting how it's split could be useful.
To make sure I completely understand the context, here's how I understand the intent and constraints:
- there is a legacy method of specifying config at the root, these expected config keys are captured in
self.CONFIG_ARGS
- there is a preferred method of specifying config which will show up as
self.args["config"]
(as a result of processingdata_test
viaself.extract_test_args()
) - the same key should not show up in both methods, otherwise raise
SameKeyNestedError()
- we need to render
str
entries, and fail on a missing macro - we need to filter
None
entries
Assuming the above is correct, this could be done by translating the above steps into the below pseudo-code:
def __init__(...)
...
raw_config = self._raw_config()
self.config = self._rendered_config(raw_config)
...
def _raw_config(self) -> Dict[str, Optional[str]]:
legacy_config = ...
config = ...
# identify issue where key is present in both
raw_config = ...
return raw_config
def _rendered_config(
self,
raw_config: Dict[str, Optional[str]],
render_ctx: Dict[str, Any],
column_name: Optional[str]
) -> Dict[str, Any]:
rendered_config = {}
for key, value in raw_config.items():
# try to render, raising an error if the macro is missing
# filter for `None` here since you're already looping, and you could make the argument that removing `None` values is a form of rendering
return rendered_config
running command
in a local project after getting models/seeds ran shows a json object with a config of
|
When I tried to combine the levels/ or hit its version of code more it would hit macro errors in dbt-adapters to augment to that style seems like it would require a higher lift to supply more configs to other existing macros. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some small nits that would make the code a bit more succinct, but nothing that's worth stopping from moving forward. If you have the time, take a look, otherwise merge and move on.
* init push arbitrary configs for generic tests pr * iterative work * initial test design attempts * test reformatting * test rework, have basic structure for 3 of 4 passing, need to figure out how to best represent same key error, failing correctly though * swap up test formats for new config dict and mixed varitey to run of dbt parse and inspecting the manifest * modify tests to get passing, then modify the TestBuilder class work from earlier to be more dry * add changelog * modify code to match suggested changes around seperate methods and test id fix * add column_name reference to init for deeper nested _render_values can use the input * add type annotations * feedback based on mike review
resolves #10197
Problem
We would like to update our
TestBuilder
class for generic tests to be able to accept any config that the user can think of needing.examples.
Solution
in the
Testbuilder
class we first setup a variable with a ll defined accepted configs we can keep this a little further down we can add a if condition to process theconfig
beyond the original of searching for theCONFIG_ARGS
variable.Checklist