canonical / is-charms-contributing-guide

The code contributing guide for the IS charms team
Apache License 2.0
2 stars 1 forks source link

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

Closed weiiwang01 closed 1 year ago

weiiwang01 commented 1 year ago

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

gregory-schiano commented 1 year ago

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

gregory-schiano commented 1 year ago

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
 ...
weiiwang01 commented 1 year ago

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!

weiiwang01 commented 1 year ago

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. https://github.com/pytest-dev/pytest/issues/3966

jdkandersson commented 1 year ago

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 commented 1 year ago

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 👌