test: Extend the type checking of the ops-scenario tests#2234
test: Extend the type checking of the ops-scenario tests#2234tonyandrewmeyer wants to merge 19 commits intocanonical:mainfrom
Conversation
…l resolve it later).
There was a problem hiding this comment.
Pull request overview
This PR extends type checking to the ops-scenario test suite by adding type annotations to approximately half of the test files in testing/tests/test_e2e/. The remaining files are excluded from pyright checking for future work.
Key changes:
- Added comprehensive type annotations to test functions, fixtures, and helper functions
- Introduced necessary
type: ignorecomments where dynamic attributes or test patterns require them - Made the codebase more type-safe by adding assertions for nullable values and runtime type checks
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated pyright configuration to include test files and exclude files still pending type annotation work |
| testing/tests/test_e2e/test_vroot.py | Added type annotations for fixture and test functions, including Generator type hint |
| testing/tests/test_e2e/test_stored_state.py | Added type annotations and replaced import alias with direct ops.StoredState reference |
| testing/tests/test_e2e/test_state.py | Added extensive type annotations, refined callable signatures, and added runtime type assertions for better type checking |
| testing/tests/test_e2e/test_resource.py | Added type annotation to init method |
| testing/tests/test_e2e/test_play_assertions.py | Added type annotations and consolidated ops imports |
| testing/tests/test_e2e/test_network.py | Added type annotations and introduced intermediate variables with null checks for type safety |
| testing/tests/test_e2e/test_event.py | Added type annotations and parameterized _CharmSpec with type variable |
| testing/tests/test_e2e/test_deferred.py | Added type annotations throughout test functions and fixtures |
| testing/tests/test_e2e/test_config.py | Added type annotations and renamed unused variables to follow convention |
| testing/tests/test_e2e/test_cloud_spec.py | Added type annotations to test functions and event handlers |
| testing/tests/test_e2e/test_actions.py | Added type annotations, including Action event-specific handler signatures |
| testing/tests/helpers.py | Added type annotations to helper functions including Callable type for sort_patch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def sort_patch( | ||
| patch: list[dict[str, Any]], | ||
| key: Callable[[dict[str, Any]], str] = lambda obj: obj['path'] + obj['op'], | ||
| ) -> list[dict[str, Any]]: |
There was a problem hiding this comment.
This gets factored out in one of these PRs doesn't it?
There was a problem hiding this comment.
Yes. I mentioned it in the commit message but probably should have put it in the description as well. My intention is to merge each of these in turn, dealing with the merge conflicts at the time, rather than try to set them up as a sequence to go into a single merge into main or anything consistent like that.
|
|
||
| @pytest.fixture(scope='function') | ||
| def mycharm(): | ||
| def mycharm() -> type[ops.CharmBase]: |
There was a problem hiding this comment.
AI-generated alternative that would make things simpler, I think:
class _MyCharmTemplate(ops.CharmBase):
META: dict[str, typing.Any] = {
"name": "mycharm",
"requires": {"foo": {"interface": "bar"}},
"containers": {"foo": {"type": "oci-image"}},
}
defer_next = 0
captured: list[ops.EventBase] = []
def __init__(self, framework: ops.Framework):
super().__init__(framework)
for evt in self.on.events().values():
if issubclass(evt.event_type, ops.LifecycleEvent):
continue
self.framework.observe(evt, self._on_event)
def _on_event(self, event: ops.EventBase):
self.captured.append(event)
if self.defer_next > 0:
self.defer_next -= 1
event.defer()
def make_mycharm_class() -> type[ops.CharmBase]:
# Fresh class object each call; class name can be constant.
return type(
"MyCharm",
(_MyCharmTemplate,),
{
# fresh per-test class attributes
"META": copy.deepcopy(_MyCharmTemplate.META),
"defer_next": 0,
"captured": [],
"__module__": __name__, # optional: nicer repr / pickling expectations
},
)
@pytest.fixture(scope="function")
def mycharm() -> type[ops.CharmBase]:
return make_mycharm_class()This way we can pass a real charm class type around, and not ops.CharmBase.
There was a problem hiding this comment.
I guess more pragmatically:
class MyCharm(ops.CharmBase):
META = ...
...
@pytest.fixture
def mycharm() -> type[MyCharm]:
"""Create a brand new MyCharm for each test."""
return type("MyCharm", (MyCharm,), {"META": ..., ...})
testing/tests is added to the files that pyright should check, and the files that are left to do are added to the exclusion list.
Add type annotations to roughly half of the test/test_e2e files (the rest are left for another PR).
See also #2230.