init push or pytest conversion of override_database for bigquery #165

McKnight-42 · 2022-04-21T20:34:41Z

resolves #164

Description

Converting the BigQuery implementation of override_database to the new pytest format

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 updated the CHANGELOG.md and added information about my change to the "dbt-bigquery next" section.

McKnight-42 · 2022-04-21T20:37:13Z

I have ran the conversion script and started playing around with the script to try and get it to work and have a question about weather or not there are additonal changes we might have to make around getting the env variables since these aren't just replacing values from dbt-core confftest.py

current log from when I run a test:


self = <test_override_database.TestModelOverride object at 0x7f9787cd4e90>

    def test_bigquery_database_override(self):
>       self.run_database_override()

tests/functional/adapter/test_override_database/test_override_database.py:53:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/functional/adapter/test_override_database/test_override_database.py:41: in run_database_override
    run_dbt(['seed'])
../dbt-core/core/dbt/tests/util.py:70: in run_dbt
    res, success = handle_and_check(args)
../dbt-core/core/dbt/main.py:191: in handle_and_check
    task, res = run_from_args(parsed)
../dbt-core/core/dbt/main.py:218: in run_from_args
    task = parsed.cls.from_args(args=parsed)
../dbt-core/core/dbt/task/base.py:184: in from_args
    move_to_nearest_project_dir(args)
../dbt-core/core/dbt/task/base.py:171: in move_to_nearest_project_dir
    nearest_project_dir = get_nearest_project_dir(args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

args = Namespace(cache_selected_only=None, cls=<class 'dbt.task.seed.SeedTask'>, debug=None, defer=None, event_buffer_size=No...lors=None, use_experimental_parser=None, vars='{}', version_check=None, warn_error=None, which='seed', write_json=None)

    def get_nearest_project_dir(args):
        # If the user provides an explicit project directory, use that
        # but don't look at parent directories.
        if args.project_dir:
            project_file = os.path.join(args.project_dir, "dbt_project.yml")
            if os.path.exists(project_file):
                return args.project_dir
            else:
                raise dbt.exceptions.RuntimeException(
                    "fatal: Invalid --project-dir flag. Not a dbt project. "
                    "Missing dbt_project.yml file"
                )

        root_path = os.path.abspath(os.sep)
        cwd = os.getcwd()

        while cwd != root_path:
            project_file = os.path.join(cwd, "dbt_project.yml")
            if os.path.exists(project_file):
                return cwd
            cwd = os.path.dirname(cwd)

        raise dbt.exceptions.RuntimeException(
>           "fatal: Not a dbt project (or any of the parent directories). "
            "Missing dbt_project.yml file"
        )
E       dbt.exceptions.RuntimeException: Runtime Error
E         fatal: Not a dbt project (or any of the parent directories). Missing dbt_project.yml file

../dbt-core/core/dbt/task/base.py:165: RuntimeException
----------------------------------------------------------------------------------------------------------------------------- Captured stdout call -----------------------------------------------------------------------------------------------------------------------------


Invoking dbt with ['seed']
=========================================================================================================================== short test summary info ============================================================================================================================
FAILED tests/functional/adapter/test_override_database/test_override_database.py::TestModelOverride::test_bigquery_database_override - dbt.exceptions.RuntimeException: Runtime Error

…s (commented out tests)

…mcknight/convert_override_test

…_bigquery_database_override test working on getting the rest to work for first test

…ormation from configs

…mcknight/convert_override_test

jtcohen6

Env var: We actually don't use BIGQUERY_TEST_DATABASE anywhere else. In the service_account profile, we use project_id, pulled out of the service account JSON. For oauth connections, we use the default configured via gcloud config. I think we could swing this test without that env var, too—but I'm open to leaving it in if you think it's better that way.

Organization of test cases: I wouldn't expect this test case to land in tests/functional/adapter, I would only expect that to be test cases that are pulled from dbt-tests-adapter. I might expect this test case to land in tests/functional/<topic_area>/test_override_database/...

Finally: We can also delete the legacy test (tests/integration/override_database_test), right?

jtcohen6 · 2022-05-09T14:58:38Z

tests/functional/adapter/test_override_database/fixtures.py

+{%- if target.type == 'bigquery' -%}
+  {{ config(project=var('alternate_db')) }}
+{%- else -%}
+  {{ config(database=var('alternate_db')) }}
+{%- endif -%}


This works either way, dbt-bigquery recognizes project + database as aliases for one another. Good to test that the alias works though!

jtcohen6 · 2022-05-09T16:39:07Z

tests/functional/adapter/test_override_database/test_override_database.py

+        run_dbt(["seed"])
+        assert len(run_dbt(["run"])) == 4
+        check_relations_equal_with_relations(project.adapter, [
+            project.adapter.Relation.create(database=os.getenv("BIGQUERY_TEST_DATABASE"), schema=project.test_schema, identifier="seed"),


We could either:

Replace database=os.getenv("BIGQUERY_TEST_DATABASE") with database=project.database

Remove the database argument entirely, and fix a v1.1 regression ([CT-604] get_columns_in_relation when database is missing #180) by restoring support for using default database/schema when unspecified in get_columns_in_relation

jtcohen6 · 2022-05-09T16:40:24Z

tests/functional/adapter/test_override_database/test_override_database.py

+                "database": os.getenv("BIGQUERY_TEST_ALT_DATABASE"),
+                "test": {
+                    "subfolder": {
+                        "database": os.getenv("BIGQUERY_TEST_DATABASE")


You could replace this env var pretty straightforwardly with either of the following.

More like what users would actually write in their projects:

Suggested change

"database": os.getenv("BIGQUERY_TEST_DATABASE")

"database": "{{ target.database }}"

More like what this is doing currently:

Suggested change

"database": os.getenv("BIGQUERY_TEST_DATABASE")

"database": project.database

I think the target.database works more as that part of the function doesn't generally have access to project and when i tried to tap it it gave recursive dependency issues.

McKnight-42 · 2022-05-09T18:07:04Z

Env var: We actually don't use BIGQUERY_TEST_DATABASE anywhere else. In the service_account profile, we use project_id, pulled out of the service account JSON. For oauth connections, we use the default configured via gcloud config. I think we could swing this test without that env var, too—but I'm open to leaving it in if you think it's better that way.

Organization of test cases: I wouldn't expect this test case to land in tests/functional/adapter, I would only expect that to be test cases that are pulled from dbt-tests-adapter. I might expect this test case to land in tests/functional/<topic_area>/test_override_database/...

Finally: We can also delete the legacy test (tests/integration/override_database_test), right?

Do we have any big overarching topics we would want to put them in or would just the override_database folder be that topic curious if this is something that will be same across all adapters and we have any discussion on it?

I'm fine removing it if we were only using it to test this use case, I know we also have the dbt-dev account and the alt account still so that covers these cases well for ci/cd and we still have the no access account to test against for that use case. would just need to make sure we remove it from bigquery eventually.

and yes plan to delete the older test once this is working its nice to have them to fall back on and compare post conversion.

…TEST_DATABASE env

ChenyuLInx

LGTM! Glad to see all things related to compare different database object resolved!

jtcohen6 · 2022-05-11T17:19:53Z

dbt/adapters/bigquery/connections.py

+        # backwards compatibility: fill in with defaults if not specified
+        database = database or conn.credentials.database
+        schema = schema or conn.credentials.schema


To discuss: Backporting this piece to 1.1.latest, to resolve #180. Let's just add a test case for the regression case as well. It's really just calling adapter.get_columns_in_relation on a Relation that's missing database/schema

Do we want this test in the test_bigquery_adapter section using the magic mock?

…-labs#165) * init push or pytest conversion of override_database for bigquery * tests are currently passing, need to implement some test changes overs (commented out tests) * implemented a vew versions of the check_realations_equal test on test_bigquery_database_override test working on getting the rest to work for first test * trying to grab local env to test against need to find way to grab information from configs * attempting to move over to using check_relations_equal_with_relations * ordering of models * post pair update * debugged test, had passing locally * adding change to conftest to see if it passes tests in ci/cd * removing conftest change * trying ci/cd after some changes * trying to take into account CT-604, remove old test, remove BIGQUERY_TEST_DATABASE env * moving override_database out of adapter folder up one

* adding regression test case draft * backporting #165 to 1.1.0 latest * trying to take into account CT-604, remove old test, remove BIGQUERY_TEST_DATABASE env

init push or pytest conversion of override_database for bigquery

e955ff4

McKnight-42 self-assigned this Apr 21, 2022

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

McKnight-42 added 3 commits April 22, 2022 17:41

tests are currently passing, need to implement some test changes over…

2ab362f

…s (commented out tests)

Merge branch 'main' of https://github.com/dbt-labs/dbt-bigquery into …

331fafc

…mcknight/convert_override_test

implemented a vew versions of the check_realations_equal test on test…

e3b42d8

…_bigquery_database_override test working on getting the rest to work for first test

McKnight-42 requested review from gshank, VersusFacit and ChenyuLInx April 25, 2022 19:34

McKnight-42 added 10 commits April 25, 2022 17:16

trying to grab local env to test against need to find way to grab inf…

ce4fde9

…ormation from configs

attempting to move over to using check_relations_equal_with_relations

a1242e2

ordering of models

09bb316

Merge branch 'main' of https://github.com/dbt-labs/dbt-bigquery into …

4086d18

…mcknight/convert_override_test

post pair update

ee6a7c3

Merge branch 'main' of https://github.com/dbt-labs/dbt-bigquery into …

3760c59

…mcknight/convert_override_test

debugged test, had passing locally

f8e3f28

Merge branch 'main' of https://github.com/dbt-labs/dbt-bigquery into …

44db9ce

…mcknight/convert_override_test

adding change to conftest to see if it passes tests in ci/cd

d523ce9

removing conftest change

0823e3b

jtcohen6 reviewed May 9, 2022

View reviewed changes

McKnight-42 added 3 commits May 9, 2022 13:10

trying ci/cd after some changes

4ff161c

trying to take into account CT-604, remove old test, remove BIGQUERY_…

513bde1

…TEST_DATABASE env

moving override_database out of adapter folder up one

5fda361

McKnight-42 requested a review from jtcohen6 May 9, 2022 18:42

McKnight-42 marked this pull request as ready for review May 9, 2022 18:55

ChenyuLInx approved these changes May 10, 2022

View reviewed changes

McKnight-42 merged commit 8aaa6ab into main May 11, 2022

McKnight-42 deleted the mcknight/convert_override_test branch May 11, 2022 16:20

jtcohen6 reviewed May 11, 2022

View reviewed changes

McKnight-42 added a commit that referenced this pull request Jun 1, 2022

backporting #165 to 1.1.0 latest

15e1b57

McKnight-42 mentioned this pull request Jun 1, 2022

[Backport] #165/#188 #197

Merged

4 tasks

McKnight-42 added a commit that referenced this pull request Jun 8, 2022

[Backport] #165/#188 (#197)

643352a

* adding regression test case draft * backporting #165 to 1.1.0 latest * trying to take into account CT-604, remove old test, remove BIGQUERY_TEST_DATABASE env

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init push or pytest conversion of override_database for bigquery #165

init push or pytest conversion of override_database for bigquery #165

McKnight-42 commented Apr 21, 2022 •

edited

Loading

McKnight-42 commented Apr 21, 2022

jtcohen6 left a comment •

edited

Loading

jtcohen6 May 9, 2022

jtcohen6 May 9, 2022

jtcohen6 May 9, 2022

McKnight-42 May 9, 2022

McKnight-42 commented May 9, 2022 •

edited

Loading

ChenyuLInx left a comment

jtcohen6 May 11, 2022

McKnight-42 May 12, 2022

	"database": os.getenv("BIGQUERY_TEST_DATABASE")
	"database": "{{ target.database }}"

	"database": os.getenv("BIGQUERY_TEST_DATABASE")
	"database": project.database

init push or pytest conversion of override_database for bigquery #165

init push or pytest conversion of override_database for bigquery #165

Conversation

McKnight-42 commented Apr 21, 2022 • edited Loading

Description

Checklist

McKnight-42 commented Apr 21, 2022

jtcohen6 left a comment • edited Loading

Choose a reason for hiding this comment

jtcohen6 May 9, 2022

Choose a reason for hiding this comment

jtcohen6 May 9, 2022

Choose a reason for hiding this comment

jtcohen6 May 9, 2022

Choose a reason for hiding this comment

McKnight-42 May 9, 2022

Choose a reason for hiding this comment

McKnight-42 commented May 9, 2022 • edited Loading

ChenyuLInx left a comment

Choose a reason for hiding this comment

jtcohen6 May 11, 2022

Choose a reason for hiding this comment

McKnight-42 May 12, 2022

Choose a reason for hiding this comment

McKnight-42 commented Apr 21, 2022 •

edited

Loading

jtcohen6 left a comment •

edited

Loading

McKnight-42 commented May 9, 2022 •

edited

Loading