feat: Make Context and Manager variadic types

[Batalex]

Closes #1444.

This change enables passing the Charm type all the way from the Context to the charm attribute in the manager for better autocompletion and type checks.

You can give this a try with the following code snippet:

from ops import CharmBase
from testing.src.scenario import Context, State


class MyCharm(CharmBase):
    some_attribute: str


def test_function():
    ctx = Context(MyCharm)
    state = State()

    with ctx(ctx.on.config_changed(), state) as manager:
        charm = manager.charm
        charm.some_attribute  # behold, autocompletion!

A word of caution, though, the testing module is not using the bundled scenario code base. It is still using ops_scenario. For users to benefit from this new feature, we would need to change the import machinery.

[dimaqq]

I like this, but will defer to typing experts for details and py 3.8 compatibility.

[james-garner-canonical]

Changes make sense to me. I think CharmEvents.__call__ should have the return type annotated with the new generic type Manager[CharmType]. I don't have much experience with scenario, so I'll have to let @tonyandrewmeyer catch whether there are any problems I didn't notice on that front.

[james-garner-canonical]

Confused me a little at first, but this works because CharmType is already being used as a parameter for (at least) the __init__ method (line 448).

__call__ (line 568) should be annotated as returning a Manager[CharmType], no? I guess the types are inferred correctly without the annotation, but still.

[tonyandrewmeyer]

I wonder if we also ought to be pushing this through to the self.ops object, so ops_main_mock.Ops has it as well. I think that means that the cast on line 94 could be an "assert not None" line instead - I'm not sure if there are other benefits as well. @james-garner-canonical any thoughts on that?

[james-garner-canonical]

Doing that would probably be correct as I see that the Ops class has a charm_spec argument that's parameterised on charm_spec (_CharmSpec[CharmType]). Luckily this will already be correctly parameterised when constructed since Ops is initialised via the (now generic) Context. So it should be as simple as changing class Ops to class Ops(Generic[CharmType]) and changing the type annotation in Manager.__init__.

Line 90 should probably be if self.ops is None rather than if not self.ops, right? Then you should be able to eliminate the cast without needing an assert.

[Batalex]

Is this what you had in mind? a4a37d8

[james-garner-canonical]

Just a comment: this threw me at first, but CharmType is being imported in the unconditional imports too, so I see that this is just removing a redundant duplicate import

[tonyandrewmeyer]

A word of caution, though, the testing module is not using the bundled scenario code base. It is still using ops_scenario. For users to benefit from this new feature, we would need to change the import machinery.

I'm not sure what you mean by this? Do you mean that ops.testing gets populated with objects from the scenario namespace? That's by design and isn't changing - having multiple packages avoids having testing code packed into the charm.

