create a happy path project fixture #10291

ChenyuLInx · 2024-06-10T23:48:58Z

resolves #10256

Problem

We are missing a central place to define a happy path project that contains a bit of everything to be reused across different tests.

Solution

We are adding a happy_path_project fixture that will give integration tests a project to use

I had to refactor the project construction a little bit to make using happy path project a bit more obvious and intuiative
I choose to use normal file folder instead of using string defined in code to improve local testing experience and readability.

github-actions · 2024-06-10T23:49:15Z

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.

codecov · 2024-06-10T23:53:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.71%. Comparing base (4df120e) to head (8839cfb).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10291      +/-   ##
==========================================
- Coverage   88.74%   88.71%   -0.03%     
==========================================
  Files         180      180              
  Lines       22495    22483      -12     
==========================================
- Hits        19963    19946      -17     
- Misses       2532     2537       +5

Flag	Coverage Δ
integration	`85.97% <100.00%> (-0.11%)`	⬇️
unit	`61.70% <80.00%> (-1.51%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

QMalcolm

Looks good to me! Thank you for doing this work 🙂

I do have some questions in regards to how things changed. They aren't blocking questions, but I'd like to discuss them so I can better understand how the code is functioning. Most of these occur in the fix commits. It'd be amazing to have some of that context in the commit messages themselves.

core/dbt/tests/fixtures/project.py

QMalcolm · 2024-06-11T03:22:31Z

tests/functional/list/test_list.py

@@ -56,7 +42,7 @@ def expect_given_output(self, args, expectations):
                else:
                    assert got == expected

-    def expect_snapshot_output(self, project):
+    def expect_snapshot_output(self, happy_path_project):  # noqa: F811


In this test we're actually using the happy_path_project fixture argument in the function. Is the # noqa: F811 necessary for another reason?

+1, also curious about this

I think for whatever reason flake8 was complaining about all of these in my local dev environment.
Let me try again and see what happened here

This is the problem with pytest, I don't see a easy way of getting rid of it.
https://stackoverflow.com/questions/50268284/why-im-getting-f811-error-in-this-code

QMalcolm · 2024-06-11T03:22:47Z

tests/functional/list/test_list.py

@@ -724,12 +710,12 @@ def expect_resource_type_env_var(self):
        }
        del os.environ["DBT_EXCLUDE_RESOURCE_TYPES"]

-    def expect_selected_keys(self, project):
+    def expect_selected_keys(self, happy_path_project):  # noqa: F811


In this test we're actually using the happy_path_project fixture argument in the function. Is the # noqa: F811 necessary for another reason?

QMalcolm · 2024-06-11T03:29:16Z

core/dbt/tests/fixtures/project.py

-    project_files,
    project_setup: TestProjInfo,
+    project_files,


I'm not asking for a change here, but I would like to understand the context. Does the ordering of the fixture arguments here mean something? Is there a reason the project_setup fixture has to be come before project_files? This is the kind of thing I would love to see in a commit comment so that I can better understand why something is changing while reviewing.

Yes, adding a comment in the following commit.
The order of the argument determines the order of fixtures being executed(if they need to).
So we do all setups, then write the project files. Setup should be independent of project file write.

Another reason is for the happy path fixture, we do not use the dbt_project.yml fixture, instead, we want to use the files in the fixture folder, changing the order makes sure that the tests run with the intended files

QMalcolm · 2024-06-11T03:33:37Z

tests/functional/test_selection/fixtures.py

-@pytest.fixture(scope="class")
-def project_files(
-    project_root,
-    tests,
-    models,
-    selectors_yml,
-):
-    write_project_files(project_root, "tests", tests)
-    write_project_files(project_root, "models", models)


Were we able to remove this because of the existing more widely available project_files satisfied the relevant tests? I'm trying to understand if this is extraneous cleanup or if it was necessary for some reason in regards to the happy path testing we are setting up in this branch.

Yes, they are covered by the project_files.
And the reason they are being removed is they failed to write certain files(like dbt_project.yml), they might not be relevant anymore with the order of writing project files but I think it is still good to remove.

MichelleArk · 2024-06-11T16:15:55Z

tests/functional/fixtures/happy_path_project/macros/macro_stuff.sql

@@ -0,0 +1,8 @@
+


super nit: looks like a lot of these files have an extra empty line at the top of the file, could we clean that up?

Sounds good!

MichelleArk

Not necessarily blocking but I would like to understand the necessity for the new # noqa: F811 usages with happy_path_project since we'll probably start seeing those all over the place once we migrate more tests to using happy_path_project.

create a happy path project fixture

b889396

ChenyuLInx requested a review from a team as a code owner June 10, 2024 23:48

cla-bot bot added the cla:yes label Jun 10, 2024

fix

e8c6cb7

ChenyuLInx added the Skip Changelog Skips GHA to check for changelog file label Jun 10, 2024

ChenyuLInx added 3 commits June 10, 2024 17:16

fix

37a34b2

fix

15cc7f2

fix

5dd23ec

QMalcolm approved these changes Jun 11, 2024

View reviewed changes

MichelleArk reviewed Jun 11, 2024

View reviewed changes

add comment and remove empty line

8839cfb

MichelleArk approved these changes Jun 11, 2024

View reviewed changes

ChenyuLInx requested review from QMalcolm and a team June 11, 2024 20:41

ChenyuLInx merged commit e699f5d into main Jun 11, 2024
63 checks passed

ChenyuLInx deleted the cl/10256 branch June 11, 2024 21:41

ChenyuLInx mentioned this pull request Jun 12, 2024

Implement state:modified for saved queries #10295

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create a happy path project fixture #10291

create a happy path project fixture #10291

ChenyuLInx commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024

codecov bot commented Jun 10, 2024 •

edited

Loading

QMalcolm left a comment

QMalcolm Jun 11, 2024

MichelleArk Jun 11, 2024

ChenyuLInx Jun 11, 2024

ChenyuLInx Jun 11, 2024

QMalcolm Jun 11, 2024 •

edited

Loading

QMalcolm Jun 11, 2024

ChenyuLInx Jun 11, 2024

QMalcolm Jun 11, 2024

ChenyuLInx Jun 11, 2024

MichelleArk Jun 11, 2024

ChenyuLInx Jun 11, 2024

MichelleArk left a comment

create a happy path project fixture #10291

create a happy path project fixture #10291

Conversation

ChenyuLInx commented Jun 10, 2024 • edited Loading

Problem

Solution

github-actions bot commented Jun 10, 2024

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

QMalcolm 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

QMalcolm Jun 11, 2024 • edited Loading

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

MichelleArk left a comment

Choose a reason for hiding this comment

ChenyuLInx commented Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

QMalcolm Jun 11, 2024 •

edited

Loading