Ct 1629/052 column quoting tests conversion

[VersusFacit]

resolves #6403

Description

Test conversion.

Do we want to see these quoting tests ported to any other DBs? I suspect "no" because they were quoting rules specifically for Postgres. We should instead go write quoting specific tests for adapters for which they don't already exist, right?

Also, just a lil' shoutout: On my last test conversion, @mikealfare suggested I look into the parameterize decorator, which I was able to use in this PR to help things be more pytest like. Just had to discover the nearly undocumented magic of indirect=True and request.param alas, this is not needed in the current circumstance, but the magic lives on in commit history

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]

Do we want to see these quoting tests ported to any other DBs? I suspect "no" because they were quoting rules specifically for Postgres. We should instead go write quoting specific tests for adapters for which they don't already exist, right?

These tests do exist in our adapter plugins today, having been copy-pasted there from this repo back in 2021:

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

We should pursue one of the following options:

1. Replace those copy-pasted tests with this converted test (inherited, modified if needed)
2. Delete all those tests in those repos, because the behavior being tested is not adapter-specific / adapter-contingent

Let's make a call one way or the other. I do know that different databases have, at the very least, different characters used for quoting columns/identifiers (e.g. " on Snowflake, backticks on BigQuery), and that quoting columns/identifiers on Snowflake also makes them case-sensitive. From quickly looking at the tests defined in those repos, I would guess that the "strategy" conditional behavior (meaning incremental_strategy) was added because it's relevant on databases (Snowflake, BigQuery) that support a merge incremental strategy.

[VersusFacit]

I'm opting for 2 here because if we really need to test those backticks, we can using tests specifically for that.

Good catch on the strategy too. I'll chop that out.

[colin-rogers-dbt]

if this is postgres specific can we indicate that in some way? Either in the test file name or folder structure (that way we prevent having these be copy pasted again)

[mikealfare]

if this is postgres specific can we indicate that in some way? Either in the test file name or folder structure (that way we prevent having these be copy pasted again)

I would vote the latter. If I understand correctly, we want to eventually create a dbt-postgres adapter that is separate from dbt-core. The migration will be easier to do if we isolate the postgres-specific items higher up in the directory structure.

[VersusFacit]

@colin-rogers-dbt Sorry if I'm misreading here, but I thought this specifically was not postgres-specific, rather this is a general test for column quoting functionality. My question was whether it was Postgres specific (and we decided it's not).

[dbeatty10]

My vote for the inherit-modify pattern

My default opinion is for this option:

1. Replace those copy-pasted tests with this converted test (inherited, modified if needed)

Reasons:

1. The inherit-modify pattern is often dead simple for the maintainer to use
2. The alternatives aren't that great

Inherit-modify is often simple

Inheriting a test within an adapter is often as simple as this:

from dbt.tests.adapter.utils.test_any_value import BaseAnyValue

class TestAnyValue(BaseAnyValue):
    pass

The inherit-modify pattern is a boon for adapter maintainers -- by doing very little, they can proactively see if there's a known issue (with their quoting logic, for example).

Alternatives aren't great

In the alternatives to the inherit-modify pattern, the maintainer(s) need to:

• have an amazing imagination for test cases of things that can go wrong; and/or
• be reactive by receiving and responding to bug reports.

Both those options feel like a method of last resort rather than our best bet.

This particular test

Let's consider the following proposal for this particular test of enabling/disabling quote_columns:

2. Delete all those tests in those repos, because the behavior being tested is not adapter-specific / adapter-contingent

Is the test written in an adapter-agnostic manner? Yeah! It looks like it would work across a wide variety of adapters, even if their quoting characters were single quotes, backticks, or the 🧠 emoji.

Would the results of the test be adapter-specific / adapter-contingent? I suspect they would! Depending on all the various knobs and levers an adapter maintainer can turn and pull, inheriting this test could help them quickly see if they have an unintended consequence, like breaking quote_columns.

Conclusion

I'm happy to be wrong about if this test happens to have truly adapter-contingent results or not -- it's actually irrelevant to the crux of my opinion. My main point is that we want to do as much as possible to help maintainers see a potential problem that it would be hard for them to foresee on their own, and the inherit-modify pattern does just that.

[VersusFacit]

Chitchatted about this elsewhere. We're going to take this into a bigger convo since this is super important in designing out the adapter test suite. But for now, I'm going to merge this, and wrap up this conversion.