Skip to content
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

Add TestCLIInvocationWithProfilesAndProjectDir #3176

Merged

Conversation

JCZuurmond
Copy link
Contributor

@JCZuurmond JCZuurmond commented Mar 17, 2021

resolves #3133

Description

See the issue #3133 for more details.

The profiles-dir flag needs to be relative to the project-dir, not the current working directory.

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.

@cla-bot
Copy link

cla-bot bot commented Mar 17, 2021

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: @JCZuurmond

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JCZuurmond, I take it you're trying to produce a test case for this bug before working on a resolution? If that's the case, I'm all for it!

I had a go at coding up the skeleton of a test case at the intersection of custom profile and project directories. As much as possible, I chose to inherit (rather than reimplement) the tricky setup from the custom-profile-dir test case. It's a bit messy, I left comments below for each scenario:

class TestCLIInvocationWithProfilesAndProjectDir(TestCLIInvocationWithProfilesDir):
    
    @use_profile('postgres')
    def test_postgres_toplevel_dbt_run_with_profile_dir_and_project_dir(self):
        workdir = os.getcwd()
        with tempfile.TemporaryDirectory() as tmpdir:
            os.chdir(tmpdir)
            
            # create a profile dir that does exist relative to our new tmp location,
            # but does not exist relative to the project location (i.e. original working directory)
            profiles_dir = 'tmp-profile'
            
            if not os.path.exists(profiles_dir):
                os.makedirs(profiles_dir)

            with open(f"./{profiles_dir}/profiles.yml", 'w') as f:
                yaml.safe_dump(self.custom_profile_config(), f, default_flow_style=True)
            
            # relative from new tmp location to project location (i.e. original working directory)
            project_dir = os.path.relpath(workdir, tmpdir)
            
            # confirm that profile does not exist relative to project dir
            self.assertEqual(os.path.exists(project_dir + '/' + profiles_dir), False)
            
            # for hand debugging of the test cases below
            import ipdb; ipdb.set_trace()
            
            # passes
            os.chdir(tmpdir)
            self.run_dbt(['deps', '--project-dir', project_dir])
            
            # passes
            os.chdir(tmpdir)
            self.run_dbt(['debug', '--profiles-dir', profiles_dir, '--project-dir', project_dir], profiles_dir=False)
            
            # fails today! dbt expects --profiles-dir to exist relative to --project-dir,
            # even though --profiles-dir exists in the current tmp directory
            os.chdir(tmpdir)
            results = self.run_dbt(['run', '--profiles-dir', profiles_dir, '--project-dir', project_dir], profiles_dir=False)
            
            # passes if we use an absolute path instead
            os.chdir(tmpdir)
            profiles_dir = os.path.abspath('tmp-profile')
            results = self.run_dbt(['run', '--profiles-dir', profiles_dir, '--project-dir', project_dir], profiles_dir=False)
            
            # run passes if --profiles-dir exists relative to --project-dir
            os.chdir(tmpdir)
            profiles_dir = 'dbt-profile'
            self.assertEqual(os.path.exists(project_dir + '/' + profiles_dir), True)
            results = self.run_dbt(['run', '--profiles-dir', profiles_dir, '--project-dir', project_dir], profiles_dir=False)

            # but debug fails today! --profiles-dir may exist relative to --project-dir (original working directory)
            # but it's missing relative to the current tmp directory
            os.chdir(tmpdir)
            profiles_dir = 'dbt-profile'
            self.assertEqual(os.path.exists(profiles_dir), False)
            self.assertEqual(os.path.exists(project_dir + '/' + profiles_dir), True)
            results = self.run_dbt(['debug', '--profiles-dir', profiles_dir, '--project-dir', project_dir], profiles_dir=False)

@cla-bot
Copy link

cla-bot bot commented Mar 24, 2021

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: @JCZuurmond

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Mar 24, 2021

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: @JCZuurmond

@JCZuurmond
Copy link
Contributor Author

JCZuurmond commented Mar 24, 2021

Hi @jtcohen6, thanks for your help. I indeed wanted to be on the same page first with the test after that I write a solution.

Inheriting a test class is not a good idea, as it will run the (non-overwritten) test methods of the parent class twice. Once for the parent class and once for the child.

If you run the following, you will see three test being executed, but we define two:

class TestParent:
    def test_first(self):
        assert True


class TestChild(TestParent):
    def test_second(self):
        assert True

I have to say it is hard for me to get to where I need. There are many side effects which are hard to understand for me. I was not able to parameterize a test or use a fixture, I think this is due to something which happens in the parent class.

Also, I am getting the error that my profile is not set correctly. This is happening due to the assert in the use_profile decorator here. Actually, now looking at the code once more I finally understand what is happening in that assert... such side effects make it hard for newbies to follow what is happening.

@jtcohen6
Copy link
Contributor

Inheriting a test class is not a good idea, as it will run the (non-overwritten) test methods of the parent class twice. Once for the parent class and once for the child.

Yes, good point! I had commented out some lines above, and just wanted to get something off the ground for use in development. Of course, if that's the approach you'd end up taking, you could split out the setup pieces into a common parent class:

class TestParent:
    def setUp(self):
        ...

class FirstTestChild(TestParent):
    def test_first(self):
        assert True

class SecondTestChild(TestParent):
    def test_second(self):
        assert True

such side effects make it hard for newbies to follow what is happening

I totally hear you on this. The way that we've implemented many of these test (especially some of the older ones) has involved a few opaque layers wrapped around pytest, especially some of the "magic" in use_profile. We need to refactor our testing suite to make it more accessible to newer contributors. I know this is something @kwigley has been thinking about a lot, too.

@JCZuurmond JCZuurmond mentioned this pull request Mar 24, 2021
@cla-bot
Copy link

cla-bot bot commented Mar 25, 2021

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: @JCZuurmond

Copy link
Contributor Author

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtcohen6 : could you have one more look? I think I got the test right. As described in the issue it fails for run it does not fail for debug (and deps)

@cla-bot
Copy link

cla-bot bot commented Mar 30, 2021

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: @JCZuurmond

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Mar 30, 2021

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: @JCZuurmond

Copy link
Contributor Author

@JCZuurmond JCZuurmond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtcohen6: The CI of 91dfa88 shows the added test fail and the CI for 97e2222 show that using abspath passes the tests. So, functionally it does the trick and solves #3133 .

However, the outcome of a test is dependent on another test, which we do not want. See comments below:

@use_profile("postgres")
def test_postgres_run_with_profiles_separate_from_project_dir(self):
self._test_postgres_sub_command_with_profiles_separate_from_project_dir("deps")
self._test_postgres_sub_command_with_profiles_separate_from_project_dir("run")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when adding run the test_postgres_test_with_profiles_separate_from_project_dir did not fail anymore

@JCZuurmond JCZuurmond marked this pull request as ready for review March 30, 2021 19:09
@jtcohen6
Copy link
Contributor

Nice work on the test cases @JCZuurmond!

Indeed, I don't quite follow the interdependency between test_postgres_test_with_profiles_separate_from_project_dir and test_postgres_run_with_profiles_separate_from_project_dir. What's the failure you initially saw in the former? Is it an issue with the context manager, in setting up / clearing the working directory?

In any case, the code change itself looks good. The only thing worth calling out is that, if the user passes a relative file path to --profiles-dir on the CLI, the value of args.profiles_dir will be an absolute file path. I think that's a totally reasonable change, we should just call it out in the Changelog.

@JCZuurmond
Copy link
Contributor Author

@jtcohen6: I get the following error:

0;dbt_test_user@56346c74684d: /usr/appdbt_test_user@56346c74684d:/usr/app$ tox -e explicit-py36 -- -vv -x -m profile_postgres test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDi
explicit-py36 installed: agate==1.6.1,apipkg==1.5,appdirs==1.4.4,asn1crypto==1.4.0,attrs==20.3.0,azure-common==1.1.26,azure-core==1.12.0,azure-storage-blob==12.8.0,Babel==2.9.0,backcall==0.2.0,backports-datetime-fromisoformat==1.0.0,
bleach==3.3.0,boto3==1.15.18,botocore==1.18.18,bumpversion==0.5.3,cachetools==4.2.1,certifi==2020.12.5,cffi==1.14.5,chardet==3.0.4,colorama==0.4.3,cryptography==3.4.6,dataclasses==0.8,-e git+git@github.com:JCZuurmond/dbt.git@97e22225
f74c0f85a51a5bb9ceadc023af4c341a#egg=dbt_bigquery&subdirectory=plugins/bigquery,-e git+git@github.com:JCZuurmond/dbt.git@97e22225f74c0f85a51a5bb9ceadc023af4c341a#egg=dbt_core&subdirectory=core,-e git+git@github.com:JCZuurmond/dbt.git
@97e22225f74c0f85a51a5bb9ceadc023af4c341a#egg=dbt_postgres&subdirectory=plugins/postgres,-e git+git@github.com:JCZuurmond/dbt.git@97e22225f74c0f85a51a5bb9ceadc023af4c341a#egg=dbt_redshift&subdirectory=plugins/redshift,-e git+git@gith
ub.com:JCZuurmond/dbt.git@97e22225f74c0f85a51a5bb9ceadc023af4c341a#egg=dbt_snowflake&subdirectory=plugins/snowflake,decorator==4.4.2,distlib==0.3.1,docutils==0.16,execnet==1.8.0,filelock==3.0.12,flake8==3.9.0,flaky==3.7.0,freezegun==
0.3.12,google-api-core==1.23.0,google-auth==1.28.0,google-cloud-bigquery==2.3.1,google-cloud-core==1.4.4,google-crc32c==1.1.2,google-resumable-media==1.2.0,googleapis-common-protos==1.52.0,grpcio==1.36.1,hologram==0.0.14,idna==2.9,im
portlib-metadata==1.7.0,importlib-resources==1.5.0,ipdb==0.13.7,ipython==7.16.1,ipython-genutils==0.2.0,isodate==0.6.0,jedi==0.18.0,jeepney==0.6.0,Jinja2==2.11.2,jmespath==0.10.0,json-rpc==1.13.0,jsonschema==3.1.1,keyring==21.8.0,lea
ther==0.3.3,Logbook==1.5.3,MarkupSafe==1.1.1,mashumaro==2.0,mccabe==0.6.1,minimal-snowplow-tracker==0.0.2,more-itertools==8.7.0,msgpack==1.0.2,msrest==0.6.21,mypy==0.782,mypy-extensions==0.4.3,networkx==2.5,oauthlib==3.1.0,oscrypto==
1.2.1,packaging==20.9,parsedatetime==2.6,parso==0.8.1,pexpect==4.8.0,pickleshare==0.7.5,pkginfo==1.7.0,pluggy==0.13.1,prompt-toolkit==3.0.17,proto-plus==1.18.0,protobuf==3.15.6,psycopg2-binary==2.8.6,ptyprocess==0.7.0,py==1.10.0,pyas
n1==0.4.8,pyasn1-modules==0.2.8,pycodestyle==2.7.0,pycparser==2.20,pycryptodomex==3.10.1,pyflakes==2.3.0,Pygments==2.8.1,PyJWT==1.7.1,pyOpenSSL==20.0.1,pyparsing==2.4.7,pyrsistent==0.17.3,pytest==5.4.3,pytest-forked==1.3.0,pytest-log
book==1.2.0,pytest-xdist==1.34.0,python-dateutil==2.8.1,python-slugify==4.0.1,pytimeparse==1.1.8,pytz==2017.2,PyYAML==5.4.1,readme-renderer==29.0,requests==2.23.0,requests-oauthlib==1.3.0,requests-toolbelt==0.9.1,rfc3986==1.4.0,rsa==
4.7.2,s3transfer==0.3.4,SecretStorage==3.3.1,six==1.15.0,snowflake-connector-python==2.3.6,sqlparse==0.3.1,text-unidecode==1.3,toml==0.10.2,tox==3.14.4,tqdm==4.59.0,traitlets==4.3.3,twine==3.3.0,typed-ast==1.4.2,typing-extensions==3.
7.4.3,urllib3==1.25.11,virtualenv==20.0.3,wcwidth==0.2.5,webencodings==0.5.1,Werkzeug==1.0.1,zipp==3.4.1                                                                                                                                
explicit-py36 run-test-pre: PYTHONHASHSEED='2704484378'
explicit-py36 run-test: commands[0] | /bin/bash -c '/usr/app/.tox/explicit-py36/bin/python -m pytest --durations 0 -v -vv -x -m profile_postgres test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithP
rofilesAndProjectDir'                                                                                                                                                                                                                   
========================================================================================================== test session starts ==========================================================================================================
platform linux -- Python 3.6.9, pytest-5.4.3, py-1.10.0, pluggy-0.13.1 -- /usr/app/.tox/explicit-py36/bin/python
cachedir: .tox/explicit-py36/.pytest_cache
rootdir: /usr/app
plugins: flaky-3.7.0, forked-1.3.0, logbook-1.2.0, xdist-1.34.0
collected 3 items                                                                                                                                                                                                                       

test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_debug_with_profiles_separate_from_project_dir PASSED                                                  [ 33%]
test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_deps_with_profiles_separate_from_project_dir PASSED                                                   [ 66%]
test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_test_with_profiles_separate_from_project_dir FAILED                                                   [100%]

=============================================================================================================== FAILURES ================================================================================================================
_________________________________________________________________ TestCLIInvocationWithProfilesAndProjectDir.test_postgres_test_with_profiles_separate_from_project_dir _________________________________________________________________

self = <test_cli_invocation.TestCLIInvocationWithProfilesAndProjectDir testMethod=test_postgres_test_with_profiles_separate_from_project_dir>

    @use_profile("postgres")
    def test_postgres_test_with_profiles_separate_from_project_dir(self):
>       self._test_postgres_sub_command_with_profiles_separate_from_project_dir("test")

test/integration/015_cli_invocation_tests/test_cli_invocation.py:284: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test/integration/015_cli_invocation_tests/test_cli_invocation.py:276: in _test_postgres_sub_command_with_profiles_separate_from_project_dir
    self.run_dbt(args, profiles_dir=False)
test/integration/base.py:578: in run_dbt
    "dbt exit state did not match expected")
E   AssertionError: False != True : dbt exit state did not match expected
--------------------------------------------------------------------------------------------------------- Captured stdout call ----------------------------------------------------------------------------------------------------------
Running with dbt=0.19.0
Found 1 model, 1 test, 0 snapshots, 0 analyses, 138 macros, 0 operations, 0 seed files, 0 sources, 0 exposures

15:14:49 | Concurrency: 1 threads (target='default')
15:14:49 | 
15:14:49 | 1 of 1 START test accepted_values_model_id__1........................ [RUN]
15:14:49 | 1 of 1 ERROR accepted_values_model_id__1............................. [ERROR in 0.02s]
15:14:49 | 
15:14:49 | Finished running 1 test in 0.14s.

Completed with 1 error and 0 warnings:

Database Error in test accepted_values_model_id__1 (models/subdir1/subdir2/schema.yml)
  relation "test16184132888554005239_test_cli_invocation_015_custom.model" does not exist
  LINE 14:     from "dbt"."test16184132888554005239_test_cli_invocation...
                    ^
  compiled SQL at target/compiled/test/models/subdir1/subdir2/schema.yml/schema_test/accepted_values_model_id__1.sql

Done. PASS=0 WARN=0 ERROR=1 SKIP=0 TOTAL=1
--------------------------------------------------------------------------------------------------------- Captured logbook call ---------------------------------------------------------------------------------------------------------
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: test connection "__test" executing: DROP SCHEMA IF EXISTS "test16184132888554005239_test_cli_invocation_015" CASCADE
[DEBUG] dbt: Opening a new connection, currently in state init
[DEBUG] dbt: On __test: Close
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: test connection "__test" executing: CREATE SCHEMA "test16184132888554005239_test_cli_invocation_015"
[DEBUG] dbt: Opening a new connection, currently in state closed
[DEBUG] dbt: On __test: Close
[INFO] dbt: Invoking dbt with ['--strict', '--test-new-parser', 'test', '--profiles-dir', './tmp-profile', '--project-dir', '../dbt-int-test-e_emovrn', '--log-cache-events']
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: test connection "__test" executing: DROP SCHEMA IF EXISTS "test16184132888554005239_test_cli_invocation_015" CASCADE
[DEBUG] dbt: Opening a new connection, currently in state init
[DEBUG] dbt: On __test: Close
[DEBUG] dbt: Connection '__test' was properly closed.
======================================================================================================== slowest test durations =========================================================================================================
0.67s call     test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_test_with_profiles_separate_from_project_dir
0.23s call     test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_debug_with_profiles_separate_from_project_dir
0.08s call     test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_deps_with_profiles_separate_from_project_dir
0.00s setup    test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_debug_with_profiles_separate_from_project_dir
0.00s teardown test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_test_with_profiles_separate_from_project_dir
0.00s setup    test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_test_with_profiles_separate_from_project_dir
0.00s setup    test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_deps_with_profiles_separate_from_project_dir
0.00s teardown test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_debug_with_profiles_separate_from_project_dir
0.00s teardown test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_deps_with_profiles_separate_from_project_dir
======================================================================================================== short test summary info ========================================================================================================
FAILED test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProfilesAndProjectDir::test_postgres_test_with_profiles_separate_from_project_dir - AssertionError: False != True : dbt exit state did n...
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! stopping after 1 failures !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
====================================================================================================== 1 failed, 2 passed in 4.21s ======================================================================================================
ERROR: InvocationError for command /bin/bash -c '/usr/app/.tox/explicit-py36/bin/python -m pytest --durations 0 -v -vv -x -m profile_postgres test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProf
ilesAndProjectDir' (exited with code 1)                                                                                                                                                                                                 
________________________________________________________________________________________________________________ summary ________________________________________________________________________________________________________________
ERROR:   explicit-py36: commands failed

@cla-bot cla-bot bot added the cla:yes label Apr 14, 2021
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JCZuurmond Ok, I took these integration tests for a spin locally and ran into some (different) errors. I think I was able to resolve pretty simply. That said, this is definitely a step beyond my knowledge of file system operations :)

The fact that this change solves the stated problem, and that there are tests passing in CircleCI to capture that changed behavior, makes me feel that the PR is good-enough shape to move ahead. My only concern now is merging a test that will brick local integration testing for other unrelated PRs in the future.

So:

  • Let me know what you think of the proposed change
  • Please add yourself to the list of contributors in the changelog! Getting to the bottom of this has been a nontrivial effort, and I really appreciate the time you've put into it

@JCZuurmond
Copy link
Contributor Author

JCZuurmond commented Apr 19, 2021

* Let me know what you think of the proposed change

The changes do not solve the interdepency between the tests. See the CI for 7ca7c9f.

I think there is an interdependency due to that the run tests creates some models (SQL tables/views/..) for which is tested, which leaks over to the other tests. But I am not completely sure.

* Please add yourself to the list of contributors in the changelog! Getting to the bottom of this has been a nontrivial effort, and I really appreciate the time you've put into it

done!

@JCZuurmond JCZuurmond force-pushed the fix-profiles-dir-relative-to-project-dir branch from 20c2720 to 41e344d Compare April 19, 2021 18:58
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huzzah! All tests pass locally for me. Really, really nice work @JCZuurmond—thanks for the contribution!

@jtcohen6
Copy link
Contributor

Ugh, it looks like we have one last failing Windows test:

test\integration\015_cli_invocation_tests\test_cli_invocation.py:276: in _test_postgres_sub_command_with_profiles_separate_from_project_dir
    self.run_dbt(other_args, profiles_dir=False)
test\integration\base.py:576: in run_dbt
    "dbt exit state did not match expected")
E   AssertionError: False != True : dbt exit state did not match expected

And then also:

[ERROR] dbt: Could not clean up after test - c:\users\vssadm~1\appdata\local\temp\dbt-int-test-jjg_8t88 not removable
...
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'c:\\users\\vssadm~1\\appdata\\local\\temp\\dbt-int-test-jjg_8t88\\logs\\dbt.log'

It definitely I'm not sure why this error only cropped up for my changelog-conflict-resolving commit, and not for the previous commit. Again, the changes here LGTM, I'm just wary of merging something that will cause intermittent failures in our CI suite.

@JCZuurmond JCZuurmond force-pushed the fix-profiles-dir-relative-to-project-dir branch from fb02caa to 13b00c5 Compare April 28, 2021 08:37
@JCZuurmond
Copy link
Contributor Author

* Let me know what you think of the proposed change

The changes do not solve the interdepency between the tests. See the CI for 7ca7c9f.

I think there is an interdependency due to that the run tests creates some models (SQL tables/views/..) for which is tested, which leaks over to the other tests. But I am not completely sure.

* Please add yourself to the list of contributors in the changelog! Getting to the bottom of this has been a nontrivial effort, and I really appreciate the time you've put into it

done!

@jtcohen6 : I think it is actually a good thing it now fails! Referring to the comment above, it now fails when running the test command, even with the run test there. I think I finally understand why: the test is expecting a model which is created by the run to exist. Most likely the tear down does not properly clean the database, hence the dependency between the tests.

Now, I have updated the test test to include the deps and run

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @JCZuurmond!!

@jtcohen6 jtcohen6 merged commit 9d295a1 into dbt-labs:develop Apr 28, 2021
@JCZuurmond JCZuurmond deleted the fix-profiles-dir-relative-to-project-dir branch April 28, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

With dbt run the --profiles-dir becomes relative to --project-dir
2 participants