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

cleanup testing.Context signature #1492

Open
PietroPasotti opened this issue Dec 9, 2024 · 1 comment · May be fixed by #1497
Open

cleanup testing.Context signature #1492

PietroPasotti opened this issue Dec 9, 2024 · 1 comment · May be fixed by #1497

Comments

@PietroPasotti
Copy link
Contributor

Scenario's Context object was designed with the old 'metadata/actions/config.yaml`s in mind.
Nowadays we have a single charmcraft.yaml, which makes the signature obsolete and confusing.
I recently helped someone debug this code:

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

which failed with a strange AttributeError caused by the charm attempting to observe(self.on.foo_relation_changed).
The reason is that Context will only attempt to autoload meta/actions/config from the yaml files IF the user doesn't specify any of the meta, config, actions Context params.

The fix was:

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

But it was not obvious.

It's not clear that the 'config' param in Context doesn't mean "juju config" but "the config section in charmcraft.yaml".
Can we improve this without breaking backwards compatibility?

@tonyandrewmeyer
Copy link
Contributor

Some possible alternatives to #1497:

Just warn

Something like this (with some of the better docs from #1497, a mention in the Context docs, and a test):

diff --git a/testing/src/scenario/context.py b/testing/src/scenario/context.py
index 411beed9..5f053527 100644
--- a/testing/src/scenario/context.py
+++ b/testing/src/scenario/context.py
@@ -510,6 +510,11 @@ class Context(Generic[CharmType]):
         else:
             if not meta:
                 meta = {"name": str(charm_type.__name__)}
+                warnings.warn(
+                    "No meta provided: only the charm name is set.",
+                    RuntimeWarning,
+                    stacklevel=2,
+                )

Deprecate all three, add from_yaml to replace

Emit deprecation warnings if meta, actions, or config are provided and add something like:

class Context[...]:
    ...
    @classmethod
    def from_yaml(charm_type: ..., charmcraft_yaml: dict) -> Context[...]:
        # private attributes to pass the things, or do it post __init__ or whatever
        return charm_type()

    @classmethod
    def from_legacy_yaml(charm_type: ..., meta: dict, *, config: dict|None, actions: dict|None) -> Context[...]:
        ...

Although the from_ methods would need to also handle passing in other arguments for the Context __init__ I guess.

Deprecate all three, use charmcraft to replace

Emit deprecation warnings if meta, actions, or config are provided and add a new charmcraft (yaml? charmcraft_yaml?) argument that expects the combined version. No confusion with the State's config, and no awkwardness about which are/aren't required since there's only one. If you have the separate files then you have to combine them yourself (or I suppose we could provide a utility method for that).

Autoload unless None

A None value for meta, config, or actions could mean "try to load from the YAML file", rather than all being false-y meaning that. I think this is what the user was expecting? We'd obviously be able to skip looking for any files if all three were provided, but in many cases (like just meta being provided) this would be more file operations, and maybe more backwards incompatible?

Validate the config YAML

If the issue is particularly with config because it's confusing whether it's the State's config or the YAML, we could reject a plain dictionary very early (probably also actions).

Would people providing something like {'foo': {'type': 'string', 'default': 'bar'}} still be confused? Probably not about this being the current value of the config, but maybe that the other metadata wasn't loaded?

We could do this as well as other things.

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 a pull request may close this issue.

2 participants