chore: solve and remove the testing/tests linting ignores #2228

tonyandrewmeyer · 2025-12-11T20:53:30Z

Removes all linting exceptions from testing/tests, fixing the underlying issues.

Changes:

Added copyright notices: Added missing copyright headers to all test files
Fixed import ordering: Reorganised imports (standard library → third party → first party)
Improved type annotations:
- Removed unnecessary quotes from type annotations
- Added ClassVar[] annotations for mutable class attributes
Enhanced code safety:
- Replaced blind exception assertions with specific exception types
- Fixed useless attribute access (properly using noqa: B018 where intentional)
- Used context managers for file operations to ensure timely cleanup
Code style improvements:
- Fixed line length violations
- Used next(iter()) instead of indexing with [0] for iterables
- Changed hardcoded /tmp paths to more generic alternatives
- Maintained CamelCase for class names

Each commit addresses (and removes) a single exception, so reviewing commit-by-commit may be the simplest approach.

Copilot

Pull request overview

This PR removes all linting exceptions from the testing/tests directory by fixing the underlying code issues. The changes modernize the test code to meet current Python style guidelines and best practices while maintaining functionality.

Key changes:

Added copyright headers to all test files for proper licensing attribution
Reorganized imports to follow the standard order (standard library → third party → first party)
Enhanced type safety with ClassVar annotations and specific exception types

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
testing/tests/test_runtime.py	Added copyright header and reordered imports
testing/tests/test_plugin.py	Added copyright header
testing/tests/test_emitted_events_util.py	Added copyright header, reordered imports, and added ClassVar annotation for META
testing/tests/test_e2e/test_vroot.py	Added copyright header, reordered imports, and added ClassVar annotation for META
testing/tests/test_e2e/test_trace_data.py	Added copyright header and reordered imports
testing/tests/test_e2e/test_stored_state.py	Added copyright header, reordered imports, added ClassVar annotations, and renamed import to PascalCase
testing/tests/test_e2e/test_storage.py	Added copyright header, reordered imports, and added ClassVar annotations with proper formatting
testing/tests/test_e2e/test_status.py	Added copyright header and reordered imports
testing/tests/test_e2e/test_state.py	Added copyright header, reordered imports, removed quotes from type annotations, replaced blind exceptions with specific types, and replaced indexing with next(iter())
testing/tests/test_e2e/test_secrets.py	Added copyright header, reordered imports
testing/tests/test_e2e/test_rubbish_events.py	Added copyright header, reordered imports, and added ClassVar annotation
testing/tests/test_e2e/test_resource.py	Added copyright header, reordered imports, and changed hardcoded /tmp path to /var/charm
testing/tests/test_e2e/test_relations.py	Added copyright header, reordered imports, and removed quotes from type annotations
testing/tests/test_e2e/test_ports.py	Added copyright header, reordered imports, added ClassVar annotation, and replaced indexing with next(iter())
testing/tests/test_e2e/test_play_assertions.py	Added copyright header and reordered imports
testing/tests/test_e2e/test_pebble.py	Added copyright header, reordered imports, and wrapped file operations in context managers
testing/tests/test_e2e/test_network.py	Added copyright header, reordered imports, and added noqa comments for intentional attribute access
testing/tests/test_e2e/test_manager.py	Added copyright header, reordered imports, and added ClassVar annotations
testing/tests/test_e2e/test_juju_log.py	Added copyright header, reordered imports, and added ClassVar annotation
testing/tests/test_e2e/test_event.py	Added copyright header, reordered imports, and added ClassVar annotations
testing/tests/test_e2e/test_deferred.py	Added copyright header, reordered imports, and added ClassVar annotations
testing/tests/test_e2e/test_config.py	Added copyright header and reordered imports
testing/tests/test_e2e/test_cloud_spec.py	Added copyright header and reordered imports
testing/tests/test_e2e/test_actions.py	Added copyright header and reordered imports
testing/tests/test_e2e/conftest.py	Added copyright header and reordered imports
testing/tests/test_e2e/init.py	Added copyright header
testing/tests/test_context_on.py	Added copyright header, reordered imports, and updated comment text
testing/tests/test_context.py	Added copyright header and reordered imports
testing/tests/test_consistency_checker.py	Added copyright header, reordered imports, and removed quotes from type annotations
testing/tests/test_charm_spec_autoload.py	Added copyright header and reordered imports
testing/tests/helpers.py	Added copyright header, reordered imports, and removed quotes from type annotations
testing/tests/init.py	Added copyright header
pyproject.toml	Removed all linting exceptions that were previously suppressed for testing/tests directory

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

testing/tests/test_context_on.py

testing/tests/test_e2e/test_event.py

james-garner-canonical

Thanks for doing this. My only blocking comment is that ClassVar[dict[... for the charm metadata is probably better as Mapping[..., but I have a couple of other suggestions too. If Mapping doesn't work (e.g. it turns out we mutate the metadata?), then fine as-is. Approving optimistically.

james-garner-canonical · 2025-12-11T21:02:46Z

testing/tests/helpers.py

            assert len(tuple(state.relations)) == 1, 'shortcut only works with one relation'
-            event = getattr(ctx.on, event)(tuple(state.relations)[0])
+            event = getattr(ctx.on, event)(next(iter(state.relations)))
        elif event.startswith('pebble_'):
            assert len(tuple(state.containers)) == 1, 'shortcut only works with one container'
-            event = getattr(ctx.on, event)(tuple(state.containers)[0])
+            event = getattr(ctx.on, event)(next(iter(state.containers)))


These are improvements on their own, but feel weird given that we still call tuple(...) on the preceding line. Also unfortunate that we need to do len(tuple(... instead of just len( given that these are actually going to be frozenset.

I wonder if it would be worth adding custom __init__ methods to the relevant classes (just State?) to correct the mismatch between the relations arg only needing to be Iterable, but the relations attribute actually always being a frozenset -- should I create an issue for this? Or discuss at daily?

In the meantime, if we're open to making some manual improvements in this PR, how about this:

if event.startswith('relation_'): (relation,) = state.relations # shortcut only works with one relation event = getattr(ctx.on, event)(relation) elif event.startswith('pebble_'): (container,) = state.containers # shortcut only works with one container event = getattr(ctx.on, event)(container)

But feel free to reject to minimise the diff.

I wonder if it would be worth adding custom __init__ methods to the relevant classes (just State?) to correct the mismatch between the relations arg only needing to be Iterable, but the relations attribute actually always being a frozenset -- should I create an issue for this? Or discuss at daily?

Yeah, this has come up a couple of times before.

We did actually have a version where they all had an __init__. The primary reason was to support keyword-only arguments in Python 3.8, and we eventually decided that having in one place (the thing with the custom __new__) was superior and ripped them all out. But they did also nicely handle the "you can give me an X but the attribute will always be a Y" case, so we lost that at the same time. Obviously, we have since ripped out the custom solution.

I'm definitely not opposed to going back to __init__. I think if we fix it for one it would be best to fix it for them all. Looking through, that's:

CloudCredential (Mapping->dict, Sequence->list, really not sure why that is not tuple!)

CloudSpec(Sequence->list, really not sure why that is not tuple!)

Secret(Mapping->RawSecretRevisionContents, could probably just be dict; Mapping->dict)

BindAddress(Sequence->tuple, at the moment stays a list if passed a list)

And probably lots more (I wouldn't be surprised if it's like 80% of the classes). However, it could also potentially get rid of or incorporate the code we have for doing the copy (for somewhat immutability).

Feel free to open a PR or add to daily.

testing/tests/test_e2e/test_deferred.py

testing/tests/test_e2e/test_event.py

testing/tests/test_e2e/test_juju_log.py

testing/tests/test_e2e/test_stored_state.py

testing/tests/test_e2e/test_storage.py

testing/tests/test_e2e/test_ports.py

testing/tests/test_e2e/test_manager.py

testing/tests/test_e2e/test_juju_log.py

…uggestion.

tonyandrewmeyer · 2025-12-12T01:01:44Z

It was a kind thought :)

…

On Fri, Dec 12, 2025 at 1:28 PM James Garner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In testing/tests/test_emitted_events_util.py <#2228 (comment)>: > from __future__ import annotations -from ops.charm import CharmBase, CharmEvents, CollectStatusEvent, StartEvent -from ops.framework import CommitEvent, EventBase, EventSource, PreCommitEvent +from typing import Any, ClassVar Oops, guess I shouldn't have overzealously tried to streamline things with all those suggestions 😄 — Reply to this email directly, view it on GitHub <#2228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGJZGSCZYMAIKUTLTINB534BID25AVCNFSM6AAAAACOY4LCZOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNRZHAYTOMBXGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

… aren't going to change, so type as Mapping rather than ClassVar[dict]. Suggestion from code review.

This PR fixes a linting error in the test newly added in #2219, which is needed due to the linting updates in #2228.

tonyandrewmeyer added 14 commits December 12, 2025 09:13

chore: add copyright notices.

d61a163

chore: Fix import order.

327766b

chore: catch the specific exception that is expected.

6f36b0d

chore: noqa an unnecessary warning.

c91fb9b

chore: fix long lines.

3e03937

ci: remove unnecessary ignore.

70e6547

chore: keep class names in CamelCase.

4acbefe

ci: remove unnecessary ignore.

2fb3987

chore: use a fake path that doesn't look like hardcode /tmp

80cf009

ci: remove unnecessary ignore.

385cd8f

chore: remove quotes from types.

909e77b

chore: add ClassVar[] where appropriate.

1b2eb3f

chore: use next(iter()) rather than [0].

527c1f3

chore: Use context managers for more timely cleanup.

e1e0147

tonyandrewmeyer requested review from Copilot, dimaqq and james-garner-canonical December 11, 2025 20:53

Copilot started reviewing on behalf of tonyandrewmeyer December 11, 2025 20:53 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

testing/tests/test_context_on.py Show resolved Hide resolved

testing/tests/test_e2e/test_event.py Show resolved Hide resolved

james-garner-canonical approved these changes Dec 11, 2025

View reviewed changes

tonyandrewmeyer added 2 commits December 12, 2025 13:19

refactor: use the tmp_path fixture rather than tempfile, per review s…

c29b527

…uggestion.

chore: consistently use ops.CharmBase, per review suggestion.

4345d8b

refactor: the expectation with the .META attribute is that the values…

93797af

… aren't going to change, so type as Mapping rather than ClassVar[dict]. Suggestion from code review.

This was referenced Dec 15, 2025

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

Open

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

Open

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

Open

dimaqq approved these changes Dec 16, 2025

View reviewed changes

tonyandrewmeyer merged commit 6c2bdee into canonical:main Dec 16, 2025
55 of 56 checks passed

tonyandrewmeyer deleted the type-check-scenario-tests branch December 16, 2025 01:27

james-garner-canonical mentioned this pull request Dec 17, 2025

test: update new scenario test to fix linting error #2244

Merged

james-garner-canonical added a commit that referenced this pull request Dec 18, 2025

test: update new scenario test to fix linting error (#2244)

7a656f8

This PR fixes a linting error in the test newly added in #2219, which is needed due to the linting updates in #2228.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: solve and remove the testing/tests linting ignores #2228

chore: solve and remove the testing/tests linting ignores #2228

Uh oh!

tonyandrewmeyer commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

james-garner-canonical left a comment

Uh oh!

james-garner-canonical Dec 11, 2025

Uh oh!

tonyandrewmeyer Dec 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonyandrewmeyer commented Dec 12, 2025 via email

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore: solve and remove the testing/tests linting ignores #2228

chore: solve and remove the testing/tests linting ignores #2228

Uh oh!

Conversation

tonyandrewmeyer commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

james-garner-canonical left a comment

Choose a reason for hiding this comment

Uh oh!

james-garner-canonical Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

tonyandrewmeyer Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonyandrewmeyer commented Dec 12, 2025 via email

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants