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

dev env clean up and improvements #3204

Merged
merged 9 commits into from
Mar 29, 2021
Merged

dev env clean up and improvements #3204

merged 9 commits into from
Mar 29, 2021

Conversation

kwigley
Copy link
Contributor

@kwigley kwigley commented Mar 25, 2021

resolves #3194

Description

  • update development dependencies (default to using the latest and greatest)
  • update code and installed a new pytest plugin named pytest-dotenv to load test.env secrets in testing environment in order to avoid running integration tests in a docker container locally.
  • remove scripts/dtr.py because it is an abstraction we shouldn't need anymore, default to using pytest to run individual tests, tox to run full test suites, and Makefile for commonly used commands
  • update tox.ini with better defaults and test envs, we are really close to only needing python and tox installed on your machine to be able to do everything related to development with dbt :)
  • update Makefile to run target on local machine by default, not in a docker container. Still provided option to run in a docker container using USE_DOCKER=true in case folks don't have a certain python version installed.
  • update CONTRIBUTING.md to reflect changes
  • CI runs a lot fast

Future ideas

  • use tox to create development virtualenvs with everything necessary to develop (example tox --devenv env -e py38 is the same thing as python3.8 -m venv env && source env/bin/activate && pip install -r dev-requirements.txt -r editable-requirements.txt)
  • create a VSCode dev container for folks who use VSCode
  • remove initial step to setup postgres database for integration tests

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 Mar 25, 2021
@kwigley kwigley self-assigned this Mar 25, 2021
test/rpc/conftest.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

This is great! A lot of good cleanup. Found a couple of typos and was wondering if we could take this opportunity to change line length checks?

tox.ini Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

I'm actually unfamiliar with the vast majority of changes you're proposing here so I'm not hitting approve, but I looked through it all and didn't see any red flags.

we are really close to only needing python and tox installed on your machine to be able to do everything related to development with dbt
I am so excited for this future.

Very excited about these changes. Thanks for all the hard work!

name: Run tests
command: tox -e integration-postgres-py39
name: Postgres integration tests
command: tox -p -e py36-postgres,py38-postgres -- -v -n4
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice. This makes things much simpler. Good idea.

@@ -64,7 +64,7 @@ def read(fname):
'sqlparse>=0.2.3,<0.4',
'networkx>=2.3,<3',
'minimal-snowplow-tracker==0.0.2',
'colorama>=0.3.9,<0.4.4',
'colorama>=0.3.9,<0.4.5',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had to pin this before because the package didn't provide the source distribution on PyPI, but they have since added it! 👍 https://pypi.org/project/colorama/0.4.4/#files

@@ -149,15 +149,15 @@ def test_postgres_dbt_commands_with_cwd_as_project_dir(self):

@use_profile('postgres')
def test_postgres_dbt_commands_with_randomdir_as_project_dir(self):
workdir = os.getcwd()
workdir = self.test_root_dir
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change, integration tests failed on MacOS because of how the os handles temp dir/file. Functionally, os.getcwd() and self.test_root_dir are the same value. os.getcwd() returns a fully resolved filepath while self.test_root_dir is a string value pointing to a symlinked path that is equivalent to os.getcwd().

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.

This is amazing! 🎤 → ⬇️

I was able to run locally with pytest, and it was a breeze.

@@ -128,100 +127,105 @@ Docker and docker-compose are both used in testing. Specific instructions for yo

For testing, and later in the examples in this document, you may want to have `psql` available so you can poke around in the database and see what happened. We recommend that you use [homebrew](https://brew.sh/) for that on macOS, and your package manager on Linux. You can install any version of the postgres client that you'd like. On macOS, with homebrew setup, you can run:

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to change/edit/remove the section about docker above, given that it's no longer a requirement for folks to test locally? Do we still want to make it available as an option?

Copy link
Contributor Author

@kwigley kwigley Mar 29, 2021

Choose a reason for hiding this comment

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

Docker is still required to create and run a testing postgres database, the scripts to setup the database for testing (test/setup_db.sh) are specific to the database created by docker-compose. So it it still a requirement to setup a postgres database, but no longer needed to invoke tests locally. I'll try to make this more clear!

@kwigley kwigley force-pushed the updates/tox branch 2 times, most recently from 9176a35 to ba03988 Compare March 28, 2021 19:26
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.

tox updates
4 participants