-
Notifications
You must be signed in to change notification settings - Fork 129
test: Extend the type checking of the ops-scenario tests #2234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9a4bab3
0f3eb00
14b2923
34adbbd
8350e8d
0a8c1d5
63c039a
58e19b0
d82d82a
eceb259
49893fc
86ff536
eab3f63
5f62371
c43500e
f45fcb8
c070335
b95c0c7
85b2b0f
efc5473
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,17 +3,20 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Any | ||
|
|
||
| import pytest | ||
| from scenario import Context | ||
| from scenario.state import State, _Action, _next_action_id | ||
|
|
||
| from ops._private.harness import ActionFailed | ||
| from ops.charm import ActionEvent, CharmBase | ||
| from ops.framework import Framework | ||
| from ops.version import version as ops_version | ||
|
|
||
|
|
||
| @pytest.fixture(scope='function') | ||
| def mycharm(): | ||
| def mycharm() -> type[CharmBase]: | ||
| class MyCharm(CharmBase): | ||
| _evt_handler = None | ||
|
|
||
|
|
@@ -22,16 +25,16 @@ def __init__(self, framework: Framework): | |
| for evt in self.on.events().values(): | ||
| self.framework.observe(evt, self._on_event) | ||
|
|
||
| def _on_event(self, event): | ||
| def _on_event(self, event: ActionEvent): | ||
| if handler := self._evt_handler: | ||
| handler(event) | ||
|
|
||
| return MyCharm | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('baz_value', (True, False)) | ||
| def test_action_event(mycharm, baz_value): | ||
| ctx = Context( | ||
| def test_action_event(mycharm: type[CharmBase], baz_value: bool): | ||
| ctx: Context[CharmBase] = Context( | ||
| mycharm, | ||
| meta={'name': 'foo'}, | ||
| actions={'foo': {'params': {'bar': {'type': 'number'}, 'baz': {'type': 'boolean'}}}}, | ||
|
|
@@ -47,11 +50,11 @@ def test_action_event(mycharm, baz_value): | |
|
|
||
| def test_action_no_results(): | ||
| class MyCharm(CharmBase): | ||
| def __init__(self, framework): | ||
| def __init__(self, framework: Framework): | ||
| super().__init__(framework) | ||
| framework.observe(self.on.act_action, self._on_act_action) | ||
|
|
||
| def _on_act_action(self, _): | ||
| def _on_act_action(self, _: ActionEvent): | ||
| pass | ||
|
|
||
| ctx = Context(MyCharm, meta={'name': 'foo'}, actions={'act': {}}) | ||
|
|
@@ -61,27 +64,27 @@ def _on_act_action(self, _): | |
|
|
||
|
|
||
| @pytest.mark.parametrize('res_value', ('one', 1, [2], ['bar'], (1,), {1, 2})) | ||
| def test_action_event_results_invalid(mycharm, res_value): | ||
| def test_action_event_results_invalid(mycharm: type[CharmBase], res_value: object): | ||
| def handle_evt(charm: CharmBase, evt: ActionEvent): | ||
| with pytest.raises((TypeError, AttributeError)): | ||
| evt.set_results(res_value) | ||
| evt.set_results(res_value) # type: ignore | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
| ctx.run(ctx.on.action('foo'), State()) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('res_value', ({'a': {'b': {'c'}}}, {'d': 'e'})) | ||
| def test_action_event_results_valid(mycharm, res_value): | ||
| def handle_evt(_: CharmBase, evt): | ||
| def test_action_event_results_valid(mycharm: type[CharmBase], res_value: dict[str, Any]): | ||
| def handle_evt(_: CharmBase, evt: ActionEvent): | ||
| if not isinstance(evt, ActionEvent): | ||
| return | ||
| evt.set_results(res_value) | ||
| evt.log('foo') | ||
| evt.log('bar') | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
|
|
||
|
|
@@ -91,7 +94,7 @@ def handle_evt(_: CharmBase, evt): | |
|
|
||
|
|
||
| @pytest.mark.parametrize('res_value', ({'a': {'b': {'c'}}}, {'d': 'e'})) | ||
| def test_action_event_outputs(mycharm, res_value): | ||
| def test_action_event_outputs(mycharm: type[CharmBase], res_value: dict[str, Any]): | ||
| def handle_evt(_: CharmBase, evt: ActionEvent): | ||
| if not isinstance(evt, ActionEvent): | ||
| return | ||
|
|
@@ -101,7 +104,7 @@ def handle_evt(_: CharmBase, evt: ActionEvent): | |
| evt.log('log2') | ||
| evt.fail('failed becozz') | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
| with pytest.raises(ActionFailed) as exc_info: | ||
|
|
@@ -111,27 +114,27 @@ def handle_evt(_: CharmBase, evt: ActionEvent): | |
| assert ctx.action_logs == ['log1', 'log2'] | ||
|
|
||
|
|
||
| def test_action_event_fail(mycharm): | ||
| def test_action_event_fail(mycharm: type[CharmBase]): | ||
| def handle_evt(_: CharmBase, evt: ActionEvent): | ||
| if not isinstance(evt, ActionEvent): | ||
| return | ||
| evt.fail('action failed!') | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
| with pytest.raises(ActionFailed) as exc_info: | ||
| ctx.run(ctx.on.action('foo'), State()) | ||
| assert exc_info.value.message == 'action failed!' | ||
|
|
||
|
|
||
| def test_action_event_fail_context_manager(mycharm): | ||
| def test_action_event_fail_context_manager(mycharm: type[CharmBase]): | ||
| def handle_evt(_: CharmBase, evt: ActionEvent): | ||
| if not isinstance(evt, ActionEvent): | ||
| return | ||
| evt.fail('action failed!') | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
| with pytest.raises(ActionFailed) as exc_info: | ||
|
|
@@ -142,11 +145,11 @@ def handle_evt(_: CharmBase, evt: ActionEvent): | |
|
|
||
| def test_action_continues_after_fail(): | ||
| class MyCharm(CharmBase): | ||
| def __init__(self, framework): | ||
| def __init__(self, framework: Framework): | ||
| super().__init__(framework) | ||
| framework.observe(self.on.foo_action, self._on_foo_action) | ||
|
|
||
| def _on_foo_action(self, event): | ||
| def _on_foo_action(self, event: ActionEvent): | ||
| event.log('starting') | ||
| event.set_results({'initial': 'result'}) | ||
| event.fail('oh no!') | ||
|
|
@@ -160,44 +163,55 @@ def _on_foo_action(self, event): | |
| assert ctx.action_results == {'initial': 'result', 'final': 'result'} | ||
|
|
||
|
|
||
| def test_action_event_has_id(mycharm): | ||
| def _ops_less_than(wanted_major: int, wanted_minor: int) -> bool: | ||
| major, minor = (int(v) for v in ops_version.split('.')[:2]) | ||
| if major < wanted_major: | ||
| return True | ||
| if major == wanted_major and minor < wanted_minor: | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| @pytest.mark.skipif(_ops_less_than(2, 11), reason="ops 2.10 and earlier don't have ActionEvent.id") | ||
|
Comment on lines
+166
to
+175
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So shouldn't |
||
| def test_action_event_has_id(mycharm: type[CharmBase]): | ||
| def handle_evt(_: CharmBase, evt: ActionEvent): | ||
| if not isinstance(evt, ActionEvent): | ||
| return | ||
| assert isinstance(evt.id, str) and evt.id != '' | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
| ctx.run(ctx.on.action('foo'), State()) | ||
|
|
||
|
|
||
| def test_action_event_has_override_id(mycharm): | ||
| @pytest.mark.skipif(_ops_less_than(2, 11), reason="ops 2.10 and earlier don't have ActionEvent.id") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here. |
||
| def test_action_event_has_override_id(mycharm: type[CharmBase]): | ||
| uuid = '0ddba11-cafe-ba1d-5a1e-dec0debad' | ||
|
|
||
| def handle_evt(charm: CharmBase, evt: ActionEvent): | ||
| if not isinstance(evt, ActionEvent): | ||
| return | ||
| assert evt.id == uuid | ||
|
|
||
| mycharm._evt_handler = handle_evt | ||
| mycharm._evt_handler = handle_evt # type: ignore | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I drop something like this at the top of the file: class MyCharm(CharmBase):
_evt_handler = Callable[[CharmBase, ActionEvent], None]And update the test argument to WDYT about that, or perhaps a Protocol?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess you disagree with this comment? I think we'd get value out of being able to turn on static type checking for these files now, and deferring further iteration til later. |
||
|
|
||
| ctx = Context(mycharm, meta={'name': 'foo'}, actions={'foo': {}}) | ||
| ctx.run(ctx.on.action('foo', id=uuid), State()) | ||
|
|
||
|
|
||
| def test_two_actions_same_context(): | ||
| class MyCharm(CharmBase): | ||
| def __init__(self, framework): | ||
| def __init__(self, framework: Framework): | ||
| super().__init__(framework) | ||
| framework.observe(self.on.foo_action, self._on_foo_action) | ||
| framework.observe(self.on.bar_action, self._on_bar_action) | ||
|
|
||
| def _on_foo_action(self, event): | ||
| def _on_foo_action(self, event: ActionEvent): | ||
| event.log('foo') | ||
| event.set_results({'foo': 'result'}) | ||
|
|
||
| def _on_bar_action(self, event): | ||
| def _on_bar_action(self, event: ActionEvent): | ||
| event.log('bar') | ||
| event.set_results({'bar': 'result'}) | ||
|
|
||
|
|
@@ -213,7 +227,7 @@ def _on_bar_action(self, event): | |
|
|
||
| def test_positional_arguments(): | ||
| with pytest.raises(TypeError): | ||
| _Action('foo', {}) | ||
| _Action('foo', {}) # type: ignore | ||
|
|
||
|
|
||
| def test_default_arguments(): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets factored out in one of these PRs doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I mentioned it in the commit message but probably should have put it in the description as well. My intention is to merge each of these in turn, dealing with the merge conflicts at the time, rather than try to set them up as a sequence to go into a single merge into main or anything consistent like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure this doesn't get reintroduced by this PR, I'm not sure what order these PRs are ending up landing in now.