Fix tuple index out of range in custom schema tests

[JusLarsen]

This PR fixes #1808

We're looking into using dbt at work and I thought that helping with a small issue would be a great way to familiarize myself with the way it works. Apologies if I've missed a style guide or submitted something incorrectly! I'm happy to make any adjustments required.

Code Changes

• modified node_runners.py. I added another condition under which a RuntimeErroris raised by the execute_test function. If table has zero rows, it provides a RuntimeError message instead of crashing.

Test Changes

All changes below are relative to the 008_schema_tests_test/ directory. I wasn't 100% sure where to put my tests but this seemed like the best place given the issue being addressed (macro failures in schema tests).

• added macros-v2/malformed/tests.sql. This macro will always return an empty result set to trigger the conditions seen in the linked issue. I tried to follow the directory layout as seen in this and other tests by creating a malformed subdirectory.
• moved macros-v2/tests.sql to macros-v2/macros/tests.sql so we can avoid loading the malformed macro for other previously existing tests.
• added models-v2/custom-bad-test-macro/schema.yml based on the custom model. I trimmed the model down to have one test that will fail and one that will error.
• added models-v2/custom-bad-test-macro/table_copy.sql based on the custom model.
• modified test_schema_v2_tests.py. I added TestMalformedMacroTests for testing schemas with malformed macros and I updated TestHooksInTests to point to the original macro file now nested one directory further down.

Verification of fix

Remove the new lines from node_runners.py and run tests. You'll see a hard crash with a tuple index out of range error.

Final thoughts

If you'd prefer I create an entirely new test folder or want me to move my tests just let me know. If this PR isn't up to snuff and you have the time for it, I always like feedback. I really dig this project and appreciate all the time and effort that has gone into it. :)

[cla-bot]

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @JusLarsen

[drewbanin]

Thanks for sending through this PR and for a very thoughtful writeup! I think you did a great job with the tests and this is all looking really good to me :)

I did add one comment - let me know what you think about that. I'm going to kick off the integration tests presently. Looking forward to getting this merged - I think we can sneak it in for our 0.15.0 release :)

@cla-bot check

[drewbanin]

What do you think about changing the if statement above this line to if num_rows != 1?

[JusLarsen]

I initially changed the if statement to use that exact logic, but ended up with the elif so that we could communicate a different RuntimeError message back to the user.

If you think the first error message is informative enough I'm happy to make that change quickly!

[drewbanin]

Yeah - I think that calling out the "shape" of the query results here makes sense. I suppose we can update this message to says "Bad test {name}: returned {rows} rows and {cols} cols but expected 1 row and 1 column". You buy that?

We use pep-8 as a style guide for dbt -- just make sure the log line isn't longer than 79 characters and you should be good!

[JusLarsen]

Sounds good to me, I've updated the logic, rebased, and force pushed over the branch for this PR.

[drewbanin]

looks great!

You did a really amazing job with this PR - it was a pleasure to review :D

Going to merge this when the tests are passing - it will ship in 0.15.0 in a couple of weeks

[drewbanin]

turns out the @cla-bot check thing only works in comments - not reviews!

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[JusLarsen]

Argh, I'm not sure why unit tests are failing in the pipeline, they're working fine for me using docker on mac os.

[drewbanin]

Alright! This is ready to go as-is. If you don't mind though, I think we can make one more tiny change here that will improve the output of this exception. Instead of raising a RuntimeError, we can raise a compiler error - this will help dbt's error handling describe where the error occurred.

If you change the RuntimeError to instead use:

dbt.exceptions.raise_compiler_error(msg, test)

Then the error will look like:

Instead of

You can also use test.test_metadata.name instead of test.name to find the name of the test definition instead of the dbt-generated test name (eg. bad vs. bad_debug_id) as shown in the error message above.

[JusLarsen]

Ooooh, I like the idea of swapping it to a compiler error.

I should have some time this afternoon to play around with the suggested change, I’ll reply in this thread once I’ve made the modification.

[JusLarsen]

@drewbanin updated to use a compiler error instead, rebased, and pushed to my repo. We should be good to go now!

[drewbanin]

Awesome - I just kicked off the tests here again. It looks like GitHub is showing the wrong checks - that happens sometimes. I'll merge this when they all pass :)

Thanks for your contribution to dbt!

[beckjake]

Thanks for the awesome PRs @JusLarsen, this is great - I'm so happy this is finally going to be a good error message!