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

feat!: add error if meta not passed when config or actions are in ops.testing #1497

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat!: add error if meta not passed when config or actions are in ops.testing #1497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 12 additions & 3 deletions testing/src/scenario/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,10 @@ def test_foo():
Use this when calling :meth:`run` to specify the event to emit.
"""

# def test_foo():
# ctx = Context(MyCharm, config={"foo": "bar"})
# ctx.run(..., State())

Comment on lines +457 to +460
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# def test_foo():
# ctx = Context(MyCharm, config={"foo": "bar"})
# ctx.run(..., State())

I'm assuming this is leftover rather than meant as an example?

def __init__(
self,
charm_type: type[CharmType],
Expand Down Expand Up @@ -503,13 +507,18 @@ def __init__(
spec: _CharmSpec[CharmType] = _CharmSpec.autoload(charm_type)
except MetadataNotFoundError as e:
raise ContextSetupError(
f"Cannot setup scenario with `charm_type`={charm_type}. "
f"Did you forget to pass `meta` to this Context?",
f"Cannot automatically setup scenario with `charm_type`={charm_type}. "
"This is likely because this charm has some nonstandard repository setup and Scenario "
"can't find the charmcraft|(metadata|config|actions) yamls in their usual location. "
"Please parse and provide their contents manually via the `meta`, `config` "
"and `actions` Context parameters.",
Comment on lines +510 to +514
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose if we're improving error messages we ought to avoid Scenario as a proper noun 😞

Suggested change
f"Cannot automatically setup scenario with `charm_type`={charm_type}. "
"This is likely because this charm has some nonstandard repository setup and Scenario "
"can't find the charmcraft|(metadata|config|actions) yamls in their usual location. "
"Please parse and provide their contents manually via the `meta`, `config` "
"and `actions` Context parameters.",
f"Cannot automatically set up with `charm_type`={charm_type}. "
"This is likely because this charm has some non-standard repository setup and the "
"charmcraft|(metadata|config|actions) YAML files aren't in their usual location. "
"Please parse and provide their contents manually via the `meta`, `config` "
"and `actions` Context parameters.",

) from e

else:
if not meta:
meta = {"name": str(charm_type.__name__)}
raise ContextSetupError(
"Scenario doesn't support overriding `actions|config` without overriding `meta` too."
)
Comment on lines +519 to +521
Copy link
Contributor

Choose a reason for hiding this comment

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

And I suppose here also 😞

Suggested change
raise ContextSetupError(
"Scenario doesn't support overriding `actions|config` without overriding `meta` too."
)
raise ContextSetupError(
"To override `actions|config` you must also override `meta`."
)

spec = _CharmSpec(
charm_type=charm_type,
meta=meta,
Expand Down
3 changes: 2 additions & 1 deletion testing/src/scenario/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,8 @@ def _load_metadata(charm_root: pathlib.Path):
def autoload(charm_type: type[CharmBase]) -> _CharmSpec[CharmType]:
"""Construct a ``_CharmSpec`` object by looking up the metadata from the charm's repo root.

Will attempt to load the metadata off the ``charmcraft.yaml`` file
Will attempt to load the metadata off the ``charmcraft.yaml`` file.
On failure, will fall back to the legacy ``metadata.yaml/actions.yaml/config.yaml`` files.
"""
charm_source_path = pathlib.Path(inspect.getfile(charm_type))
charm_root = charm_source_path.parent.parent
Expand Down
17 changes: 17 additions & 0 deletions testing/tests/test_charm_spec_autoload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,23 @@ def test_no_meta_raises(tmp_path, legacy):
Context(charm)


@pytest.mark.parametrize("legacy", (True, False))
@pytest.mark.parametrize(
"params",
(
{"actions": {"foo": "bar"}},
{"config": {"foo": "bar"}},
),
)
def test_partial_meta_raises(tmp_path, legacy, params):
with create_tempcharm(
tmp_path, legacy=legacy, meta={"type": "charm", "name": "sergio"}
) as charm:
# metadata not found:
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this initially confusing because there's metadata two lines above, maybe this would be clearer?

Suggested change
# metadata not found:
# metadata not found in YAML:

with pytest.raises(ContextSetupError):
Context(charm, **params)


@pytest.mark.parametrize("legacy", (True, False))
def test_relations_ok(tmp_path, legacy):
with create_tempcharm(
Expand Down
Loading