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

fix: adjust OpsTest.tmp_path to avoid os.sep #125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 14 additions & 2 deletions pytest_operator/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -510,18 +510,30 @@ def models(self) -> Mapping[str, ModelState]:
"""Returns the dict of managed models by this fixture."""
return {k: dataclasses.replace(v) for k, v in self._models.items()}

@staticmethod
def _model_name_to_filename(name: str):
"""Sanitises a model name to one that is suitable for use as a component
of a filename."""
# Model names can only contain lowercase letters, digits, hyphens, and
# slashes, so most of the normalisation is already done, but we do a
# little bit of duplicated sanitisation here just to be certain.
# "letters" currently means ASCII letters, but may not in the future.
return "".join((c if c.isalnum() or c == "-" else "_") for c in name)

@property
def tmp_path(self) -> Path:
tmp_path = self._global_tmp_path
current_state = self.current_alias and self._models.get(self.current_alias)
if current_state and current_state.tmp_path is None:
tmp_path = self._tmp_path_factory.mktemp(current_state.model_name)
tmp_path = self._tmp_path_factory.mktemp(
self._model_name_to_filename(current_state.model_name)
)
current_state.tmp_path = tmp_path
elif current_state and current_state.tmp_path:
tmp_path = current_state.tmp_path
elif not tmp_path:
tmp_path = self._global_tmp_path = self._tmp_path_factory.mktemp(
self.default_model_name
self._model_name_to_filename(self.default_model_name)
)
log.info(f"Using tmp_path: {tmp_path}")
return tmp_path
Expand Down
8 changes: 8 additions & 0 deletions tests/integration/test_pytest_operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ def test_tmp_path(ops_test):
assert ops_test.tmp_path.relative_to(tox_env_dir)


async def test_tmp_path_nonpath_chars(ops_test):
model_alias = f"user{os.sep}name"
await ops_test.track_model(model_alias)
with ops_test.model_context(model_alias):
assert os.sep not in str(ops_test.tmp_path)
await ops_test.forget_model(model_alias)


async def test_run(ops_test):
assert await ops_test.run("/bin/true") == (0, "", "")
assert await ops_test.run("/bin/false") == (1, "", "")
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/test_pytest_operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,24 @@ async def test_without_tox(request, ops_test):
result.assert_outcomes(passed=1)


@patch.object(plugin.OpsTest, "default_model_name", "user/model")
@patch.object(plugin.OpsTest, "_setup_model", AsyncMock())
@patch.object(plugin.OpsTest, "_cleanup_models", AsyncMock())
def test_tmp_path_with_nonpath_chars(pytester):
pytester.makepyfile(
f"""
import os
from pathlib import Path

os.environ.update(**{ENV})
async def test_with_tox(ops_test):
assert ops_test.tmp_path.name == "user_model0"
"""
)
result = pytester.runpytest()
result.assert_outcomes(passed=1)


Comment on lines +63 to +80
Copy link
Member

Choose a reason for hiding this comment

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

So, that test was kinda confusing to me. Can we do this:

Suggested change
@patch.object(plugin.OpsTest, "default_model_name", "user/model")
@patch.object(plugin.OpsTest, "_setup_model", AsyncMock())
@patch.object(plugin.OpsTest, "_cleanup_models", AsyncMock())
def test_tmp_path_with_nonpath_chars(pytester):
pytester.makepyfile(
f"""
import os
from pathlib import Path
os.environ.update(**{ENV})
async def test_with_tox(ops_test):
assert ops_test.tmp_path.name == "user_model0"
"""
)
result = pytester.runpytest()
result.assert_outcomes(passed=1)
async def test_tmp_path_with_nonpath_chars(tmp_path_factory, setup_request):
setup_request.module.__name__ = "user/model"
ops_test = plugin.OpsTest(setup_request, tmp_path_factory)
assert ops_test.tmp_path.name.startswith("user_model")

async def test_destructive_mode(monkeypatch, tmp_path_factory):
patch = monkeypatch.setattr
patch(plugin.os, "getgroups", mock_getgroups := Mock(return_value=[]))
Expand Down
Loading