-
Notifications
You must be signed in to change notification settings - Fork 42
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
Unit test prep #117
Unit test prep #117
Conversation
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.
small comments for now! gonna mostly rely on the CI here given the size of the diffs
- {id: 2, code: 'def'} | ||
model: my_first_dbt_model | ||
|
||
- name: my_second_unit_test |
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.
can we add an inline csv to any of the unit tests?
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.
yeh good idea
@@ -0,0 +1,67 @@ | |||
# this file was generated with dbt init with dbt 1.2.1 | |||
|
|||
version: 2 |
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.
can we add an invalid unit test here?
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.
We can... but I chose not to because adding an additional invalid item to an already-invalid test doesn't actually test anything. if I added a totally correct unit test block, the CI would still mark it as Task Failed Successfully because there are other failing components. So we probably need to think a bit about what we do there.
Maybe check for an expected number of failures?
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.
OK I did some more digging here, and it's possible to output the list of --all-errors
which I thought I could just count. But there can be multiple failure entries for a single line of yaml like this:
{
instancePath: '/tests/test/+enabled',
schemaPath: '#/$defs/jinja_string/pattern',
keyword: 'pattern',
params: { pattern: '\\{\\{.*\\}\\}' },
message: 'must match pattern "\\{\\{.*\\}\\}"'
},
{
instancePath: '/tests/test/+enabled',
schemaPath: '#/oneOf/1/type',
keyword: 'type',
params: { type: 'boolean' },
message: 'must be boolean'
},
{
instancePath: '/tests/test/+enabled',
schemaPath: '#/oneOf',
keyword: 'oneOf',
params: { passingSchemas: null },
message: 'must match exactly one schema in oneOf'
},
So my new theory for how we could approach this is to generate the failure list manually, and then compare against that in CI. I'm going to punt on that for another day, but I will add a failing mode so that when I come to it at least there's the necessary info.
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.
this would be an excellent enhancement! but agree that we should do it outside the scope of this PR
key: val | ||
jinja_key: "{{ target.name }}" | ||
|
||
unit_tests: |
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.
per graces comment, we can't have unit and regular tests here!
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.
We can't have data_tests
and tests
, but having unit_tests
and either one of the others is OK
|
||
unit_tests: | ||
meta: | ||
pineapple: pizza |
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.
delicious IMO
"type": "string" | ||
}, | ||
"format": { | ||
"description": "Defaults to `dict` when not specified", |
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.
do we define that default here?
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.
Decided not to because it uses the default
value for autocomplete purposes, and you wouldn't want it to autocomplete to the thing you're getting for free (so shouldn't be specifying at all)
} | ||
}, | ||
"given": { | ||
"type": "array", |
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.
is it worth moving the logic around data to a def
and reusing for given
and expect
?
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.
It surely is
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.
Actually I don't think it's possible - given
includes an input
block, and is also an array of objects instead of a single object.
As far as I can see, I can't union all of the common stuff with the input
block for the given
schema.
For a slightly friendlier diff, check out ff059d0...unit-test-prep |
Add support for unit tests
Diffing is gonna suck sorry. On the bright side, now that there is a long-lived
latest
folder, future changes will be easier to handle!