Add a better error for undefined macros

[jaypeedevlin]

resolves #2999

Description

Adds a new UndefinedMacroException and raises this when an undefined macro is caught.

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.

[jaypeedevlin]

The issue talks about the possibility of comparing packages.yml to dbt_modules in order to give a more nuanced response here. I would be open to adding that in if I could get some guidance as to the code paths involved.

[jtcohen6]

@jaypeedevlin I just left a comment over in #2999! I think there's a way we can do both :)

[jtcohen6]

Okay, I think I figured out why the tests are failing! There's one integration test in particular, however, that expects to fail when its package is missing, and raise a helpful error message, before it actually runs deps:

https://github.com/fishtown-analytics/dbt/blob/bbab6c2361139073db5259d98f12addae7302b4c/test/integration/059_source_overrides_test/test_source_overrides.py#L78-L84

I think we could either:

• Split apart the test into a Failing and Passing version. In the former, the packages_config should be empty (thereby passing the check we're adding in this PR), and we can still confirm that source overrides raise a helpful error message if their associated package is missing. Then, the Passing version has a reasonable packages_config and runs deps right away.
• Remove that piece of the test. With this amazing new error message, no one should ever have this problem ever again!

[jaypeedevlin]

Thanks for the pointers, we're looking in much better shape now. Here's what I have as outstanding:

• mypy is failing over the self.packages.packages call in runtime.py. I tried replacing .packages with .get('packages', []) but it seemed like sometimes that object isn't a dict so I'm not sure how to resolve this.
• There's a pesky error coming out of the 109 (RPC) set of tests that I'm not sure how to handle.
• There are a series of tests around documentation in the 029 set — initially these were failing because they were missing calls to dbt deps, but now those calls are being made there are extra models included that are causing test failures. I see two potential paths forward, and I'm interested in your thoughts:
  • Removing the packages config altogether — I've tested and this does solve the problem, but I'm not sure if it means the tests aren't doing all we want them to do.
  • Changing the default expected models, and then adding extra data to the expected manifests (but I'm not sure how to get this extra data if I went that route).

[jtcohen6]

Nice, we're really close!

I left comments for mypy (let's cheat) and 029_docs_generate (seems like we were never using the package at all).

As far as the 100_rpc_test, here is the problematic bit:

https://github.com/fishtown-analytics/dbt/blob/8ac5cdd2e1b5de14b6e341188a2d3498af305687/test/integration/100_rpc_test/test_rpc.py#L1143-L1151

Namely, line 1146 returns:

E   AssertionError: 'error' != 'ready'
E   - error
E   + ready

Why? Now that we raise a compilation error as soon as we detect a mismatch between packages.yml and dbt_modules, the RPC server will really aggressively raise that error on startup/sighup. Previously, we tested starting up an RPC server and checking that it's "ready" (even though it's missing a dependency), now it returns "error" status:

{'result': {'logs': [], 'state': 'error', 'error': {'message': 'Compilation Error\n  dbt found 1 package(s) specified in packages.yml, but only 0 package(s) installed in dbt_modules. Run "dbt deps" to install package dependencies.'}, 'timestamp': '2021-05-17T22:00:29.817312Z', 'pid': 62478}, 'id': 1, 'jsonrpc': '2.0'}

This is a positive change IMO! So we just need to rework the test accordingly:

    def _check_start_predeps(self):
        self.assertFalse(os.path.exists('./dbt_modules'))
        status = self.assertIsResult(self.query('status').json())
        # will return an error because defined dependency is missing
        self.assertEqual(status['state'], 'error')
        return status

[jtcohen6]

Ok, nothing makes you feel like you're on the right track quite like some mysterious RPC test errors! I figured out what's going on here for test/rpcs/test_deps.py, which makes sense. I'm not sure about the others, but they're passing when I run locally on your branch, so I'm hopeful it's just guilt by association.

This is, I realize, an opinionated change to how the RPC server works. Today, if there are packages defined but not installed, dbt rpc is still happy to spin up a server and wait for requests—unless, of course, the uninstalled package implicitly causes a Jinja parse error, e.g. 'dbt_utils' is undefined, which is the very thing we seek here to ameliorate. So, I see this PR taking a principled stand that it's better to raise a very early, very explicit error as soon as a user interacts with the RPC server, and allow no further activity until they run deps.

Here's what that looks like in the test case:

https://github.com/fishtown-analytics/dbt/blob/30fed8d4218ce79d324ec41741fd41b79f678e9d/test/rpc/test_deps.py#L16-L29

We need:

• To pass criteria='error' as an additional keyword argument to get_querier(). That tells the RPC test utility, which is responsible for spinning up the RPC server, that we're not expecting it to return a 'ready' status right from the get go.
• Contrary to the comment, we should not be able to run queries at startup if there are packages missing, given the error message that this PR seeks to raise. So we should change async_wait_for_result() to is_error() in line 26—that is, it can still try to run that query, but it should expect an error in response.
• The status query should still work fine, just the comment is wrong: the status should be something negative.

[jaypeedevlin]

That the previous commit passed is interesting — I still had a single failing test locally. I'm going to fix the merge conflicts and see if it passes on a second go!

[jtcohen6]

@jaypeedevlin This sure looks good to me! Thanks so much for the contribution :)