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.

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

Recommend to use the name argument in the pytest.fixture decorator #55

Merged
merged 5 commits into from
Apr 4, 2023

Conversation

weiiwang01
Copy link
Contributor

@weiiwang01 weiiwang01 commented Mar 22, 2023

Add a tip about using the name argument in the pytest.fixture decorator to avoid naming conflicts.

@weiiwang01 weiiwang01 requested a review from a team as a code owner March 22, 2023 04:00
Copy link
Collaborator

@jdkandersson jdkandersson left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
yanksyoon
yanksyoon previously approved these changes Mar 22, 2023
Copy link
Contributor

@yanksyoon yanksyoon left a comment

Choose a reason for hiding this comment

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

LGTM!

@gregory-schiano
Copy link
Contributor

Can you add a description to the PR, I know the title is self explaining, but let's be consistent in adding descriptions

CONTRIBUTING.md Outdated Show resolved Hide resolved
@gregory-schiano
Copy link
Contributor

gregory-schiano commented Mar 22, 2023

I have the filling that this convention could be wider, or more generic.
Something like "always set the name argument" and convention towards fixture function naming "always _fixture suffix" and eventually a "scope" prefix

What will happen if we have the following:
conftest.py

@pytest.fixture
def app():
  yield "app"

test_charm.py

@pytest.fixture(name="app")
def app_fixture(ops_test):
   yield ops_test.model.app

def test_charm(app): None
 ...

CONTRIBUTING.md Outdated Show resolved Hide resolved
@weiiwang01
Copy link
Contributor Author

Can you add a description to the PR, I know the title is self explaining, but let's be consistent in adding descriptions

Added, thanks!

arturo-seijas
arturo-seijas previously approved these changes Mar 22, 2023
Copy link
Contributor

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM

@weiiwang01
Copy link
Contributor Author

I have the filling that this convention could be wider, or more generic. Something like "always set the name argument" and convention towards fixture function naming "always _fixture suffix" and eventually a "scope" prefix

What will happen if we have the following: conftest.py

@pytest.fixture
def app():
  yield "app"

test_charm.py

@pytest.fixture(name="app")
def app_fixture(ops_test):
   yield ops_test.model.app

def test_charm(app): None
 ...

I actually view this PR as a recommendation or tip, the original form of fixture still have its benefit of being short and easy to write.

In terms of two fixtures have the same name, pytest will choice one without raising an error. This is a known issue in pytest. pytest-dev/pytest#3966

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@jdkandersson
Copy link
Collaborator

jdkandersson commented Mar 23, 2023

I have the filling that this convention could be wider, or more generic. Something like "always set the name argument" and convention towards fixture function naming "always _fixture suffix" and eventually a "scope" prefix
What will happen if we have the following: conftest.py

@pytest.fixture
def app():
  yield "app"

test_charm.py

@pytest.fixture(name="app")
def app_fixture(ops_test):
   yield ops_test.model.app

def test_charm(app): None
 ...

I actually view this PR as a recommendation or tip, the original form of fixture still have its benefit of being short and easy to write.

In terms of two fixtures have the same name, pytest will choice one without raising an error. This is a known issue in pytest. pytest-dev/pytest#3966

I'm with Weii here, this standard is more to prevent needing to have clashing names with function and arguments which is a bit messy in Python. When there is no clash, name="app" shouldn't be used since it has the drawback of being longer

@gregory-schiano
Copy link
Contributor

I have the filling that this convention could be wider, or more generic. Something like "always set the name argument" and convention towards fixture function naming "always _fixture suffix" and eventually a "scope" prefix
What will happen if we have the following: conftest.py

@pytest.fixture
def app():
  yield "app"

test_charm.py

@pytest.fixture(name="app")
def app_fixture(ops_test):
   yield ops_test.model.app

def test_charm(app): None
 ...

I actually view this PR as a recommendation or tip, the original form of fixture still have its benefit of being short and easy to write.
In terms of two fixtures have the same name, pytest will choice one without raising an error. This is a known issue in pytest. pytest-dev/pytest#3966

I'm with Weii here, this standard is more to prevent needing to have clashing names with function and arguments which is a bit messy in Python. When there is no clash, name="app" shouldn't be used since it has the drawback of being longer

Ok 👌

Copy link
Contributor

@gregory-schiano gregory-schiano left a comment

Choose a reason for hiding this comment

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

LGTM

@weiiwang01 weiiwang01 merged commit 7c41936 into main Apr 4, 2023
@jdkandersson jdkandersson deleted the fixture-name branch June 23, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants