Test priority order of profiles directory configuration #5715

dbeatty10 · 2022-08-25T21:00:07Z

resolves #5714

Description

Verifies the following reverse priority order to search for profiles.yml:

HOME directory of the user (i.e. ~/.dbt/)
DBT_PROFILES_DIR environment variable
--profiles-dir command-line argument

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~~

ChenyuLInx · 2022-08-25T21:21:18Z

Looks good! You will need to setup pre-commit so that you commit are passing the code quality checks.

Here's the failed check. The command it run was pre-commit run --all-files --show-diff-on-failure

dbeatty10 · 2022-08-25T21:55:19Z

@ChenyuLInx it is passing all the automated checks now.

gshank · 2022-08-26T16:39:33Z

core/dbt/tests/util.py

@@ -69,7 +71,10 @@ def run_dbt(args: List[str] = None, expect_pass=True):

    print("\n\nInvoking dbt with {}".format(args))
    res, success = handle_and_check(args)
-    assert success == expect_pass, "dbt exit state did not match expected"
+
+    if expect_pass is not None:


I don't see the reason for this change. "expect_pass" should always be True or False.

If expect_pass is a binary option, then the exit code of the dbt <subcommand> <options> will preempt everything else and take priority over subsequent assertions. Having an option to run_dbt that doesn't assert allows the user to take control of the assertion process independent of the exit code.

Why this matters in this case:

dbt debug will have a non-zero exit code in certain situations (like if a profile is not found at a particular location).

In these tests, I want to get the output of dbt debug regardless of the exit code and test the content.

Is there some reason that it doesn't work to do expect_pass=False?

Overview

pytest recommends thinking of tests being composed of four steps:

Arrange

Act

Assert

Cleanup

Currently, the implementation of dbt_run combines both "Act" and "Assert". I want the option for it to only "Act" and leave the "Assert" for a separate and independent step. This also seems like it would align with the anatomy of a test that pytest is promoting.

Why

This pull request is a prelude to testing any implementation of #5411.

As-is, expect_pass=False might work for the ~9 different dbt_runs that end up being executed. But an implementation of #5411 will introduce another ~3 executions of dbt_runs (actually doing dbt debug) which might actually have a zero exit codes, hence those specific cases would need to be expect_pass=True.

It's greatly simplified if there is the option to separate "Assert" from "Act" within dbt_run.

Additionally, this would give more flexibility to other test implementers (while retaining the current default value of expect_pass=True).

Are there downsides or risks you see in my proposed change?

I don't really object to this change. It mostly works this way because that's the way it was in test/integration/base.py and it's just convenient to not have to do an assert for almost every dbt_run.

gshank · 2022-08-26T16:43:19Z

core/dbt/tests/util.py

@@ -94,6 +99,16 @@ def run_dbt_and_capture(args: List[str] = None, expect_pass=True):
    return res, stdout


+# Use this if you need to capture the standard out in a test


Why duplicate the 'run_dbt_and_capture' functions? What does this do different?

dbt debug contains print() statements which weren't showing up in the output for run_dbt_and_capture.

In my hands-on experiments, run_dbt_and_capture doesn't actually capture standard out -- I think it captures content streamed to the logs.

So I created a variant that includes the output of print() statements.

We don't need two different variations of this function. Looking at the original one, it looks like it certainly intended to capture the stdout logs, if you look at the 'capture_stdout_logs' in core/dbt/events/functions.py.

If there's something that it's missing, we should fix it rather than create an almost duplicate. What do you mean by "print()" statements? We shouldn't have any actual print statements to capture, should we?

I used git grep "print(" ./ to find instances of print().

dbt debug uses print() statements. There are a some other places too, but I didn't examine them in detail.

Do you know how to modify capture_stdout_logs to capture print() statements? Or some other alternative to avoid functions that are near duplicates? I saw how capture_stdout_logs captures logging, but didn't see any obvious way for it to capture standard out also.

gshank · 2022-08-26T16:48:18Z

tests/functional/profiles/test_profile_dir.py

+
+
+@pytest.fixture(scope="class")
+def profiles_home_root():


Have you tested what happens if there is no profiles.yml at this location? Or the ~/.dbt directory doesn't exist? Because we can't depend on there being such a file in a test environment.

I did for profiles.yml! I didn't check how dbt debug behaves if ~/.dbt doesn't exist, but that's something I can follow-up on.

The change I made to run_dbt to allow expect_pass=None enabled the tests to run dbt debug and capture the output, even when there are no profiles.yml files.

The beauty of these tests is that they merely confirm that it is looking in the expected directories for profiles.yml -- it is irrelevant to these tests if the profiles.yml actually exist within the user-specified directory or not. Other tests should verify the correct behavior when profiles.yml does/doesn't exist.

dbeatty10 · 2022-08-30T16:50:25Z

@gshank is the conversation regarding run_dbt_and_capture_stdout the only outstanding piece for this review? Or are there other portions as well?

In regards to run_dbt_and_capture_stdout, do you see how it can be combined with run_dbt_and_capture? I didn't see any way that was obvious to me, and that's why I created a separate function.

gshank · 2022-08-30T18:13:27Z

That's the only issue. I'd really rather not have a separate method in dbt.tests.util, but if you want to move it into the test case, that would be fine. Unfortunately I don't have time right now to investigate how to do it in the same method. Ideally we wouldn't have any print statements at all to deal with. Moving those print statements to logging statements might be the way to go, but I don't know if there are complications there.

dbeatty10 · 2022-08-30T21:47:58Z

@gshank Per your recommendation, moved run_dbt_and_capture_stdout into the test case.

dbeatty10 · 2022-08-30T21:50:07Z

@gshank Per your recommendation, moved run_dbt_and_capture_stdout into the test case.

gshank

Looks good!

* Method for capturing standard out during testing (rather than logs) * Allow dbt exit code assertion to be optional * Verify priority order to search for profiles.yml configuration * Updates after pre-commit checks * Move `run_dbt_and_capture_stdout` into the test case

dbeatty10 added 3 commits August 25, 2022 14:23

Method for capturing standard out during testing (rather than logs)

38f9781

Allow dbt exit code assertion to be optional

177160d

Verify priority order to search for profiles.yml configuration

5b683dd

dbeatty10 added the Skip Changelog Skips GHA to check for changelog file label Aug 25, 2022

dbeatty10 requested review from gshank and ChenyuLInx August 25, 2022 21:00

dbeatty10 requested a review from a team as a code owner August 25, 2022 21:00

cla-bot bot added the cla:yes label Aug 25, 2022

Updates after pre-commit checks

1934113

dbeatty10 mentioned this pull request Aug 26, 2022

Default to current working directory for profiles.yml and fall back to ~/.dbt #5717

Merged

11 tasks

gshank reviewed Aug 26, 2022

View reviewed changes

jtcohen6 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 30, 2022

dbeatty10 requested a review from gshank August 30, 2022 18:08

leahwicz added the Team:Language label Aug 30, 2022

Move run_dbt_and_capture_stdout into the test case

9d2b6f6

gshank approved these changes Aug 30, 2022

View reviewed changes

dbeatty10 merged commit 1df713f into main Aug 30, 2022

dbeatty10 deleted the dbeatty/test-profiles-dir-priority-order branch August 30, 2022 23:18

dbeatty10 mentioned this pull request Sep 16, 2022

Default to current working directory for profiles.yml and fall back to ~/.dbt #5412

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test priority order of profiles directory configuration #5715

Test priority order of profiles directory configuration #5715

dbeatty10 commented Aug 25, 2022 •

edited

Loading

ChenyuLInx commented Aug 25, 2022

dbeatty10 commented Aug 25, 2022

gshank Aug 26, 2022

dbeatty10 Aug 26, 2022

gshank Aug 29, 2022

dbeatty10 Aug 29, 2022

gshank Aug 29, 2022

gshank Aug 26, 2022

dbeatty10 Aug 26, 2022

gshank Aug 29, 2022

dbeatty10 Aug 29, 2022

gshank Aug 26, 2022

dbeatty10 Aug 26, 2022 •

edited

Loading

dbeatty10 commented Aug 30, 2022

gshank commented Aug 30, 2022

dbeatty10 commented Aug 30, 2022

dbeatty10 commented Aug 30, 2022

gshank left a comment

		@@ -94,6 +99,16 @@ def run_dbt_and_capture(args: List[str] = None, expect_pass=True):
		return res, stdout


		# Use this if you need to capture the standard out in a test

Test priority order of profiles directory configuration #5715

Test priority order of profiles directory configuration #5715

Conversation

dbeatty10 commented Aug 25, 2022 • edited Loading

Description

Checklist

ChenyuLInx commented Aug 25, 2022

dbeatty10 commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Overview

Why

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbeatty10 Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

dbeatty10 commented Aug 30, 2022

gshank commented Aug 30, 2022

dbeatty10 commented Aug 30, 2022

dbeatty10 commented Aug 30, 2022

gshank left a comment

Choose a reason for hiding this comment

dbeatty10 commented Aug 25, 2022 •

edited

Loading

dbeatty10 Aug 26, 2022 •

edited

Loading