There will be new ops-scenario releases (generally every month, alongside ops releases), so installing ops[testing] will get you the latest stable testing code, accessible in the ops.testing namespace (or if you want code from a local location, you'd do a local install, like pip install .[testing], and the objects are still in the ops.testing namespace).

Having the ops-scenario code in the operator repository, alongside the ops code, is mostly for convenience - it's one fewer repo for the team to manage, it's easier to do PRs that change both the core and testing code at the same time, and so on.

If I've misunderstood and you meant something else, could you please elaborate?

[tonyandrewmeyer]

Tested manually with:

import ops

class MyCharm(ops.CharmBase):
    foo: str
    ""This is an example attribute, of type string."""

And:

from ops import testing
from charm import MyCharm

def test_function():
    ctx = testing.Context(MyCharm)

    with ctx(ctx.on.config_changed(), testing.State()) as mgr:
        mgr.charm.foo

(With a uv pip install . and uv pip install testing/ install from this branch).

And I did indeed get nice autocomplete (including the type and doc, as you'd expect) as well as the improved static checking. Ideally, tests aren't needing to access the charm object in most cases, but this is a nice improvement when they do.

I also had no trouble running tests with the code from this branch and with Python 3.8, although I only did some basic checks.

[tonyandrewmeyer]

Generally looks good to me, although I'm not an expert on Python type annotations. Manual testing works as well (as posted separately).

I wonder if we could have a test for this - not necessarily testing the type explicitly (although that would be fine), but one that would exercise it so that the static checks would fail if we regressed on this?

[tonyandrewmeyer]

I wonder if we also ought to be pushing this through to the self.ops object, so ops_main_mock.Ops has it as well. I think that means that the cast on line 94 could be an "assert not None" line instead - I'm not sure if there are other benefits as well. @james-garner-canonical any thoughts on that?

[Batalex]

A word of caution, though, the testing module is not using the bundled scenario code base. It is still using ops_scenario. For users to benefit from this new feature, we would need to change the import machinery.

I'm not sure what you mean by this? Do you mean that ops.testing gets populated with objects from the scenario namespace? That's by design and isn't changing - having multiple packages avoids having testing code packed into the charm.

There will be new ops-scenario releases (generally every month, alongside ops releases), so installing ops[testing] will get you the latest stable testing code, accessible in the ops.testing namespace (or if you want code from a local location, you'd do a local install, like pip install .[testing], and the objects are still in the ops.testing namespace).

Having the ops-scenario code in the operator repository, alongside the ops code, is mostly for convenience - it's one fewer repo for the team to manage, it's easier to do PRs that change both the core and testing code at the same time, and so on.

If I've misunderstood and you meant something else, could you please elaborate?

@tonyandrewmeyer Thank you, I had not realized at the time that the two packages would be built from this single repo, and I was confused that the original repo was being archived. No problem then :)

I wonder if we could have a test for this - not necessarily testing the type explicitly (although that would be fine), but one that would exercise it so that the static checks would fail if we regressed on this?

There is not a lot of content around testing type annotations, I found this: https://typing.readthedocs.io/en/latest/reference/quality.html#testing-using-assert-type-and-warn-unused-ignores.
We would need to add a type checker as part of the test suite, which might be a big task. Happy to give it a try if you think that's within scope for this PR, though

[PietroPasotti]

A word of caution, though, the testing module is not using the bundled scenario code base. It is still using ops_scenario. For users to benefit from this new feature, we would need to change the import machinery.

I'm not sure what you mean by this? Do you mean that ops.testing gets populated with objects from the scenario namespace? That's by design and isn't changing - having multiple packages avoids having testing code packed into the charm.
There will be new ops-scenario releases (generally every month, alongside ops releases), so installing ops[testing] will get you the latest stable testing code, accessible in the ops.testing namespace (or if you want code from a local location, you'd do a local install, like pip install .[testing], and the objects are still in the ops.testing namespace).
Having the ops-scenario code in the operator repository, alongside the ops code, is mostly for convenience - it's one fewer repo for the team to manage, it's easier to do PRs that change both the core and testing code at the same time, and so on.
If I've misunderstood and you meant something else, could you please elaborate?

@tonyandrewmeyer Thank you, I had not realized at the time that the two packages would be built from this single repo, and I was confused that the original repo was being archived. No problem then :)

I wonder if we could have a test for this - not necessarily testing the type explicitly (although that would be fine), but one that would exercise it so that the static checks would fail if we regressed on this?

There is not a lot of content around testing type annotations, I found this: https://typing.readthedocs.io/en/latest/reference/quality.html#testing-using-assert-type-and-warn-unused-ignores. We would need to add a type checker as part of the test suite, which might be a big task. Happy to give it a try if you think that's within scope for this PR, though

I did some type checker tests in the past, IIRC using reveal_type

I reckon this should work:

# in tests/resources/example_typing.py
from ops.testing import XYZ
reveal_type(XYZ)

# in tests/test_typing_annotations.py
def test_type_annotation():
    proc = Popen("mypy ../resources/example_typing.py", stdout=PIPE, text=True)
    assert "<what type you expect to be printed.>" in proc.stdout.read()

[dimaqq]

We've had a few discussions on checking the types earlier this year.

There were pros and cons, and fancier type tests did not get accepted.

What got accepted is here: operator/test/test_main_type_hint.py

[tonyandrewmeyer]

Looks good to me, thanks.

Note that I have a PR open that refactors a bunch of this code - whichever one lands first, I'll merge across the changes to the other one.

[dimaqq]

waiting for @james-garner-canonical whose concern appears to have been addressed, ptal.