Refactor `build` test to new framework #5134

iknox-fa · 2022-04-21T23:36:11Z

resolves #5128 (CT-538)

Description

Converts build test to new framework

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-04-21T23:36:29Z

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.

stu-k

Nothing stands out, all previous models and snapshots accounted for 👍

tests/functional/build/test_build.py

emmyoop · 2022-04-22T14:09:18Z

tests/functional/build/fixtures.py

+
+"""
+
+


Everything below this comment should be defined in the test file. It makes it much more clear what's happening in the tests that way. The project fixture takes care of writing all the files to the project.

I've been putting the actual model strings into the test files most of the time. I tend to only have strings in this file that are extremely long or shared across multiple files. If it makes sense to do so i also split up the single giant test file into multiple files to make it easier to find and organize tests. That doesn't always make sense though, it's a different call for each set of tests, not a rule.

Everything below this comment should be defined in the test file. It makes it much more clear what's happening in the tests that way.

I'm not sure I agree with that assessment. IMO it's easier to look at the test logic in one place (the test file) and the underlying project data in another file (the fixtures file). That said, I don't have strong feelings here so I'm happy to move it if it's more consistent.

I hear you. I'm going to start a thread so we can decide on a standard here as a team.

emmyoop

Just a bit more cleanup to reduce repetition.

emmyoop · 2022-04-27T13:01:18Z

tests/functional/build/test_build.py

+@pytest.fixture(scope="class")
+def snapshots():
+    return {
+        "snap_1.sql": snapshots__snap_1_sql,
+        "snap_0.sql": snapshots__snap_0_sql,
+        "snap_99.sql": snapshots__snap_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def tests_failing():
+    return {
+        "model_2.sql": tests_failing__model_2_sql,
+        "test.yml": tests_failing__test_yml,
+        "model_0.sql": tests_failing__model_0_sql,
+        "model_1.sql": tests_failing__model_1_sql,
+        "model_99.sql": tests_failing__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models():
+    return {
+        "model_2.sql": models__model_2_sql,
+        "test.yml": models__test_yml,
+        "model_0.sql": models__model_0_sql,
+        "model_1.sql": models__model_1_sql,
+        "model_99.sql": models__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models_failing():
+    return {
+        "model_3.sql": models_failing__model_3_sql,
+        "model_2.sql": models_failing__model_2_sql,
+        "test.yml": models_failing__test_yml,
+        "model_0.sql": models_failing__model_0_sql,
+        "model_1.sql": models_failing__model_1_sql,
+        "model_99.sql": models_failing__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def seeds():
+    return {"countries.csv": seeds__countries_csv}
+
+
+@pytest.fixture(scope="class")
+def models_circular_relationship():
+    return {
+        "test.yml": models_circular_relationship__test_yml,
+        "model_0.sql": models_circular_relationship__model_0_sql,
+        "model_1.sql": models_circular_relationship__model_1_sql,
+        "model_99.sql": models_circular_relationship__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models_simple_blocking():
+    return {
+        "schema.yml": models_simple_blocking__schema_yml,
+        "model_b.sql": models_simple_blocking__model_b_sql,
+        "model_a.sql": models_simple_blocking__model_a_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models_interdependent():
+    return {
+        "schema.yml": models_interdependent__schema_yml,
+        "model_c.sql": models_interdependent__model_c_sql,
+        "model_a.sql": models_interdependent__model_a_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def project_files(
+    project_root,
+    snapshots,
+    tests_failing,
+    models,
+    models_failing,
+    seeds,
+    models_circular_relationship,
+    models_simple_blocking,
+    models_interdependent,
+):
+    write_project_files(project_root, "snapshots", snapshots)
+    write_project_files(project_root, "tests-failing", tests_failing)
+    write_project_files(project_root, "models", models)
+    write_project_files(project_root, "models-failing", models_failing)
+    write_project_files(project_root, "seeds", seeds)
+    write_project_files(project_root, "models-circular-relationship", models_circular_relationship)
+    write_project_files(project_root, "models-simple-blocking", models_simple_blocking)
+    write_project_files(project_root, "models-interdependent", models_interdependent)


None of this is needed. Just define the models/snapshots/etc in the class. The project fixture takes care of the write_project_files() function. I'll add an example below of what I mean.

the reason we were having different folders for different model was that there could be only one set of files for the whole tests so we put everything in different folders and use model_path to point to it. In the new test framework, we write a project repo to disk for every test class, so you can define the models fixture that only containes files needed for current class, and not need the model_path any more.

dbt-core/tests/functional/schema_tests/test_schema_v2_tests.py

Lines 41 to 43 in ab0c351

@pytest.fixture(scope="class")

def models(self, all_models): # noqa: F811

return all_models["models"]

emmyoop · 2022-04-27T13:02:03Z

tests/functional/build/test_build.py

+class TestFailingBuild(BuildBase):
+    @pytest.fixture(scope="class")
+    def models(self, models_failing):  # noqa: F811
+        return {
+            "model_3.sql": models_failing["model_3.sql"],
+            "model_2.sql": models_failing["model_2.sql"],
+            "test.yml": models_failing["test.yml"],
+            "model_0.sql": models_failing["model_0.sql"],
+            "model_1.sql": models_failing["model_1.sql"],
+            "model_99.sql": models_failing["model_99.sql"],
+        }


Here's the example of how to define the models without everything at the top of this file.

def models(self): return { "model_3.sql": models_failing__model_3_sql, "model_2.sql": models_failing__model_2_sql, "test.yml": models_failing__test_yml, "model_0.sql": models_failing__model_0_sql, "model_1.sql": models_failing__model_1_sql, "model_99.sql": models_failing__model_99_sql, }

When you import the strings this would be how you define the models fixture and can then remove the # noqa.

ChenyuLInx · 2022-05-06T15:02:03Z

Agreed with Emily's comment, just some clean up then this is good to go!

ChenyuLInx

@iknox-fa Looks like one conflict to resolve otherwise looks great to me!

emmyoop

I don't think the new manifest should be here?

We also have a pending conversation in our slack about how to approach pulling in files.

tests/functional/artifacts/data/state/v6/manifest.json

emmyoop · 2022-08-04T13:23:43Z

tests/functional/build/fixtures.py

+
+"""
+
+


I hear you. I'm going to start a thread so we can decide on a standard here as a team.

jtcohen6 · 2022-08-10T16:55:25Z

@emmyoop @iknox-fa Is the only remaining blocker for this PR a question about how fixture data should be formatted / located in functional tests?

emmyoop

There's a bit more missing with the standardization around how we format and import.

emmyoop · 2022-11-02T16:32:45Z

tests/functional/build/fixtures.py

+def snapshots():
+    return {
+        "snap_1.sql": snapshots__snap_1_sql,
+        "snap_0.sql": snapshots__snap_0_sql,
+        "snap_99.sql": snapshots__snap_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def tests_failing():
+    return {
+        "model_2.sql": tests_failing__model_2_sql,
+        "test.yml": tests_failing__test_yml,
+        "model_0.sql": tests_failing__model_0_sql,
+        "model_1.sql": tests_failing__model_1_sql,
+        "model_99.sql": tests_failing__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models():
+    return {
+        "model_2.sql": models__model_2_sql,
+        "test.yml": models__test_yml,
+        "model_0.sql": models__model_0_sql,
+        "model_1.sql": models__model_1_sql,
+        "model_99.sql": models__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models_failing():
+    return {
+        "model_3.sql": models_failing__model_3_sql,
+        "model_2.sql": models_failing__model_2_sql,
+        "test.yml": models_failing__test_yml,
+        "model_0.sql": models_failing__model_0_sql,
+        "model_1.sql": models_failing__model_1_sql,
+        "model_99.sql": models_failing__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def seeds():
+    return {"countries.csv": seeds__countries_csv}
+
+
+@pytest.fixture(scope="class")
+def models_circular_relationship():
+    return {
+        "test.yml": models_circular_relationship__test_yml,
+        "model_0.sql": models_circular_relationship__model_0_sql,
+        "model_1.sql": models_circular_relationship__model_1_sql,
+        "model_99.sql": models_circular_relationship__model_99_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models_simple_blocking():
+    return {
+        "schema.yml": models_simple_blocking__schema_yml,
+        "model_b.sql": models_simple_blocking__model_b_sql,
+        "model_a.sql": models_simple_blocking__model_a_sql,
+    }
+
+
+@pytest.fixture(scope="class")
+def models_interdependent():
+    return {
+        "schema.yml": models_interdependent__schema_yml,
+        "model_c.sql": models_interdependent__model_c_sql,
+        "model_a.sql": models_interdependent__model_a_sql,
+    }


You should completely remove all of this, then import the specific strings you're using instead of the fixtures in the test file.

I was not in the team meeting but my understanding was if there are more than 50 lines to define, we define them in this file but just the strings. It is the pattern I've seen with most other conversions we have done.

emmyoop · 2022-11-02T16:33:18Z

tests/functional/build/test_build.py

+class TestFailingBuild(BuildBase):
+    @pytest.fixture(scope="class")
+    def models(self, models_failing):  # noqa: F811
+        return {
+            "model_3.sql": models_failing["model_3.sql"],
+            "model_2.sql": models_failing["model_2.sql"],
+            "test.yml": models_failing["test.yml"],
+            "model_0.sql": models_failing["model_0.sql"],
+            "model_1.sql": models_failing["model_1.sql"],
+            "model_99.sql": models_failing["model_99.sql"],
+        }


When you import the strings this would be how you define the models fixture and can then remove the # noqa.

github-actions · 2023-05-02T01:52:30Z

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

github-actions · 2023-05-09T01:53:36Z

Although we are closing this PR as stale, it can still be reopened to continue development. Just add a comment to notify the maintainers.

iknox-fa added 2 commits April 21, 2022 13:37

raw unchecked test conversion

8af6292

tests refactored

cc3ce9d

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

iknox-fa added the Skip Changelog Skips GHA to check for changelog file label Apr 21, 2022

iknox-fa linked an issue Apr 21, 2022 that may be closed by this pull request

[CT-538] 069_build_tests #5128

Closed

iknox-fa marked this pull request as ready for review April 21, 2022 23:49

iknox-fa requested a review from a team as a code owner April 21, 2022 23:49

iknox-fa requested review from emmyoop and ChenyuLInx April 21, 2022 23:49

stu-k approved these changes Apr 22, 2022

View reviewed changes

tests/functional/build/test_build.py Outdated Show resolved Hide resolved

emmyoop requested changes Apr 22, 2022

View reviewed changes

tests/functional/build/test_build.py Outdated Show resolved Hide resolved

tests/functional/build/test_build.py Outdated Show resolved Hide resolved

tests/functional/build/test_build.py Outdated Show resolved Hide resolved

tests/functional/build/test_build.py Show resolved Hide resolved

emmyoop reviewed Apr 22, 2022

View reviewed changes

iknox-fa added 4 commits April 22, 2022 14:03

PR feedback

e7145fa

PR feedback

39c0e11

PR feedback

d45ad6e

PR feedback

abcdca6

iknox-fa requested a review from emmyoop April 25, 2022 17:55

emmyoop requested changes Apr 27, 2022

View reviewed changes

iknox-fa added 2 commits July 28, 2022 15:08

Merge branch 'main' into iknox-refactor-build-test-CT-482-CT-538

1e5549d

PR feedback from waaay back

35dc39b

iknox-fa requested a review from emmyoop July 28, 2022 21:47

ChenyuLInx approved these changes Jul 31, 2022

View reviewed changes

iknox-fa and others added 2 commits August 3, 2022 10:49

Merge branch 'main' into iknox-refactor-build-test-CT-482-CT-538

08402f1

missing newline after merge conflict fix in ui

953adb7

emmyoop requested changes Aug 4, 2022

View reviewed changes

iknox-fa added 2 commits August 9, 2022 11:21

Merge branch 'main' into iknox-refactor-build-test-CT-482-CT-538

5ce33ac

restore manifest.json due to bug (ref CT-962)

174e3f6

emmyoop requested changes Nov 2, 2022

View reviewed changes

github-actions bot added the stale Issues that have gone stale label May 2, 2023

github-actions bot closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `build` test to new framework #5134

Refactor `build` test to new framework #5134

iknox-fa commented Apr 21, 2022 •

edited by stu-k

Loading

github-actions bot commented Apr 21, 2022

stu-k left a comment

emmyoop Apr 22, 2022

emmyoop Apr 22, 2022

iknox-fa Apr 22, 2022

emmyoop Aug 4, 2022

emmyoop left a comment

emmyoop Apr 27, 2022

ChenyuLInx May 6, 2022

emmyoop Apr 27, 2022 •

edited

Loading

emmyoop Nov 2, 2022

ChenyuLInx commented May 6, 2022

ChenyuLInx left a comment

emmyoop left a comment

emmyoop Aug 4, 2022

jtcohen6 commented Aug 10, 2022

emmyoop left a comment

emmyoop Nov 2, 2022

emmyoop Nov 2, 2022

github-actions bot commented May 2, 2023

github-actions bot commented May 9, 2023

	@pytest.fixture(scope="class")
	def models(self, all_models): # noqa: F811
	return all_models["models"]


		"""


		"""

Refactor build test to new framework #5134

Refactor build test to new framework #5134

Conversation

iknox-fa commented Apr 21, 2022 • edited by stu-k Loading

Description

Checklist

github-actions bot commented Apr 21, 2022

stu-k 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

emmyoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmyoop Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChenyuLInx commented May 6, 2022

ChenyuLInx left a comment

Choose a reason for hiding this comment

emmyoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 commented Aug 10, 2022

emmyoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2023

github-actions bot commented May 9, 2023

Refactor `build` test to new framework #5134

Refactor `build` test to new framework #5134

iknox-fa commented Apr 21, 2022 •

edited by stu-k

Loading

emmyoop Apr 27, 2022 •

edited

Loading