init push up of converted unique_key tests #4958

McKnight-42 · 2022-03-25T16:51:50Z

resolves #4882

Description

Converting the new unique_id_as_list tests to new pytest version in core and then in each adapter as needed.

Checklist

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 added information about my change to be included in the CHANGELOG.

github-actions · 2022-03-25T16:52:08Z

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.

McKnight-42 · 2022-03-25T16:52:43Z

TODO: update changelog before final merge

…unique_key

jtcohen6 · 2022-03-26T14:03:39Z

@McKnight-42 Very glad to see an early draft PR!

There's a namespacing question here:

Do we want this test to live in tests/adapter/basic, alongside the "baseline" dbt behavior? You must be this tall to call yourself a dbt adapter
Or still within the "adapter zone," but in a way that signals it's more of an opt-in? tests/adapter/optional, tests/adapter/advanced, ...?

@gshank @ChenyuLInx Definitely interested in hearing your thoughts. This distinction is something we'd want to reflect in our documentation as well (dbt-labs/docs.getdbt.com#1263). It also leads me to wonder if there's some way to "auto-document" the set of test cases available in tests/adapter? A programmatic update to the module README?

gshank · 2022-03-28T20:21:22Z

That's a good question about where it should live. At some point we probably do want to put the required tests into one directory. Adapter tests will have to end up in the "adapter zone", which is in a directory in tests/adapter/dbt/tests/adapter. That's where they have to be in order to be subclasses in the adapter repos. So right now I'd put them in tests/adapter/dbt/tests/adapter/incremental_unique_id. Then when we create override tests they can be in tests/functional/adapter/test_incremental_unique_id.py. We might want to reorganize them later.

…pter zone

gshank · 2022-03-28T20:24:44Z

The idea is that tests/functional will have various directories of tests, including ones that are specific to that repo, but that tests that are overridden would be in tests/functional/adapter. That was my initial idea, anyway, but if someone has a better idea, please suggest it.

McKnight-42 · 2022-03-28T20:25:48Z

That's a good question about where it should live. At some point we probably do want to put the required tests into one directory. Adapter tests will have to end up in the "adapter zone", which is in a directory in tests/adapter/dbt/tests/adapter. That's where they have to be in order to be subclasses in the adapter repos. So right now I'd put them in tests/adapter/dbt/tests/adapter/incremental_unique_id. Then when we create override tests they can be in tests/functional/adapter/test_incremental_unique_id.py. We might want to reorganize them later.

Moved test up one directory to be on level with with basic. will keep looking into the errors that are popping up, have we picked the first adapter we want to convert in?

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py

gshank

The tests need to be in the base test class so they can be importable.

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py

gshank · 2022-03-28T20:47:42Z

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py

+        return run_result.status, run_result.message
+
+
+class TestIncrementalWithoutUniqueKey(IncrementalUniqueKeyBase):


None of these tests are actually importable, because they start with the name 'Test'. Why aren't the tests in the base test class?

the original tests were separated out into classes that inherited the setup based on type of unique_key case it was testing. do we have a preferred way to reformat the test/class names?

moved all tests to the base class and created a new test class based on the other examples we have.

Ah, this conversation clarified for me why we're doing it like this:

dbt-core/tests/adapter/dbt/tests/adapter/basic/test_base.py

Lines 109 to 110 in 5071b00

class TestSimpleMaterializations(BaseSimpleMaterializations):

pass

As I understand it:

By default, pytest only runs tests if the method starts with test_, and the class starts with Test, in a file whose name starts with test_

Our move has been to create a "base" class (not named Test), with test methods defined on it that do start with test_*

Then, inherit that "base" class into an actual test class, whose name starts with Test, and either pass (no changes needed) or override/reimplement fixtures/methods from the base class

Yes, that's correct :-). You can import a base class that starts with Test, the problem is that pytest would then run all of the tests in the base class plus all of those same tests in the subclass. Which means that if you need to override a test method or get failures, you'd get failures.

test/integration/076_incremental_unique_id_test/test_incremental_unique_id.py

jtcohen6 · 2022-03-29T10:03:08Z

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py

+    state::varchar(2) as state,
+    county::varchar(12) as county,
+    city::varchar(12) as city,


We'll want to be thoughtful about data types in order to get this running successfully on multiple databases. E.g. BigQuery doesn't support a data type named varchar(length). Our options here:

Override/reimplement all fixtures when we run this test in dbt-bigquery (ugh)

Remove type casts here, try to handle all data type business via seeds instead. That may also require a different approach for add_new_rows.sql/duplicate_insert.sql

These tests are also working on the adapter repos? Did you change the datatypes?

We could provide some of that information (with data_types) in a custom fixture. Avoiding that would be even better. There is actually an update_rows function in dbt.tests.util which came from dbt-adapter-tests and does work on all of our repos because it calls an adapter method.

Also, I think putting the test settings in variables and then putting them into the structures isn't the best practice. I'm gong to look at it a bit more to come up with recommendations.

These tests are also working on the adapter repos? Did you change the datatype

Yes, I believe there are slight modifications for each adapter repo

There is actually an update_rows function in dbt.tests.util which came from dbt-adapter-tests and does work on all of our repos because it calls an adapter method.

I'm a bit skeptical of this method tbh (I know it required us to override a data type in Redshift for basic tests), and want to discuss alongside table comparison logic—but maybe it's the way to go!

Maybe the move should be:

Once this test case is passing in this repo (for Postgres), we try running it (from this branch) in a branch of each plugin repo as well

Update the test cases here, override them there, get them all passing for all plugins

Fine-tune the framework so that the overrides feel minimal + appropriate

Merge across the board (and delete all the previously duplicated test code)

That sounds like the right process.

The override in Redshift for a seed data column was because the test goes and updates the name by appending '_update'. Redshift created the seed table with a varchar datatype that wasn't big enough. The best solution might actually have been to change that update so the size didn't increase, but the 'update_rows' method was kind of fiddly.

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py

…unique_key

McKnight-42 · 2022-03-30T21:36:44Z

dbt-labs/dbt-redshift#92 redshift seems to be passing tests locally as we would hope

gshank · 2022-03-31T16:24:02Z

Let's put 'test_incremental_unique_id.py' into an 'incremental' directory. Then when we convert the other incremental tests they will all be in the same directory. This will reduce the number of files that adapter repos will have to create to pull in adapter tests.

gshank · 2022-03-31T16:19:06Z

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py

+        return run_result.status, run_result.message
+
+    # no unique_key test
+    def test__no_unique_keys(self, project):


I think the factoring in the way these tests are done could be more straightforward and easier to read. I would recommend that you do something like:

def test__no_unique_keys(self, project): """with no unique keys, seed and model should match""" expected_fields = self.get_expected_fields(relation="seed", seed_rows=8) test_case_fields = self.get_test_fields( project, seed="seed", incremental_model="no_unique_key", update_sql_file="add_new_rows" ) self.check_scenario_correctness(project, expected_fields, test_case_fields)

Rename stub_expected_fields to 'get_expected_fields'. Rename 'setup_test' to 'get_test_fields'. Refactor get_test_fields to return the ResultHolder object and change the signature to use keywords, except for project, which should be first.

have pushed up a new version if we think of better ways to handle it from here on willing to keep iterating, have all tests passing in postgres

McKnight-42 · 2022-03-31T16:31:52Z

Let's put 'test_incremental_unique_id.py' into an 'incremental' directory. Then when we convert the other incremental tests they will all be in the same directory. This will reduce the number of files that adapter repos will have to create to pull in adapter tests.

would this be within the dbt/tests/adapter area or would we want it on same level as functional and the other directories?

…of leftover breakpoint

gshank · 2022-03-31T22:09:58Z

Within the adapter zone, so tests/adapter/dbt/tests/adapter/incremental

ChenyuLInx · 2022-04-01T01:30:13Z

Let's put 'test_incremental_unique_id.py' into an 'incremental' directory. Then when we convert the other incremental tests they will all be in the same directory. This will reduce the number of files that adapter repos will have to create to pull in adapter tests.

would this be within the dbt/tests/adapter area or would we want it on same level as functional and the other directories?

I believe we probably want the level like what Gerda suggested and for other repos, it would be just one file running all of the tests for 'incremental'.

gshank

Looks good!

…ns_equal instead

McKnight-42 · 2022-04-04T15:45:12Z

Trying to swap to the check_relations_equal implementation.

gshank

Looks good! Everything is passing. Just remove the commented out lines :-)

McKnight-42 · 2022-04-07T16:19:58Z

@cla-bot[bot] check

cla-bot · 2022-04-07T16:20:02Z

The cla-bot has been summoned, and re-checked this pull request!

* init push up of converted unique_key tests * testing cause of failure * adding changelog entry * moving non basic test up one directory to be more broadly part of adapter zone * minor changes to the bad_unique_key tests * removed unused fixture * moving tests to base class and inheriting in a simple class * taking in chenyu's changes to fixtures * remove older test_unique_key tests * removed commented out code * uncommenting seed_count * v2 based on feedback for base version of testing, plus small removal of leftover breakpoint * create incremental test directory in adapter zone * commenting out TableComparision and trying to implement check_relations_equal instead * remove unused commented out code * changing cast for date to fix test to work on bigquery

init push up of converted unique_key tests

17b5d11

cla-bot bot added the cla:yes label Mar 25, 2022

Merge branch 'main' of github.com:dbt-labs/dbt into mcknight/convert_…

1e52c9c

…unique_key

McKnight-42 added 2 commits March 28, 2022 11:33

testing cause of failure

bd15abf

adding changelog entry

f8d93b0

McKnight-42 marked this pull request as ready for review March 28, 2022 20:14

McKnight-42 requested a review from a team as a code owner March 28, 2022 20:14

McKnight-42 requested a review from ChenyuLInx March 28, 2022 20:14

moving non basic test up one directory to be more broadly part of ada…

3dec58f

…pter zone

gshank reviewed Mar 28, 2022

View reviewed changes

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py Outdated Show resolved Hide resolved

minor changes to the bad_unique_key tests

d3451a6

McKnight-42 requested a review from gshank March 28, 2022 20:48

gshank requested changes Mar 28, 2022

View reviewed changes

McKnight-42 added 2 commits March 28, 2022 16:02

removed unused fixture

053910c

moving tests to base class and inheriting in a simple class

ae12ccf

McKnight-42 requested a review from gshank March 28, 2022 21:28

taking in chenyu's changes to fixtures

970d57d

jtcohen6 reviewed Mar 29, 2022

View reviewed changes

McKnight-42 added 2 commits March 29, 2022 15:50

remove older test_unique_key tests

50c7ba1

removed commented out code

b8f7914

jtcohen6 reviewed Mar 30, 2022

View reviewed changes

tests/adapter/dbt/tests/adapter/test_incremental_unique_id.py Outdated Show resolved Hide resolved

McKnight-42 added 2 commits March 30, 2022 09:57

uncommenting seed_count

9282b4c

Merge branch 'main' of github.com:dbt-labs/dbt into mcknight/convert_…

0b26360

…unique_key

gshank requested changes Mar 31, 2022

View reviewed changes

v2 based on feedback for base version of testing, plus small removal …

f430efc

…of leftover breakpoint

McKnight-42 requested a review from gshank March 31, 2022 22:08

jtcohen6 mentioned this pull request Apr 1, 2022

Port testing framework changes. databricks/dbt-databricks#70

Merged

create incremental test directory in adapter zone

32f0a30

gshank approved these changes Apr 1, 2022

View reviewed changes

jtcohen6 mentioned this pull request Apr 1, 2022

init push of unique key test reformat for redshift dbt-labs/dbt-redshift#92

Merged

4 tasks

McKnight-42 self-assigned this Apr 4, 2022

commenting out TableComparision and trying to implement check_relatio…

f75b2ff

…ns_equal instead

McKnight-42 requested a review from gshank April 4, 2022 15:44

gshank approved these changes Apr 4, 2022

View reviewed changes

McKnight-42 added 2 commits April 4, 2022 11:40

remove unused commented out code

934d2f9

changing cast for date to fix test to work on bigquery

2693ab8

McKnight-42 closed this Apr 6, 2022

McKnight-42 reopened this Apr 6, 2022

McKnight-42 merged commit 3ade206 into main Apr 7, 2022

McKnight-42 deleted the mcknight/convert_unique_key branch April 7, 2022 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init push up of converted unique_key tests #4958

init push up of converted unique_key tests #4958

McKnight-42 commented Mar 25, 2022 •

edited

Loading

github-actions bot commented Mar 25, 2022

McKnight-42 commented Mar 25, 2022

jtcohen6 commented Mar 26, 2022 •

edited

Loading

gshank commented Mar 28, 2022

gshank commented Mar 28, 2022

McKnight-42 commented Mar 28, 2022

gshank left a comment

gshank Mar 28, 2022

McKnight-42 Mar 28, 2022

McKnight-42 Mar 28, 2022

jtcohen6 Mar 29, 2022

gshank Mar 29, 2022

jtcohen6 Mar 29, 2022

gshank Mar 29, 2022

gshank Mar 29, 2022

jtcohen6 Mar 29, 2022

gshank Mar 30, 2022

McKnight-42 commented Mar 30, 2022

gshank commented Mar 31, 2022

gshank Mar 31, 2022

gshank Mar 31, 2022

McKnight-42 Mar 31, 2022

McKnight-42 commented Mar 31, 2022

gshank commented Mar 31, 2022

ChenyuLInx commented Apr 1, 2022

gshank left a comment

McKnight-42 commented Apr 4, 2022

gshank left a comment

McKnight-42 commented Apr 7, 2022

cla-bot bot commented Apr 7, 2022

		return run_result.status, run_result.message


		class TestIncrementalWithoutUniqueKey(IncrementalUniqueKeyBase):

	class TestSimpleMaterializations(BaseSimpleMaterializations):
	pass

init push up of converted unique_key tests #4958

init push up of converted unique_key tests #4958

Conversation

McKnight-42 commented Mar 25, 2022 • edited Loading

Description

Checklist

github-actions bot commented Mar 25, 2022

McKnight-42 commented Mar 25, 2022

jtcohen6 commented Mar 26, 2022 • edited Loading

gshank commented Mar 28, 2022

gshank commented Mar 28, 2022

McKnight-42 commented Mar 28, 2022

gshank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McKnight-42 commented Mar 30, 2022

gshank commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McKnight-42 commented Mar 31, 2022

gshank commented Mar 31, 2022

ChenyuLInx commented Apr 1, 2022

gshank left a comment

Choose a reason for hiding this comment

McKnight-42 commented Apr 4, 2022

gshank left a comment

Choose a reason for hiding this comment

McKnight-42 commented Apr 7, 2022

cla-bot bot commented Apr 7, 2022

McKnight-42 commented Mar 25, 2022 •

edited

Loading

jtcohen6 commented Mar 26, 2022 •

edited

Loading