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

mv on_schema_change tests -> "adapter zone" #6618

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Jan 14, 2023

follow-up to #6330

Description

In the previous conversion PR, @VersusFacit made a reasonable point:

Oh, do we want these imported into any repos? Incremental tests have so many caveats, I'm unsure a direct import is valuable in this case.

All I did was convert the postgres__ tests to ordinary tests to live in tests/functional

As it stands, that these tests have already been living in those other repos, and they were originally written (in #3387) to be cross-adapter compatible. Then, they were copy-pasted everywhere, as part of the Great Adapter Split, in fall 2021 / the lead-up to v1.0.

By moving this test into the "adapter zone" (a.k.a. dbt-tests-adapter), we can inherit it for use in all our adapter repos, and delete a LOT of copy-pasted code.

While here:

  • Replace the hardcoded string_type with dbt.type_string() (added in v1.2!)
  • Refactor the test setup very slightly so it's easier to extend/repurpose (relevant to dbt-bigquery), without having to run existing test functions multiple times
  • Remove the test fixtures, since they were actually unused in the test
  • Do a little bit of file renaming / reorg within dbt-tests-adapter (which is v1 distributed software!)

Adapter PRs

Checklist

@jtcohen6 jtcohen6 requested review from a team as code owners January 14, 2023 14:21
@jtcohen6 jtcohen6 added the Skip Changelog Skips GHA to check for changelog file label Jan 14, 2023
@cla-bot cla-bot bot added the cla:yes label Jan 14, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM!

# Tests
#

_TESTS__SELECT_FROM_INCREMENTAL_IGNORE = """
Copy link
Contributor

@VersusFacit VersusFacit Jan 19, 2023

Choose a reason for hiding this comment

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

Clearly from my more naive days of test conversions :p

That said, I do recall thinking "should we keep these?" I know they live in the code history and don't necessary need to exist in current code. More a "principles" question: if there are seemingly extraneously models in tests, do we nix them with cautious abandon or seek to add tests to include them? Of course, we merged this without getting much of answer at the time, and I'd love to hear your thoughts if you have any now. 🙇‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a general rule: If it's not being actively used in one of our existing tests, we probably don't need it. (There are rare exceptions, where we've added a new fixture/combination with the intent of testing, but completely forgot! I just found an instance where I'd made this mistake a ~while ago: dbt-labs/dbt-bigquery#41)

Longer answer: Intent is a useful input, to the extent we can infer it. In this case, I remember the history as being: our colleague Matt W added this feature (on_schema_change) back in 2021, and he added integration tests for it by copying + adapting some other recent integration tests (for --store-failures). The "tests" component was important for the original tests, but not necessary for these adapted ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciate the history lesson! That makes much more sense and I'll run with your heuristic in the future.

Copy link
Contributor

@VersusFacit VersusFacit left a comment

Choose a reason for hiding this comment

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

Barring the merge conflicts, and my lil' comment, looks good.

Thanks for introducing me to that neat dbt.type_string().

@jtcohen6 jtcohen6 force-pushed the jerco/mv-onschemachange-adapter-zone branch from 6716836 to 096e520 Compare January 19, 2023 08:39
@jtcohen6 jtcohen6 merged commit b05582d into main Jan 19, 2023
@jtcohen6 jtcohen6 deleted the jerco/mv-onschemachange-adapter-zone branch January 19, 2023 09:13
colin-rogers-dbt pushed a commit that referenced this pull request Jan 20, 2023
* Mv incremental on_schema_change tests to 'adapter zone'

* Use type_string()

* Cleanup
colin-rogers-dbt pushed a commit that referenced this pull request Jan 20, 2023
* Mv incremental on_schema_change tests to 'adapter zone'

* Use type_string()

* Cleanup
colin-rogers-dbt added a commit that referenced this pull request Jan 20, 2023
* mv `on_schema_change` tests -> "adapter zone" (#6618)

* Mv incremental on_schema_change tests to 'adapter zone'

* Use type_string()

* Cleanup

* mv `on_schema_change` tests -> "adapter zone" (#6618)

* Mv incremental on_schema_change tests to 'adapter zone'

* Use type_string()

* Cleanup

Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
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.

3 participants