Run Pyright on test files to catch typing issues #1007
Assignees
Labels
Comments
This was referenced Sep 22, 2023
This was referenced Sep 24, 2023
benhoyt
pushed a commit
that referenced
this issue
Sep 26, 2023
* Add type hints to tests/test_helpers * For fake_script and fake_script_calls, there are a bunch of `type: ignore`s to work around not being able to state the type of `fake_script_path` - I think there will be a few of these through other test modules as well, as hints are added there. I feel it would be cleaner to have the fake script functionality in a class rather than dynamically added to TestCase instances, but [that is a much more substantial change](tonyandrewmeyer@46238ee) Partially addresses #1007
This was referenced Sep 27, 2023
benhoyt
pushed a commit
that referenced
this issue
Sep 29, 2023
One minor (type) change to ops.testing: the harness `pull()` signature is changed to allow `encoding==None`, which matches the actual `pebble.Client` method.
Otherwise, changes are in test_testing:
* Explicitly ignore types where we are testing invalid type behaviour
* Use `assert`s to show type checkers that `obj|None` is always `obj` in a few places - this seems better tn tests than `cast(obj_type, obj)` and pyright doesn't understand that this is the case with `assertIsNotNone(obj)`
* Remove two `print()` calls, one of which seems to be a debugging print that slipped in, and one that's not required although the expression being printed is
* Replace a few `dict(a=b, c=d)` with `{'a': b, 'c': d}` as pyright seems to understand the latter much more easily (plus it's insignificantly faster and generally more recommended style)
* Fix `test_push_with_permission_mask` where the exception's `kind` attribute was (presumably in error) only being tested for one of the test cases
* In the `_PebbleStorageAPIsTestMixin` class make it clear (especially for type checkers, but also in general) that a `client` attribute must be provided by the eventual class
* In the `_PebbleStorageAPIsTestMixin` class make it clear to type checkers that various TestCase methods will be provided (via mixing with `unittest.TestCase`)
* Sprinkle type hints where required for pyright to be happy
Partially addresses #1007
benhoyt
pushed a commit
that referenced
this issue
Sep 29, 2023
* One change to ops/charm, but only in the type hints. It looks from the use of `Required` that the other fields are not meant to be required, so make that the case (this also appears to match the code). * Ignore types in the tests that are checking that bad types are handled properly. * Help type checkers understand when some optional attributes won't be None. * Replace a few bits of code where a minimal object was passed, enough for the test to do it's check, but not of the correct type, with an actual object of the proper type. * Sprinkle type hints where required for pyright to understand what's happening. Partially fixes #1007
This was referenced Oct 2, 2023
benhoyt
pushed a commit
that referenced
this issue
Oct 3, 2023
* Change the type of `Handle`'s `key` (in `__init__` and `nest()` and the property) to explicitly allow `None * Add a CHANGES.md file to document changes (backfilled only to 2.7) * Tidy up the type hinting of `BoundStoredState.__getattr__` - multiple overloads need to be provided, and once that's done the ignores can be removed (and a bunch of type issues in the tests get resolved) * Ignore type where creating an object that's only partially complete, but the test only requires that much and fully creating it would be a lot of extra code for no real gain * Where's it's simple to do, pass the required attributes rather than `None` * Ignore types where we're checking handling of invalid types * Add casts around the very dynamic snapshot/restore functionality * For a couple of the "list of test cases" situations, move the documentation of what each element in the test case is to a new type definition that covers both what was there before and the (rough) expected type * Sprinkle type hints where required for pyright to be happy Partially addresses #1007
benhoyt
pushed a commit
that referenced
this issue
Oct 3, 2023
* Fix `_TestMain.test_multiple_events_handled`: this broke in f398e12 * The last part of the test would never execute (the `if` was always false) * The departing unit's name is provided in a `departing_unit` attribute not `departing_unit_name` * It seems like at some point `state.__getitem__` was replaced with `state.__getattr__` as well, but I didn't dig into that. * Later, when the secret events were added, the attributes listed were not all correct (but since the test never run that part, this wasn't noticed) * Also when the secret events were added, no snapshot was added to the charm, so none of the data was available * Fix the indentation in an `ops.Framework` docstring. * Add type hints to the `test_main` charm. As part of this, I did a drive-by change to use "import ops" style, to avoid importing many different types of event. * Add type hints in the smoke testing * Ignore types where we are testing that the type is invalid * When using the fake script functionality from test_helpers with an ABC, cast the case to a `unittest.TestCase` since we know that's what it will be at run time (this seems a bit ugly, but I think maybe the best choice without doing more significant changes) * Ignore types in the `test_smoke` code where they are not provided by `pytest-operator` or `libjuju`. * Sprinkle enough type hints around test_main to make pyright happy Partially addresses #1007
benhoyt
pushed a commit
that referenced
this issue
Oct 6, 2023
* Adds overloads for pebble's `pull` method to clarify what you get when you do/do not provide an encoding.
* `test/pebble_cli.py` has a bit of a workaround - the type returned by `exec` is generic, and I believe we could have pyright understand which one was being used, but by repeating the `exec` call in the various `if` statements above. It seems like this would make the code messier for no real gain, so I've cheated and told pyright it will be a `[str]` even though that may not be true.
* Add a few asserts to show pyright that values aren't None or are the correct type
* Add `type: ignore' where we are checking that types are invalid
* Sprinkle type hints as required for pyright to understand what's happening
Note that the latest version of pyright has a problem where we use code like this:
```python
with self.assertRaises(ops.pebble.ExecError) as cm:
call_something()
self.assertEqual(cm.exception, some_value)
```
The issue is related to `ExecError` being generic combined with `cm` being generic and dynamic. The version of pyright we are currently using does not have an issue with this - but looking at the pyright release notes (1.1.318) it seems like accepting this is considered a bug and failing it in the latest version is correct. However, it's not clear what we are meant to do here, and the documentation (stdlib and pyright) is vague, so I'm leaving this to figure out (if necessary) when we upgrade our pyright version.
We now type check everything under `test/`, except for the smoke test (which needs to wait until pytest-operator and pylibjuju have more type hinting).
Fixes #1007
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We currently don't run Pyright over our
test/*.pyfiles to type-check those, meaning we're not getting "coverage" of most of our type annotations, which results in issues like #963.We should enable Pyright on these files. I suspect this will be a fairly big diff -- if so, we may want to opt the files in one by one in separate PRs to streamline review.
The text was updated successfully, but these errors were encountered: