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

dbt debug should return 1 when one of the tests fail #3018

Merged

Conversation

sdebruyn
Copy link
Contributor

resolves #3017

Description

DBT debut task now returns a boolean to indicate if all of the tests have succeeded or not.

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 cla-bot bot added the cla:yes label Jan 20, 2021
@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 20, 2021

@sdebruyn I'm not sure why CircleCI tests aren't running. Do you by chance have your fork set up in a different Circle org? For the time being, I'll try to manually trigger them.

@sdebruyn
Copy link
Contributor Author

@jtcohen6 No, didn't do anything special. We're not using CircleCI. Do you know why the postgres tests are failing? I thought I only had to toggle expect_pass
It's not that straightforward to debug the unit tests locally.

@kwigley
Copy link
Contributor

kwigley commented Jan 20, 2021

hey @sdebruyn! looks like the unit test workflow in CircleCI is failing because of a flake8 error. I definitely agree with you, this isn't very straight forward. I'm hoping we can make this a better experience, I'm actively thinking about this.

In the meantime, you can run linting and tests locally with docker if you haven't done this already.

docker-compose run --rm test tox -e flake8 should help with the error blocking CircleCI tests at the moment. If you don't want to use docker you can run the same flake8 command defined in here https:/fishtown-analytics/dbt/blob/1f927a374c8bd52a12a20d892fed9d59cffd04f4/tox.ini#L7-L11 Hopefully this unblocks you!

@sdebruyn
Copy link
Contributor Author

Okay, I fixed the flake test, but how do I fix the other ones? CircleCi is not triggering after fixing the flake test. How do you usually work on the project in your IDE? Do you always run the docker containers for the tests or is there a way to setup PyCharm/IntelliJ or VS Code to be able to set breakpoints etc.?

@kwigley
Copy link
Contributor

kwigley commented Jan 20, 2021

@sdebruyn not sure what's up with CircleCI tests.. can you try closing and reopening the PR to force tests to run again. Sorry about this!

At the moment, yes ☹️. I and other devs run tests in docker container and use python's pdb for debugging. We can definitely make this experience more IDE friendly, I use VSCode at the moment so I feel the pain as well.

Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

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

Looks good! I just added some small comments. Hopefully we can get the CI tests sorted out.

core/dbt/task/debug.py Outdated Show resolved Hide resolved
core/dbt/task/debug.py Outdated Show resolved Hide resolved
@jtcohen6
Copy link
Contributor

@kwigley I'm not sure why the Circle tests aren't running; sometimes there are weird settings with certain forks. In the meantime, we can manually trigger them by pulling commits and pushing to branch pr/3018, which is how we used to do it in the old days:

git branch -D pr/3018
git fetch origin pull/3018/head:pr/3018
git checkout pr/3018
git push origin pr/3018

@sdebruyn
Copy link
Contributor Author

When I run the tests locally, I don't see the actual error message. I always get this one instead:

============================================================================================= FAILURES ==============================================================================================
_______________________________________________________________________________ TestDebug.test_postgres_empty_target ________________________________________________________________________________
[gw1] linux -- Python 3.6.9 /usr/app/.tox/integration-postgres-py36/bin/python
/usr/app/test/integration/base.py:375: in setUp
    self._symlink_test_folders()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <test_debug.TestDebug testMethod=test_postgres_empty_target>

    def _symlink_test_folders(self):
        for entry in os.listdir(self.test_original_source_path):
            src = os.path.join(self.test_original_source_path, entry)
            tst = os.path.join(self.test_root_dir, entry)
            if os.path.isdir(src) or src.endswith('.sql'):
                # symlink all sql files and all directories.
                os.symlink(src, tst)
>       os.symlink(self._logs_dir, os.path.join(self.test_root_dir, 'logs'))
E       FileExistsError: [Errno 17] File exists: '/usr/app/logs/test16112220008926706441' -> '/tmp/dbt-int-test-61pai0fb/logs'

@sdebruyn sdebruyn requested a review from kwigley January 21, 2021 10:00
@sdebruyn
Copy link
Contributor Author

FYI for the CircleCI issue: I had to grant CircleCI access to our organization. Strange, since the repo is public...

Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

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

👍

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Thanks for the contribution @sdebruyn!

(Sorry, did not mean to tag @lynxcare there, was just looking at the fork when I commented)

@jtcohen6 jtcohen6 merged commit cb460a7 into dbt-labs:dev/margaret-mead Jan 21, 2021
@sdebruyn sdebruyn deleted the fix-issue-debug-exit-code branch January 21, 2021 15:36
@anthonymichaelclark
Copy link

anthonymichaelclark commented Apr 23, 2021

At the moment, yes . I and other devs run tests in docker container and use python's pdb for debugging. We can definitely make this experience more IDE friendly, I use VSCode at the moment so I feel the pain as well.

@kwigley @jtcohen6 Any idea whether integration tests still have to be run inside Docker? If so, can you explain how you actually accomplish this in greater detail? Are you using Dockerfile.test to build your test container? I suspect I'm doing something wrong here, but trying to run the docker-compose.yml is giving me this error:

ERROR: Encountered errors while bringing up the project.

And when I build just the test container from Dockerfile.test, I cannot in fact access the contents of /root:

bash: cd: /root: Permission denied

I can docker run the test container with --user=root but I'm missing a /root/.virtualenvs directory which I assume the docker-compose creates...

Apologies if I'm making some unlearned Docker mistakes here, took a look at the bit about setting up an env/running tests in the contributing guide but am still struggling to actually run any of the integration tests

@jtcohen6
Copy link
Contributor

@anthonymichaelclark We've significantly improved the way to run integration tests over the past few months, so it's no longer strictly necessary to run them via Docker. (You still can if you choose to.) Check out the current version of CONTRIBUTING.md, which details the available commands, including many handy make shortcuts.

@anthonymichaelclark
Copy link

@anthonymichaelclark We've significantly improved the way to run integration tests over the past few months, so it's no longer strictly necessary to run them via Docker. (You still can if you choose to.) Check out the current version of CONTRIBUTING.md, which details the available commands, including many handy make shortcuts.

@jtcohen6 Thanks for the reply -- so I should have been clearer, I've been through the Contributing doc (and I apologize in advance if I'm overlooking something there that's going to answer my questions), I can successfully run the postgres integration tests covered there. I'm struggling to run the BigQuery integration tests, many of which are producing the same FileExistsError that sdebruyn mentioned above. Is it still the case the BQ integrations tests need to be run in a container and if so, any pointers on how I get them to actually run?

@jtcohen6
Copy link
Contributor

@anthonymichaelclark Got it, I'm not sure what would be causing the FileExistsError (or what was causing it above). Locally, I'm able to run BQ integration tests via pytest. If that's not working, though, you should be able to run them via Docker, since that's how they run in CI:

docker-compose run test tox -p -e py36-bigquery -- -v -n4

@anthonymichaelclark
Copy link

@jtcohen6 Got to the bottom of it, figured I'd share the details in case someone besides sdebruyn and I run into it: somehow I had a directory called logs created in test/integration/022_bigquery_test. This breaks symlinking in the test setUp:

https:/fishtown-analytics/dbt/blob/33dc970859f5535610b2335f8cad5369095a686c/test/integration/base.py#L341-L348

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.

debug exit code does not match connection test
4 participants