-
Notifications
You must be signed in to change notification settings - Fork 125
test: Extend the type checking of the ops-scenario tests #2234
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
base: main
Are you sure you want to change the base?
test: Extend the type checking of the ops-scenario tests #2234
Conversation
…l resolve it later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets factored out in one of these PRs doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| include = ["ops/*.py", "ops/_private/*.py", "test/*.py", "test/charms/*/src/*.py", "testing/src/*.py", "testing/tests/*.py", "testing/tests/test_e2e/*.py"] | ||
| exclude = [ | ||
| "tracing/*", | ||
| "testing/tests/helpers.py", | ||
| "testing/tests/test_charm_spec_autoload.py", | ||
| "testing/tests/test_consistency_checker.py", | ||
| "testing/tests/test_context_on.py", | ||
| "testing/tests/test_context.py", | ||
| "testing/tests/test_emitted_events_util.py", | ||
| "testing/tests/test_plugin.py", | ||
| "testing/tests/test_runtime.py", | ||
| "testing/tests/test_e2e/test_secrets.py", | ||
| "testing/tests/test_e2e/test_relations.py", | ||
| "testing/tests/test_e2e/test_trace_data.py", | ||
| "testing/tests/test_e2e/test_juju_log.py", | ||
| "testing/tests/test_e2e/test_status.py", | ||
| "testing/tests/test_e2e/test_storage.py", | ||
| "testing/tests/test_e2e/test_manager.py", | ||
| "testing/tests/test_e2e/test_ports.py", | ||
| "testing/tests/test_e2e/test_rubbish_events.py", | ||
| "testing/tests/test_e2e/test_pebble.py", | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea to make this more incremental?
Also, how does it work when a path is excluded but also matches an included glob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea to make this more incremental?
Not sure what you mean by this. Do you mean will there be follow-ups to do the rest? If so, yes, #2230, #2235. Do you mean incremental in this PR? It started as one file per commit, but then after review I have not done any squashing so there is a mixed bag after that.
Also, how does it work when a path is excluded but also matches an included glob?
Include first, exclude second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I'd like to see is that eventually the long hand-crafted list of excludes goes away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It goes away when all three PRs are merged; they each do about a third of the files.
|
|
||
| @pytest.fixture(scope='function') | ||
| def mycharm(): | ||
| def mycharm() -> type[ops.CharmBase]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.