Ct 1827/064 column comments tests conversion

[VersusFacit]

resolves #6623

Description

Straightforward test conversion. Turned a private method into a fixture and requested it as is the pytest way.

We interested in porting any of these to adapters? I don't see an immediate reason to.

Checklist

• I have read the contributing guide and understand what's expected of me
• 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 opened an issue to add/update docs, or docs changes are not required/relevant for this PR

[jtcohen6]

We interested in porting any of these to adapters? I don't see an immediate reason to.

The legacy version of this test was copy-pasted over to dbt-redshift + dbt-snowflake back in 2021:

• https://github.com/dbt-labs/dbt-redshift/blob/main/tests/integration/column_comments_tests
• https://github.com/dbt-labs/dbt-snowflake/tree/main/tests/integration/column_comments_tests

We should either:

1. Replace them with the new converted test (inherited, modified if needed)
2. Delete them because this isn't adapter-specific behavior

It looks like this test was added as a new test back in #2733 (one of Gerda's very first contributions!) — but it's really an extension of tests for persist_docs, which you just converted (!) in #6409.

That one is running everywhere:

• https://github.com/dbt-labs/dbt-redshift/tree/main/tests/integration/persist_docs_tests
• https://github.com/dbt-labs/dbt-snowflake/tree/main/tests/integration/persist_docs_tests
• https://github.com/dbt-labs/dbt-bigquery/tree/main/tests/integration/persist_docs_tests
• https://github.com/dbt-labs/dbt-spark/tree/main/tests/integration/persist_docs

My vote would be:

• Move this column_comments test into the persist_docs tests
• Set up the new persist_docs tests to run in adapter plugins (replacing the current copy-pasted versions). This behavior varies a lot across different data platforms, and it requires some real implementation feats to provide end users with a consistent experience. E.g. on Snowflake we have to take a roundabout (and therefore riskier) approach for views, which don't support adding column-level comments after the view has been created.

[VersusFacit]

Good insight, @jtcohen6 . I'll opt for 2 since it seems the better option for the long term. But, I'm a bit shakey on the steps. Can you verify my plan looks right:

1. merge what I've written here into the persist_docs test
2. move that persist_docs test into the adapter zone
3. kick up PRs in each adapter to swap through the adapter zone tests for the copy-paste versions already present that you linked to

[jtcohen6]

@VersusFacit Those steps look exactly right to me!

[colin-rogers-dbt]

Good insight, @jtcohen6 . I'll opt for 2 since it seems the better option for the long term. But, I'm a bit shakey on the steps. Can you verify my plan looks right:

1. merge what I've written here into the persist_docs test
2. move that persist_docs test into the adapter zone
3. kick up PRs in each adapter to swap through the adapter zone tests for the copy-paste versions already present that you linked to

Generally don't we kick up PRs per adapter and point them at this branch of core before merging?

[jtcohen6]

Generally don't we kick up PRs per adapter and point them at this branch of core before merging?

@colin-rogers-dbt That's true! Here's how I understood the steps that @VersusFacit outlined:

1. Update this PR so that it's adding the "column comments" tests into the larger set of persist_docs tests, since they're all very closely related, and should never really have been created/stored separately. Then, merge this PR.
2. Open a new dbt-core PR that moves all persist_docs tests into the "adapter zone" (dbt-tests-adapter).
3. Kick up adapter PRs that inherit/run all persist_docs tests from the "adapter zone" (replacing the old unconverted ones that have been copy-pasted into each repo). Those adapter PRs should repoint to the core branch for step 2.

Steps 2 + 3 may require some "rinse + repeat" iteration, as you find that there are adapter-specific behaviors that warrant modifications to the "core" test definition.