test: Extend the type checking of the ops-scenario tests #2235

tonyandrewmeyer · 2025-12-14T04:47:32Z

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 half of the test_e2e test files (the other half are in another PR, #2234).

The class variable gets adjusted in the ruff fixes PR, but we can handle the merge conflict later.

The type: ignore should be able to go away when the storage class gets annotations, which is in another PR in this series. We can handle the merge then.

The class vars are being adjusted in another PR. We can handle the merge later.

The storage type ignore we should be able to remove after the other PRs are merged.

Copilot

Pull request overview

This PR extends type checking coverage to the ops-scenario test suite by adding type annotations to half of the test_e2e test files and configuring pyright to check them. The remaining files are explicitly excluded until a follow-up PR.

Key Changes

Added type annotations to test functions, fixtures, and event handlers across 9 test files
Updated pyright configuration to include testing/tests directory while excluding files not yet annotated
Added necessary type narrowing assertions where types could be None

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pyproject.toml	Added testing/tests to pyright includes and specified exclusions for unannotated files
test_storage.py	Added type annotations to fixtures and test functions; added type ignore comments for filesystem operations
test_status.py	Added EventBase import and return type annotations for all event handlers and test functions
test_secrets.py	Added comprehensive type annotations including Literal types for owner parameters and type narrowing assertions
test_rubbish_events.py	Added ClassVar annotations for class attributes and type annotations for test functions
test_relations.py	Moved StateValidationError import to errors module; added type annotations and cast operations for dynamic types
test_ports.py	Moved StateValidationError import to errors module; added return type annotations
test_pebble.py	Added TYPE_CHECKING block for LayerDict/ServiceDict; corrected period/timeout to string format; added type annotations throughout
test_manager.py	Moved AlreadyEmittedError import to errors module; added type annotations for fixtures and tests
test_juju_log.py	Added type annotations to fixtures and test functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

testing/tests/test_e2e/test_juju_log.py

benhoyt · 2025-12-14T20:02:34Z

Thanks for doing these. Out of interest, did you use a tool to help automate these?

tonyandrewmeyer · 2025-12-14T20:10:48Z

Thanks for doing these. Out of interest, did you use a tool to help automate these?

Not an agent, but I did use the tab-completion functionality in VS Code a reasonable amount. I find it's particularly useful when what you're doing is very repetitive like this was.

Not sure if you remember or not, but I tried getting Copilot to do this work in the Gothenburg workshop (and then over the following days) and it did a terrible job. I still feel that an agent ought to be able to do this and that I probably just needed a different prompt or configuration, but I wasn't willing to spend more time on that with this ticket.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

benhoyt · 2025-12-14T20:14:41Z

Not an agent, but I did use the tab-completion functionality in VS Code a reasonable amount. I find it's particularly useful when what you're doing is very repetitive like this was.

Not sure if you remember or not, but I tried getting Copilot to do this work...

Yeah, I did remember. I was more curious about whether you'd used a mechanical tool like https://github.com/orsinium-labs/infer-types (have no idea whether it works or is any good).

tonyandrewmeyer · 2025-12-14T20:16:05Z

I was more curious about whether you'd used a mechanical tool like https://github.com/orsinium-labs/infer-types (have no idea whether it works or is any good).

Ah, interesting. I wasn't aware of such tools, otherwise I probably would have given one a go :)

…thod.

james-garner-canonical · 2025-12-15T00:14:24Z

testing/tests/test_e2e/test_juju_log.py

 def mycharm():
    class MyCharm(CharmBase):
-        META = {'name': 'mycharm'}
+        META: dict[str, Any] = {'name': 'mycharm'}


I don't think the type annotations on the META and ACTIONS attributes get us anything right now tbh. Though why aren't we getting complaints about mutable class attributes needing ClassVar? If we do annotate these it should probably be Mapping[str, Any]. And we should investigate why we're not getting the ClassVar complaint.

Kind of superseded by lifting these to the top-level if we can do that, see other comment.

I think the ClassVar bit comes from ruff, not from pyright. There's overlap here with #2228

Ah maybe that's where all the deja vu is coming from. Could we perhaps merge #2228 before working further on these then?

james-garner-canonical · 2025-12-15T00:16:27Z

testing/tests/test_e2e/test_manager.py



-def test_manager(mycharm):
+def test_manager(mycharm: Any) -> None:


WDYT about lifting MyCharm and its META and ACTIONS attributes to the top-level like we did for canonical/grafana-agent-operator#355?

I guess this PR will need the -> None annotations stripped as well.

testing/tests/test_e2e/test_pebble.py

dimaqq · 2025-12-16T05:34:27Z

testing/tests/test_e2e/test_rubbish_events.py


 @pytest.mark.parametrize('evt_name', ('rubbish', 'foo', 'bar'))
-def test_rubbish_event_raises(mycharm: CharmBase, evt_name: str):
+def test_rubbish_event_raises(mycharm: type[CharmBase], evt_name: str):


question: why are we not changing CharmBase to ops.CharmBase in this file? Is it because it's covered by another PR?

P.S. I wonder if a type alias for type[ops.CharmBase] would be handy here.

I was trying to avoid doing the change from from ops import CharmBase to import ops as much as possible to keep the size/scope down. I was originally just doing it when there was a new import needed (most often Framework). Then when I merged in the ruff PR there were a bunch of merge conflicts in most of the imports so I also simplified those.

I guess I could uniformly make this change, but I think it would probably be better as a separate PR, and it would be nice if it was after this one so I don't have to redo those changes.

+1 on leaving it for a separate PR

testing/tests/test_e2e/test_storage.py

dimaqq · 2025-12-19T04:18:38Z

testing/src/scenario/state.py

        return infos

-    def get_filesystem(self, ctx: Context) -> pathlib.Path:
+    def get_filesystem(self, ctx: Context[CharmBase]) -> pathlib.Path:


Has this become ctx: Context[Any] in the other PR?

tonyandrewmeyer added 11 commits December 13, 2025 21:28

ci: switch to individual files for type checking scenario tests.

a5dadbd

test: add type annotations for test_secrets.

8aed1fb

test: add type annotations to test_relations.

4a15063

ci: remove unnecessary exclude.

56c1b98

test: add type annotations to test_juju_log

796720b

The class variable gets adjusted in the ruff fixes PR, but we can handle the merge conflict later.

test: add type annotations for test_status.

5d255f0

test: add type annotations to test_storage

0f5ca37

The type: ignore should be able to go away when the storage class gets annotations, which is in another PR in this series. We can handle the merge then.

test: add type annotations to test_manager.

ffa4e13

The class vars are being adjusted in another PR. We can handle the merge later.

test: add type annotations to test_ports.

362feb7

test: add type annotations to test_rubbish_events.

dbce9c0

test: add type annotaitons for test_pebble.

0f97dd7

The storage type ignore we should be able to remove after the other PRs are merged.

tonyandrewmeyer requested review from Copilot, dimaqq and james-garner-canonical December 14, 2025 04:47

Copilot started reviewing on behalf of tonyandrewmeyer December 14, 2025 04:47 View session

Copilot AI reviewed Dec 14, 2025

View reviewed changes

testing/tests/test_e2e/test_juju_log.py Outdated Show resolved Hide resolved

Update testing/tests/test_e2e/test_juju_log.py

04ef78c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

tonyandrewmeyer added 2 commits December 15, 2025 09:35

fix: update the asserts to match the corrected layer values.

be57cb5

fix: correct the call and type annotation, we're replacing a bound me…

2eb63d2

…thod.

james-garner-canonical reviewed Dec 15, 2025

View reviewed changes

tonyandrewmeyer added 2 commits December 15, 2025 14:49

chore: remove -> None.

55a7930

chore: remove scopes from type: ignore.

e243933

tonyandrewmeyer mentioned this pull request Dec 16, 2025

test: Extend the type checking of the ops-scenario tests #2234

Open

tonyandrewmeyer added 2 commits December 16, 2025 14:55

Merge origin/main.

d58d19a

Post merge fixes.

effb48f

dimaqq reviewed Dec 16, 2025

View reviewed changes

tonyandrewmeyer added 3 commits December 19, 2025 08:23

Add generic type to avoid ignore.

7028747

Reformat per review request.

19c2982

Remove return type annotation per review request.

f884387

dimaqq reviewed Dec 19, 2025

View reviewed changes



		def test_manager(mycharm):
		def test_manager(mycharm: Any) -> None:

test: Extend the type checking of the ops-scenario tests #2235

Are you sure you want to change the base?

test: Extend the type checking of the ops-scenario tests #2235

Uh oh!

Conversation

tonyandrewmeyer commented Dec 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

benhoyt commented Dec 14, 2025

Uh oh!

tonyandrewmeyer commented Dec 14, 2025

Uh oh!

benhoyt commented Dec 14, 2025

Uh oh!

tonyandrewmeyer commented Dec 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james-garner-canonical Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

james-garner-canonical Dec 15, 2025 •

edited

Loading