test: Extend the type checking of the ops-scenario tests #2230
Conversation
tonyandrewmeyer
requested review from
Copilot,
dimaqq and
james-garner-canonical
|
This moves #1785 closer to completion but there are still many files to go (probably at least 2 more PRs, to keep the size manageable). |
There was a problem hiding this comment.
Pull request overview
This PR extends type checking coverage for the ops-scenario test suite by adding type annotations to test files outside of test_e2e (which are deferred to a future PR) and configuring pyright to check these files.
Key changes:
- Added comprehensive type annotations to test functions and charm classes
- Updated pyproject.toml to include testing/tests in pyright checking with explicit exclusions for remaining test_e2e files
- Added return type annotation to Runtime.exec method to support the type checking improvements
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testing/tests/test_runtime.py | Added type annotations to test functions, charm classes, and variable declarations; moved UncaughtCharmError import to scenario.errors |
| testing/tests/test_plugin.py | Added pytest.Pytester type annotation and type ignore for pytester.makepyfile |
| testing/tests/test_emitted_events_util.py | Added type annotations to charm classes and test functions |
| testing/tests/test_e2e/test_state.py | Removed import of unused sort_patch function; inlined sorting logic where needed |
| testing/tests/test_context_on.py | Added comprehensive type annotations including proper Literal types for owner parameter and type casts for narrowing event types |
| testing/tests/test_context.py | Added type annotations to test functions and moved from CharmBase import to ops.CharmBase usage |
| testing/tests/test_consistency_checker.py | Added type annotations to helper functions and test parameters |
| testing/tests/test_charm_spec_autoload.py | Added type annotations including Generator return types and imports reorganization |
| testing/tests/helpers.py | Removed sort_patch function and inlined sorting into jsonpatch_delta; added proper type annotations and cast for jsonpatch types |
| testing/src/scenario/_runtime.py | Added Generator return type annotation to Runtime.exec method and made Runtime generic over CharmType |
| pyproject.toml | Extended pyright include to cover testing/tests and added explicit exclusion list for test_e2e files not yet annotated |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| META = { | ||
| META: dict[str, Any] = { |
There was a problem hiding this comment.
| META: dict[str, Any] = { | |
| META: Mapping[str, Any] = { |
So the type checker helps us not to mutate this.
There was a problem hiding this comment.
#2228 has changes related to this, it'd be nice to get that merged rather than making more conflicting changes.
There was a problem hiding this comment.
Now that #2228 is merged, I actually think there's no overlap here, and we should just go with Mapping so the type checker helps us avoid mutating the META global.
…ests2 # Conflicts: # testing/tests/helpers.py # testing/tests/test_charm_spec_autoload.py # testing/tests/test_consistency_checker.py # testing/tests/test_context.py # testing/tests/test_e2e/test_state.py # testing/tests/test_emitted_events_util.py # testing/tests/test_runtime.py
| meta: dict[str, Any] = { | ||
| 'name': app_name, | ||
| } |
There was a problem hiding this comment.
Rather than adding a type annotation, how about inlining this in the charm_spec creation below?
meta = {'name': app_name},| def test_env_clean_on_charm_error(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setenv('SCENARIO_BARE_CHARM_ERRORS', 'false') | ||
| meta = {'name': 'frank', 'requires': {'box': {'interface': 'triangle'}}} | ||
| meta: dict[str, Any] = {'name': 'frank', 'requires': {'box': {'interface': 'triangle'}}} |
There was a problem hiding this comment.
I'd inline this one too.
meta={'name': 'frank', 'requires': {'box': {'interface': 'triangle'}}},
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
I've finished my review of this PR, see individual comments and replies.
Co-authored-by: James Garner <james.garner@canonical.com>
There was a problem hiding this comment.
this file has been removed in the other PR, please merge main into your branch and resolve the conflicts
4 participants
testing/testsis 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 the test files (all of the ones outside of test_e2e, which are left for another PR).
To service this, add a type annotation to
Runtime.exec.