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

Conversation

tonyandrewmeyer
Copy link

When generating the tmp_path, ensure that only characters from the model name that are safe for use in paths are used. This particularly covers the case where the model name is in the form user/name, like MAAS.

Fixes #123

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review February 20, 2024 09:43
Copy link
Member

@addyess addyess left a comment

Choose a reason for hiding this comment

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

@tonyandrewmeyer looks good -- a few lint issues

@tonyandrewmeyer
Copy link
Author

Are there docs somewhere on how to run the tests (locally, not relying on CI)? tox -e lint and tox -e unit both pass locally, using Python 3.11, but fail in the CI, so I must not be running them correctly. tox -e integration fails locally, but with a totally different error to in the CI: RuntimeError: Missing dependency: charm.

Copy link
Member

@addyess addyess left a comment

Choose a reason for hiding this comment

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

This still looks good. I think the unit tests could be a bit easier to reason about

Comment on lines +63 to +80
@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)


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")

@addyess
Copy link
Member

addyess commented Feb 28, 2024

Are there docs somewhere on how to run the tests (locally, not relying on CI)? tox -e lint and tox -e unit both pass locally, using Python 3.11, but fail in the CI, so I must not be running them correctly. tox -e integration fails locally, but with a totally different error to in the CI: RuntimeError: Missing dependency: charm.

if you rebase, you'll see the integration tests are likely going to work after merging #128

@dimaqq
Copy link
Contributor

dimaqq commented Oct 7, 2024

@tonyandrewmeyer poke if still relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

MAAS cloud model name issue when using build_charm method
3 participants