Functional test framework working with Click, dbtRunner #6387

MichelleArk · 2022-12-06T03:52:45Z

Description

adds dbtRunner to run dbt CLI programmatically. Accepts profile, project, and manifest (for later), and uses them in combination with CLI args during invoke calls to invoke the CLI.
runs tests/functional tests through dbtRunner
- many integration tests are failing because most commands are not yet implemented, but expected to start passing as we implement commands.
- did some manual inspection and found tests/functional/dependencies/test_simple_dependency.py::TestSimpleDependencyUnpinned::test_simple_dependency, which only depends on running deps - and succeeds!
runs test/integration tests last so that we can start surfacing failing functional tests in CI logs.
sets a couple param defaults to callables so they are set at runtime as opposed to param.py import. This is needed for correct fixture setup which set the project/profile dirs dynamically.

Checklist

I have read the contributing guide and understand what's expected of me
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 opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

github-actions · 2022-12-06T03:53:03Z

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.

core/dbt/cli/context.py

core/dbt/tests/util.py

core/dbt/cli/context.py

ChenyuLInx · 2022-12-12T22:59:33Z

core/dbt/cli/main.py

+        ctx.obj["project"] = project
+
+    # Context for downstream commands
+    ctx.obj["flags"] = flags


Not saying we need to change it but since we actually construct flags, profile, project within the command, is ctx.obj still the best place to put it?

Hmmm, definitely worth questioning! An alternative could be to inject profile, project, manifest, etc into the command method's args, similar to how click.pass_context works. This way we'd be a little less coupled to click, although we'd probably still need to use the click context to store any project/profile overrides passed to dbtRunner. There would be some naming conflicts to resolve (ex: params from options named --profile and a profile override).

I think it's worth keeping in mind alternatives to using ctx.obj for passing user-configured objects to commands in mind as we go as the current approach doesn't preclude us from changing this up in the future.

core/dbt/cli/main.py

jtcohen6 · 2022-12-13T12:13:06Z

core/dbt/cli/main.py

+
+
+# Programmatic invocation
+class dbtRunner:


Is this first rough version of thin wrapper, for end users who want to programmatically invoke dbt-core via Python API? Thinking:

# initialize longer-lived dbtRunner # by implicitly loading project/profile/etc based on config + cwd dbt = dbtRunner() # submit commands to dbtRunner # each command returns # results: List[Result] # success: bool results, success = dbt.run(...) results, success = dbt.test(...)

Yes, exactly!

# initialize longer-lived dbtRunner # by implicitly loading project/profile/etc based on config + cwd dbt = dbtRunner()

^ We could load profile/project/etc from defaults at during dbtRunner instantiation but depending on the args in subsequent commands, they may require reloading. Something to consider for a follow-up though.

results, success = dbt.run(...) results, success = dbt.test(...)

this part is tbd I think? current way would be pass in command through args, the main advantage is some CLI args need to be specified before the actual command. So exposing things this way provide best flexibility at the moment.

args = ['run', '--project-dir', 'something'] res, success = dbt.invoke(args)

current way would be pass in command through args

We'll definitely want to support this generic pattern ("given an arbitrary list of CLI arg strings, invoke"). I could also see us wanting to support a more structured approach, where users can build up a dictionary of attributes and pass it into a named command. All of which is to say, we should design this programmatic interface for real, before we commit to it as a public-facing API — and that means sitting down, writing some user stories, talking with community members. All good stuff to do next year :)

the main advantage is some CLI args need to be specified before the actual command

Lukewarm take: This is pretty confusing for end users! We should really seek to support flags in any order (and resolve any resulting ambiguities by actually renaming flags). I know we implemented the new CLI modeling in accordance with the old CLI model — parity is a good goal to shoot for with any refactor — but it would be great to just support both of these, seamlessly and equivalently:

$ dbt --no-use-color run --select one_model $ dbt run --select one_model --no-use-color

cc @iknox-fa

Well, that wasn't part of the original plan, but I guess we can see about supporting arbitrary flag ordering-- It's not something we should take on in this PR though. @jtcohen6, can you write up a ticket that we can put into the estimation process.

@iknox-fa Totally - opened here: #6497

MichelleArk · 2022-12-14T16:07:08Z

core/dbt/cli/requires.py

+            # TODO: Decouple target from profile, and remove the need for profile here:
+            # https://github.com/dbt-labs/dbt-core/issues/6257
+            if not ctx.obj.get("profile"):
+                raise DbtProjectError("profile required for project")


Is there a more appropriate exception to raise here? I'm not too concerned either way because we should be able to get away from requiring a profile at which point the exception won't be necessary here.

Can we also check for requires.profile is added to the current command or not?

I think that may be overly-rigid. Technically this method only requires profile to exist in ctx.obj, and shouldn't need to be concerned with how it gets there. Adding that check would make it unnecessarily difficult to change the 'how'. Also not sure it's worth the effort given we're looking to decouple profile from project creation.

Is there a more appropriate exception to raise here?

This seems appropriate to me. I do know that exceptions are getting some closer attention from @emmyoop and others soon so I wouldn't stress it.

There's also a DbtProfileError Exception that is also a subclass of `DbtConfigError.

stu-k

Looks good to me. There is one test.py file that seems to be empty that you can probably remove.

ChenyuLInx

Looks good to me, also tried to extend this in another PR, works great!

iknox-fa · 2022-12-16T22:00:28Z

tox.ini

  {envpython} -m pytest --cov=core {posargs} tests/functional
  {envpython} -m pytest --cov=core {posargs} tests/adapter
+  {envpython} -m pytest --cov=core -m profile_postgres {posargs} test/integration


Nothing important, but I'm curious why the test run-order changed?

Given the test/integration tests will continue to fail on this branch, I moved them to run last so that errors from tests/functional would be visible in CI logs

iknox-fa · 2022-12-16T22:02:52Z

core/dbt/tests/util.py

@@ -73,7 +73,8 @@ def run_dbt(args: List[str] = None, expect_pass=True):
        args = ["run"]

    print("\n\nInvoking dbt with {}".format(args))
-    res, success = handle_and_check(args)
+    dbt = dbtRunner()


I love how simple this change ended up being. It's a sign that we made the right abstractions when we implemented the new functional testing framework.

iknox-fa

This looks great @MichelleArk!
Ship it!

cla-bot bot added the cla:yes label Dec 6, 2022

MichelleArk force-pushed the click-functional-tests branch from b608756 to d37d023 Compare December 7, 2022 21:51

ChenyuLInx reviewed Dec 7, 2022

View reviewed changes