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 1.8 and test coverage updates #513

Merged
merged 8 commits into from
Aug 23, 2024

Conversation

cody-scott
Copy link
Collaborator

Updates the adapter to support 1.8 and implements most of the dbt-test-adapters tests to fix the coverage issues.

Additionally fixes some regressions created from the 1.4 => 1.7 move.

This merge will likely collide with other PRs including #502, #504, #474, #487

Tests

The test coverage has intentionally not inherited from any fabric tests; only importing from the core dbt-test-adapter.

The key idea is if the fabric adapter makes a breaking change, this adapter should fail loudly with respect to their changes, instead of being masked by their own coverage passing.

Coverage is passing, so outstanding issues such as #481 should be able to be better addressed, if they are still failing post update.

@cody-scott
Copy link
Collaborator Author

@schlich Can you take a look at this PR please?

This should implement much better coverage of tests and bring the adapter to 1.8

@schlich
Copy link
Contributor

schlich commented Aug 21, 2024

@cody-scott went ahead and ran CI, let me know if there's anything you want eyes on in particular otherwise i trust your judgement!

@schlich
Copy link
Contributor

schlich commented Aug 22, 2024

hey @cody-scott i suggest importing the futures annotations instead of reverting to the old typing syntax... just pushed to my branch a few minutes ago and it's running in CI, i'll leave it up to you to merge my changes w yours

@cody-scott
Copy link
Collaborator Author

@schlich I'd be OK with keeping the old type syntax for now, or at least until DBT formally drops support for 3.8.

Its passing in the current state, so i'd be open to releasing this version as a pre-release for feedback over the next week, then once that is good adopting other changes. Thoughts?

Its still looking like that | causes failures in the test jobs on the #514 checks.

@schlich
Copy link
Contributor

schlich commented Aug 22, 2024

Sounds fine by me!

@cody-scott
Copy link
Collaborator Author

For a pre-release do you merge the changes then push a release as pre with this off the main/master branch? What is the normal flow for this one?

@schlich
Copy link
Contributor

schlich commented Aug 22, 2024

Yeah you've pretty much got it, it's been a minute and it was a bit of trial and error with the 2 releases i did but i think this one was relatively clean: #483

IIRC i just merged all the changes then opened a release PR w/its own release commit that updated the version number and I think CI/CD took care of the rest.

@cody-scott cody-scott merged commit 09b577b into dbt-msft:master Aug 23, 2024
53 checks passed
@ms32035 ms32035 mentioned this pull request Sep 6, 2024
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