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 tests and CI #258

Merged
merged 14 commits into from
Nov 9, 2023
Merged

Add tests and CI #258

merged 14 commits into from
Nov 9, 2023

Conversation

moomindani
Copy link
Collaborator

resolves #240

Description

Added CI

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-glue next" section.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@moomindani moomindani changed the title Added CI [WiP] Add CI Nov 8, 2023
@moomindani moomindani changed the title [WiP] Add CI Add CI Nov 8, 2023
@moomindani moomindani changed the title Add CI [WiP] Add CI Nov 8, 2023
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@nicor88
Copy link
Contributor

nicor88 commented Nov 8, 2023

As a general note I recommend this approach to use in your CI:

  • unit tests - where you mock as much as possible everything that you need, e.g. using moto. unit tests do not need a real account to run, and they can be run easily even locally.
  • functional tests, they implement dbt-tests-adapter tests, so pretty much a real case scenario test with real models. Those tests at the contrary require a real AWS account with some infra to run on. In dbt-athena we run those conditionally in the CI (for cost reasons), but they should be separated from unit tests. I do not see any functional tests added here, and without those, you miss a big part of it.

@moomindani moomindani changed the title [WiP] Add CI Add tests and CI Nov 9, 2023
@moomindani
Copy link
Collaborator Author

As a general note I recommend this approach to use in your CI:

* unit tests - where you mock as much as possible everything that you need, e.g. using moto. unit tests do not need a real account to run, and they can be run easily even locally.

* functional tests, they implement dbt-tests-adapter tests, so pretty much a real case scenario test with real models. Those tests at the contrary require a real AWS account with some infra to run on. In dbt-athena we run those conditionally in the CI (for cost reasons), but they should be separated from unit tests. I do not see any functional tests added here, and without those, you miss a big part of it.

Thanks for the feedback. It was planned but not added yet yesterday.
Today I have separated tests into unit test and functional test, and used AWS account only for the functional test.

Copy link
Contributor

@aajisaka aajisaka 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 to me. Thank you!

@menuetb menuetb merged commit 1b1578f into aws-samples:main Nov 9, 2023
17 checks passed
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.

support CI for dbt-adapter-test suite
4 participants