Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

create a happy path project fixture #10291

Merged
merged 6 commits into from
Jun 11, 2024
Merged

create a happy path project fixture #10291

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b889396
create a happy path project fixture
ChenyuLInx Jun 10, 2024
e8c6cb7
fix
ChenyuLInx Jun 10, 2024
37a34b2
fix
ChenyuLInx Jun 11, 2024
15cc7f2
fix
ChenyuLInx Jun 11, 2024
5dd23ec
fix
ChenyuLInx Jun 11, 2024
8839cfb
add comment and remove empty line
ChenyuLInx Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/dbt/tests/fixtures/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@
profiles_root,
profiles_yml,
clean_up_logging,
dbt_project_yml,
):
# The profiles.yml and dbt_project.yml should already be written out
args = Namespace(
Expand Down Expand Up @@ -598,7 +599,7 @@
# to pull in the other fixtures individually to access their information.
@pytest.fixture(scope="class")
def project(
project_files,
project_setup: TestProjInfo,
project_files,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

):
return project_setup

Check warning on line 605 in core/dbt/tests/fixtures/project.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/tests/fixtures/project.py#L605 

Added line #L605 was not covered by tests
2 changes: 1 addition & 1 deletion tests/functional/fixtures/happy_path_fixture.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ def happy_path_project_files(project_root):


@pytest.fixture(scope="class")
def happy_path_project(happy_path_project_files, project_setup):
def happy_path_project(project_setup, happy_path_project_files):
return project_setup
13 changes: 0 additions & 13 deletions tests/functional/test_selection/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import pytest

from dbt.tests.fixtures.project import write_project_files

tests__cf_a_b_sql = """
select * from {{ ref('model_a') }}
cross join {{ ref('model_b') }}
Expand Down Expand Up @@ -84,14 +82,3 @@ def models():
"model_b.sql": models__model_b_sql,
"model_a.sql": models__model_a_sql,
}


@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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

6 changes: 1 addition & 5 deletions tests/functional/test_selection/test_selection_expansion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import pytest

from dbt.tests.util import run_dbt
from tests.functional.test_selection.fixtures import ( # noqa: F401
models,
project_files,
tests,
)
from tests.functional.test_selection.fixtures import models, tests # noqa: F401


class TestSelectionExpansion:
Expand Down
Loading