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

[CT-1630] Convert column_types test for dbt-bigquery #476

Merged
merged 7 commits into from
Jan 26, 2023

Conversation

McKnight-42
Copy link
Contributor

@McKnight-42 McKnight-42 commented Jan 24, 2023

resolves #dbt-labs/dbt-core#6404

Description

convesion of 056 to new functional test method

todo before merge: revert dev_requirements dbt-core pin

Checklist

@McKnight-42 McKnight-42 added the Skip Changelog Skips GHA to check for changelog file label Jan 24, 2023
@McKnight-42 McKnight-42 self-assigned this Jan 24, 2023
@cla-bot cla-bot bot added the cla:yes label Jan 24, 2023
@McKnight-42 McKnight-42 marked this pull request as ready for review January 24, 2023 17:00
@@ -0,0 +1,34 @@
import pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to separate this from test_alter_column_types.py? The two fixtures (module level variables) are copies of fixtures in the other file. So the only new content is the class with two methods.

I would keep the name test_column_types.py as it's more generic, so I guess I'm saying to move the other file's contents here technically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are separated for now as it is only currently tested in bigquery and is how it was historically, this does also raise a fine question of how big of a change in functionality a test is doing should they be before they are their own file/test but still grouped together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest just revising this to have the fixtures in the same file, then import them, thereby reducing the duplicate code. Best of both worlds.

Nit: do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few very slight differences in the fixtures for each test, one easily fixed by a project_config_update in that we materialize alt model as a table, the second for the schema being a change in the int64 type

@McKnight-42 McKnight-42 merged commit 368742d into main Jan 26, 2023
@McKnight-42 McKnight-42 deleted the mcknight/bq-ct-1630 branch January 26, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants