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

remove source quoting setting in adapter tests #5839

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

sdebruyn
Copy link
Contributor

resolves #5836

Description

As discussed in #5836, the adapter tests should not override the quoting policies defined in the adapters. I removed all quoting policies from this the docs generate test.

Checklist

@sdebruyn sdebruyn requested a review from a team as a code owner September 14, 2022 17:39
@cla-bot cla-bot bot added the cla:yes label Sep 14, 2022
@sdebruyn
Copy link
Contributor Author

sdebruyn commented Sep 14, 2022

I just had the same build failure in the dbt-sqlserver GH Actions as this one, something we can do about it or just ignore and retry?

@ChenyuLInx
Copy link
Contributor

It looks like there's some issue regarding pulling down setup-tools, maybe just retry in a bit.

As for the change, they look good to me! @gshank what was the reason those quoting setting were added in the beginning, do we expect this to break tests in other adapter repos?

@ChenyuLInx ChenyuLInx requested a review from gshank September 15, 2022 16:11
@lostmygithubaccount lostmygithubaccount added ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering support_rotation A good task to pick up during support rotation labels Sep 16, 2022
@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label Sep 16, 2022
@gshank
Copy link
Contributor

gshank commented Sep 16, 2022

Setting the quoting was copied from the earlier integration tests. It's quite possible that it was originally there because we ran multiple adapter tests on the same test suite? I think we might want to check what this does to the tests in Redshift, BigQuery, and Snowflake before merging though. If it breaks those tests we'd have to quickly do PRs to fix.

@ChenyuLInx
Copy link
Contributor

Created a test pr to test this change in dbt-bigquery repo

@ChenyuLInx
Copy link
Contributor

The tests in test PR works as expected, so I think this is good to go. @sdebruyn Thanks for contributing it!!

@ChenyuLInx ChenyuLInx merged commit 3680b6a into dbt-labs:main Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering support_rotation A good task to pick up during support rotation Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1173] [Bug] Adapter doc tests override adapter quoting settings
5 participants