Skip to content

Simplify running the integration tests #211

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

Merged
merged 2 commits into from
Apr 21, 2020

Conversation

bjgbeelen
Copy link

During the development and discussion of #205 I wanted to run the integration tests against a Spark backend. To make this happen, I made some changes presented here. While adding Spark integration is something for the long run, I already did some work that I think might be valuable for others users too.

It's twofold:

  • making it simpler to run the integration tests locally
  • and, also make sure that they mimick what is happening in CircleCI as best as possible.

I have not seen the results yet on CircleCI of course, so it might need some extra attention at things I overlooked

@clrcrl clrcrl requested a review from drewbanin April 17, 2020 14:34
@@ -11,7 +11,7 @@ integration_tests:
outputs:
postgres:
type: postgres
host: localhost
host: "{{ env_var('CI_DBT_HOST') }}"
Copy link
Author

Choose a reason for hiding this comment

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

This would mean btw that this environment variable would need to be set in CircleCI too, or we should put localhost as a default value, but that would not be my preference.

Now that I take another look on it, adding POSTGRES somewhere in these environment variables, e.g. CI_POSTGRES_DBT_HOST would probably make the naming more consistent in the profiles.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a sample file - the actual env var is set in the .circle/config.yml file! I'm supportive of this change

@drewbanin
Copy link
Contributor

hey @bjgbeelen - this is awesome!

Your PR here inspired me to play around with CircleCI's workflows. I made a branch for this over here: https://github.com/fishtown-analytics/dbt-utils/compare/pr/211-drew?expand=1

The key benefits are that:

  1. tests for each adapter can run in parallel
  2. we can enable build-on-PRs for PRs submitted from forks. We've historically disabled this because it's a vector for abuse, but I think workflows would let us at least kick off the Postgres tests here for forked PR builds

I'm pretty happy with this PR as-is. I'm going to do one more pass here, and then this will be good to merge :)

@bjgbeelen
Copy link
Author

Glad you like it! Sounds like great new benefits you're investigating!

Copy link
Contributor

@drewbanin drewbanin 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 really wonderful! Thanks for making this PR - it's very thoughtful and well put together. Going to merge this now :)

@@ -11,7 +11,7 @@ integration_tests:
outputs:
postgres:
type: postgres
host: localhost
host: "{{ env_var('CI_DBT_HOST') }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a sample file - the actual env var is set in the .circle/config.yml file! I'm supportive of this change

@drewbanin drewbanin merged commit 99d44a3 into dbt-labs:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants