Remove TableComparison and convert existing calls to use dbt.tests.util #4986

gshank · 2022-04-01T19:46:15Z

resolves #4778

Description

Remove TableComparison and the dbt.test.tables file. Replace existing calls to TableComparison to functions in dbt.tests.util which were converted from dbt-adapter-tests. Add some comments.

I fixed a commented out assert in the dbt_run function, and had to update a bunch of tests because of that.

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.

dbt.tests.util. Also update tests for accidentally commented out assert in dbt_run.

github-actions · 2022-04-01T19:46:33Z

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.

gshank · 2022-04-01T19:55:07Z

This pr is not complete--I'm still looking at the run_sql piece--but I wanted to get it out for comment.

jtcohen6

@gshank Very impressive work! I'm glad to see this code rightfully retiring. It served us well enough, for a long time, and need serve no longer :)

I used this branch to run our "basic" functional testing suite against dbt-redshift, dbt-snowflake, dbt-bigquery, and dbt-databricks. All passed. This does make me yearn for an easy way to test changes in a dbt-core branch by triggering GHAs in plugins: #4988

jtcohen6 · 2022-04-03T12:00:06Z

core/dbt/tests/util.py

+# Test utilities
+#   run_dbt
+#   run_dbt_and_capture
+#   get_manifest
+#   copy_file
+#   rm_file
+#   write_file
+#   read_file
+#   get_artifact
+#   update_config_file
+#   get_unique_ids_in_results
+#   check_results_nodes_by_name
+#   check_result_nodes_by_unique_id
+
+# SQL related utilities
+#   run_sql_with_adapter
+#   relation_from_name
+#   check_relation_types (table/view)
+#   check_relations_equal
+#   check_relations_equal_with_relations
+#   check_table_does_exist
+#   check_table_does_not_exist
+#   get_relation_columns
+#   update_rows
+#      generate_update_clause


Which of these do you feel comfortable documenting, as "public" methods? Are there any we want to keep "private"? (The current draft docs mention only run_dbt)

I think 'get_artifact' should be okay to document. I'm not totally sure about 'get_manifest', mainly because it will load an internal version of the manifest that I'm not sure we want people to rely on. Most of the rest of the test utilities are pretty innocuous. I'm not totally clear on how useful they would be to users, because my imagination is that they'd be testing their own project, and most of those are helpful for modifying/updating a project.

in SQL related utilities, I think update_rows is still kind of funky, i.e. the interface is very specific to a couple of particular tests. And I'd like to look a bit more at run_sql_with_adapter (which is mostly designed to be used by the project.run_sql method). I think check_relations_equal, relation_from_name, check_relation_types, check_table_does_(not)_exist, and get_relation_columns are okay. We will be expecting adapter contributors to be dealing with those utilities. I'm not totally clear how much uptake they'd get with general users.

The principal intended user here is very much the adapter plugin maintainer, and I think that should inform how we publish/document the first cut of this interface. If it proves valuable for package/project maintainers as well, all the better.

Makes sense that get_manifest wants to be private, given that it's largely the output of parsing, and ought not vary across adapters

Agree that update_rows is not my favorite. I'd like to see all these methods refactored to use SQL wrapped in Jinja macros, rather than in Python f-strings. We can make that change independently of the test utilities, though.

jtcohen6 · 2022-04-03T12:00:41Z

core/dbt/tests/util.py

+# Uses:
+#    adapter.config.credentials
+#    adapter.quote
+#    adapter.run_sql_for_tests


Thanks for annotating these so clearly! Did you find a need for any totally new adapter methods?

Below I see we make use of some standard attributes (credentials, quote, Relation) and standard methods (get_colums_in_relation), as well as some slightly less-standard methods (run_sql_for_tests, get_rows_different_sql).

Many adapters won't need to reimplement the default versions, but we'll definitely want to enumerated all of them in our documentation.

core/dbt/tests/util.py

tests/functional/basic/test_copy_uppercase.py

ChenyuLInx

LGTM

core/dbt/tests/fixtures/project.py

ChenyuLInx · 2022-04-06T20:53:12Z

core/dbt/tests/fixtures/project.py

@@ -182,6 +228,7 @@ def adapter(unique_schema, project_root, profiles_root, profiles_yml, dbt_projec
    runtime_config = RuntimeConfig.from_args(args)
    register_adapter(runtime_config)
    adapter = get_adapter(runtime_config)
+    adapter.load_macro_manifest(base_macros_only=True)


Why we only load base macros here?

This particular adapter is only used until the first dbt command is executed, when a new adapter is built from the full project. For the initial create_schema call and the run_sql commands that might be run in a test prior to a dbt command, the base macros should be enough. I'm trying to limit the pieces of the project that are loaded here because everything used here has to actually work, so if you want to test bad project files or profiles or macros, they will have to be loaded later instead of when the project is initially constructed.

I've added some comments about it.

That makes total sense!! Thanks for explaining it!

ChenyuLInx · 2022-04-06T20:57:45Z

core/dbt/tests/fixtures/project.py

+            relation = self.adapter.Relation.create(
+                database=self.database, schema=self.test_schema
+            )
+            self.adapter.create_schema(relation)


Love the fact that we are using more adapter function here!

ChenyuLInx · 2022-04-06T20:58:54Z

core/dbt/tests/util.py

@@ -28,10 +56,11 @@ def run_dbt(args: List[str] = None, expect_pass=True):

    print("\n\nInvoking dbt with {}".format(args))
    res, success = handle_and_check(args)
-    #   assert success == expect_pass, "dbt exit state did not match expected"
+    assert success == expect_pass, "dbt exit state did not match expected"


This is super helpful for prevent unknown downstream check error!

ChenyuLInx · 2022-04-06T21:18:39Z

tests/functional/basic/test_simple_reference.py

+    check_relations_equal(project.adapter, ["summary_expected", "materialized_summary"])
+    check_relations_equal(project.adapter, ["summary_expected", "view_summary"])
+    check_relations_equal(project.adapter, ["summary_expected", "ephemeral_summary"])
+    check_relations_equal(project.adapter, ["summary_expected", "view_using_ref"])


Feels like we can merge all these checks into one check? Also okay to leave like this

Good point. I've updated those tests. No point in leaving bad examples out there.

ChenyuLInx · 2022-04-06T21:25:12Z

I think we need to wait for some update finish in spark and then update here before this can be merged right?

ChenyuLInx · 2022-04-07T15:01:09Z

core/dbt/tests/fixtures/project.py

+# The main functional test fixture is the 'project' fixture, which combines
+# other fixtures to write out a dbt_project in a temporary directory.


Suggested change

# The main functional test fixture is the 'project' fixture, which combines

# other fixtures to write out a dbt_project in a temporary directory.

# The main functional test fixture is the 'project' fixture, which combines

# other fixtures, write out a temp dbt_project in a directory, create a temp

# schema in the testing database, and return a `TestProjInfo` object that

# contains information about that temp dbt_project.

I've updated the comment.

…il (dbt-labs#4986)

Remove TableComparison and convert calls to use functions in

db57bcb

dbt.tests.util. Also update tests for accidentally commented out assert in dbt_run.

gshank requested a review from a team as a code owner April 1, 2022 19:46

cla-bot bot added the cla:yes label Apr 1, 2022

Add changie file

2ca9425

Minor cleanup

7e9cd5b

This was referenced Apr 1, 2022

[CT-281] Make obsolete the test compare tables code now in core/dbt/tests/tables.py #4778

Closed

use adapter to execute sql #4987

Closed

jtcohen6 mentioned this pull request Apr 3, 2022

[CT-449] Make it easy to run integration tests in an adapter plugin against a dbt-core PR #4988

Open

jtcohen6 reviewed Apr 3, 2022

View reviewed changes

gshank force-pushed the ct-281-table_comparison branch 5 times, most recently from ed7b2c1 to e0cccd7 Compare April 5, 2022 01:00

gshank requested a review from a team as a code owner April 5, 2022 01:00

gshank force-pushed the ct-281-table_comparison branch from e0cccd7 to ebff78f Compare April 5, 2022 01:40

Stop patching providers and retrieve current adapter instead

0770ed2

gshank force-pushed the ct-281-table_comparison branch from ebff78f to 0770ed2 Compare April 5, 2022 01:47

gshank added 2 commits April 4, 2022 23:28

Remove another patch of providers

4ee1485

Use macros for drop and create schema in project

4fd5a79

gshank requested a review from a team as a code owner April 5, 2022 21:06

gshank requested a review from VersusFacit April 5, 2022 21:06

gshank added 3 commits April 5, 2022 19:14

load_dependencies cleanup

9856fa9

Merge branch 'main' into ct-281-table_comparison

7bbc82e

Comments, cleanup run_sql_for_tests

fd1caf4

ChenyuLInx approved these changes Apr 6, 2022

View reviewed changes

Add comment about adapter fixture. Update test_simple_reference.

d88a63c

ChenyuLInx reviewed Apr 7, 2022

View reviewed changes

more comments

eaf7a2c

gshank merged commit 899b0ef into main Apr 7, 2022

gshank deleted the ct-281-table_comparison branch April 7, 2022 17:04

nathaniel-may mentioned this pull request Apr 8, 2022

Fix multi threaded test failures #5015

Closed

4 tasks

agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request May 20, 2022

Remove TableComparison and convert existing calls to use dbt.tests.ut…

5a08cc9

…il (dbt-labs#4986)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TableComparison and convert existing calls to use dbt.tests.util #4986

Remove TableComparison and convert existing calls to use dbt.tests.util #4986

gshank commented Apr 1, 2022

github-actions bot commented Apr 1, 2022

gshank commented Apr 1, 2022

jtcohen6 left a comment

jtcohen6 Apr 3, 2022

gshank Apr 4, 2022

jtcohen6 Apr 4, 2022

jtcohen6 Apr 3, 2022

ChenyuLInx left a comment

ChenyuLInx Apr 6, 2022

gshank Apr 7, 2022

gshank Apr 7, 2022

ChenyuLInx Apr 7, 2022

ChenyuLInx Apr 6, 2022

ChenyuLInx Apr 6, 2022

ChenyuLInx Apr 6, 2022

gshank Apr 7, 2022

ChenyuLInx commented Apr 6, 2022

ChenyuLInx Apr 7, 2022

gshank Apr 7, 2022

		# The main functional test fixture is the 'project' fixture, which combines
		# other fixtures to write out a dbt_project in a temporary directory.

Remove TableComparison and convert existing calls to use dbt.tests.util #4986

Remove TableComparison and convert existing calls to use dbt.tests.util #4986

Conversation

gshank commented Apr 1, 2022

Description

Checklist

github-actions bot commented Apr 1, 2022

gshank commented Apr 1, 2022

jtcohen6 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

ChenyuLInx 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

ChenyuLInx commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment