ci: check httpbin-demo example charm with mypy#2339
ci: check httpbin-demo example charm with mypy#2339james-garner-canonical wants to merge 7 commits intocanonical:mainfrom
Conversation
This should fix mypy thinking that ops.testing.Container is ops.model.Container rather than scenario.Container.
|
Excellent, thanks for this. Let's discuss in daily when Tony and Dima are both back (I've added it to the top of the doc). |
|
my 2c: let's bite the bullet: merge, call it out in release notes, but don't bump the major version. |
| _compatibility_names = [ | ||
| 'CharmBase', | ||
| 'CharmMeta', | ||
| 'Container', # If Scenario has been installed, then this will be scenario.Container. |
There was a problem hiding this comment.
Do we have containers in Harness, at all?
There was a problem hiding this comment.
Harness exported the name Container (the object ops.model.Container) for a long time, and that was kept when we added Scenario to the ops.testing namespace to avoid breaking test code that (lazily / incorrectly) would use Container imported from there, like from ops.testing import Container, Harness, ....
tonyandrewmeyer
left a comment
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
I don't like that this makes CI changes in the same PR (and that's not mentioned at all in the PR summary even though it's in the description). If we want to do that (and I think it's an entirely separate question) then I think it belongs in a dedicated PR.
The CI changes are essentially tests that exercise fix in the PR and would fail on EDIT: Updated PR summary to mention this. |
My concern here is that I think the CI change (basically that we are committing to some increased level of support for mypy) is the biggest change here, and adjusting If we are going to explicitly support I don't think it's the right approach to have the more significant change as a drive-by. |
Happy to reframe the PR that way. I guess if we opt to merge #2343 (with manual verification) this PR can become CI changes only. |
|
Per discussion, change this PR to just "add mypy to the example charms". |
…le checkers understand it (#2343) Test code, based on the code in the Matrix thread, but fixed: ```python from typing import Iterable import ops import ops.testing import pytest execs: Iterable[ops.testing.Exec] = set() @pytest.fixture() def mediawiki_container(): return ops.testing.Container( name="mediawiki", can_connect=True, execs=execs, ) class TestPebbleReadyEvent: def test_can_connect(self, ctx: ops.testing.Context[ops.CharmBase], mediawiki_container: ops.testing.Container) -> None: state_in = ops.testing.State(containers=[mediawiki_container], leader=True) ctx.run(ctx.on.update_status(), state_in) ``` With main: ``` tameyer@tam-canoncial-1:~/w/operator$ uvx --with=pytest --with=ops[testing] mypy --follow-imports=silent /tmp/mypy_example.py /tmp/mypy_example.py:21: error: List item 0 has incompatible type "ops.model.Container"; expected "scenario.state.Container" [list-item] Found 1 error in 1 file (checked 1 source file) tameyer@tam-canoncial-1:~/w/operator$ uvx --with=pytest --with=ops[testing] pyright /tmp/mypy_example.py 0 errors, 0 warnings, 0 informations ``` With this branch: ``` tameyer@tam-canoncial-1:~/w/fix-mypy-testing-container$ uvx --with=pytest --with=ops[testing] mypy --follow-imports=silent /tmp/mypy_example.py Success: no issues found in 1 source file tameyer@tam-canoncial-1:~/w/fix-mypy-testing-container$ uvx --with=pytest --with=ops[testing] pyright /tmp/mypy_example.py 0 errors, 0 warnings, 0 informations ``` By moving the import from `ops.model` to the `except` block, type checkers like mypy understand that it's one or the other. We also need a `type: ignore` there because mypy otherwise complains about incompatible imports. This is an alternative to #2339.
james-garner-canonical
force-pushed
the
26-02+fix+testing-mypy
branch
from
8f7412a to
75f23cf
Compare
james-garner-canonical
changed the title
|
Closing in favour of: |
4 participants
This PR updates the
httpbin-demoexample charm to addmypyto its existing linting. I've verified that this linting fails with the reported error without the update toops/testing.pymade in #2343.To keep this check current, I've updated
tool.uv.sourcesfor the charm to pullopsfrom the local files -- but this introduces a gotcha, since we'd need to relock every time we change theopsversion. I've updated the release script to do this, but I wonder if we'd be better off adding a separate CI step instead.We could add
mypyto the linting for our other example charms, the K8s and machine tutorial charms. However, this wouldn't reflect what we actually want charmers to do when following the tutorial, so we shouldn't do so on disc. Instead, it would probably be cleaner to add a custom CI job like below. If we did so, we could use the same approach for thehttpbin-democharm. #2360 implements this approach.