Add process execution simulation to test harness #993

weiiwang01 · 2023-08-22T02:27:31Z

The Pebble process execution plays an important role in a lot of charms. However, the ops framework testing module currently lacks built-in functionality to simulate this process, leading many charms to roll their own patching and mocking mechanisms for unit tests.

To address this problem, this pull request introduces ops.testing.Harness.handle_exec, a new feature that enables the simulation of Pebble process execution within the Ops framework testing module.

See spec OP039 for discussion.

benhoyt · 2023-08-22T04:34:40Z

@weiiwang01 Is this ready for review? If so, I can take a look tomorrow.

Thanks for the pebble.py None-safety fix. Are you able to separate that out into a separate PR, with a regression test of that code path if possible?

One other thing: it looks like the docs are failing right now. You can generate those locally with tox -e docs and then open docs/_build/html/index.html to review the docs locally.

weiiwang01 · 2023-08-22T16:41:04Z

@weiiwang01 Is this ready for review? If so, I can take a look tomorrow.

Thanks for the pebble.py None-safety fix. Are you able to separate that out into a separate PR, with a regression test of that code path if possible?

One other thing: it looks like the docs are failing right now. You can generate those locally with tox -e docs and then open docs/_build/html/index.html to review the docs locally.

Yes, this is ready for review. I have fixed the document issue and reverted changes in pebble.py. I will create a new PR for the None safety issue later.

benhoyt

This is great work again, thanks @weiiwang01. I've left a bunch of comments, but most of them are minor -- structurally it seems fine.

benhoyt · 2023-08-22T23:54:32Z

ops/pebble.py

-        t = _start_thread(shutil.copyfileobj, self.stdout, out)
-        self._threads.append(t)
+        # self.stdout can be None, when exec is invoked with the stdout argument
+        if self.stdout is not None:


I think the correct fix here is to raise an exception in this case (at the top of the method), because ExecProcess.wait_output shouldn't be called if exec is invoked with the stdout argument. wait_output is intended to be used when you want to collect output as a string. If using exec's stdout argument, you'd use ExecProcess.wait. I'd probably raise TypeError with a nice message.

We can tweak the docstring to be more explicit about that too.

I've rolled back the changes. Let's address this issue specifically in the next pull request. I hope there's a way so we can use some type gymnastics to avoid using exceptions while keep the typing.

ops/testing.py

benhoyt · 2023-08-23T01:07:43Z

ops/testing.py

+        if self._exit_code != 0:
+            raise pebble.ExecError(self._command, cast(int, self._exit_code), None, None)
+
+    def wait_output(self) -> Tuple[Optional[AnyStr], Optional[AnyStr]]:


We should change this back to the same signature (non-optional stdout) and check that they didn't pass stdout in to exec -- similar to my first comment.

I think this is a private class within the testing module. I believe this function signature has no effect. I'll update it in the next pull request to align with pebble.py.

Perhaps it has no effect, but let's change it to the same signature as the real wait_output in this PR to avoid confusion.

Updated, thanks!

test/test_testing.py

Co-authored-by: Ben Hoyt <[email protected]>

benhoyt

Thanks for the updates! This is very close now. Let's just make the simplification to use a dict for the registered handlers, and I've added a couple of other minor suggestions.

I also tweaked the handle_exec docstring and fixed it in a couple of places, and reverted the line-break change to ops/pebble.py to avoid that in the diff.

benhoyt · 2023-08-24T02:24:58Z

ops/testing.py

+        if isinstance(result, int):
+            result = ExecResult(exit_code=result)
+        if isinstance(result, (str, bytes)):
+            result = ExecResult(stdout=result)


How about this, to help people not using static type checking? The "not bool" test is because bools are ints, but we don't want someone confusing things by returning True (1) or False (0).

Suggested change

if isinstance(result, int):

result = ExecResult(exit_code=result)

if isinstance(result, (str, bytes)):

result = ExecResult(stdout=result)

if isinstance(result, int) and not isinstance(result, bool):

result = ExecResult(exit_code=result)

elif isinstance(result, (str, bytes)):

result = ExecResult(stdout=result)

elif not isinstance(result, ExecResult):

raise TypeError(f"result must be int, str, bytes, or ExecResult, not {result.__class__.__name__}")

Never know the bool is int, but I guess it makes sense, updated, thanks!

benhoyt · 2023-08-24T02:26:49Z

ops/testing.py

+    def wait(self):
+        if self._is_timeout:
+            raise pebble.TimeoutError(
+                f'timed out waiting for change 123 ({self._timeout} seconds)'


What I was suggesting here is that we simply remove the change ID, the " 123" part, from the error message entirely. I think it will be confusing to have a hard-coded value here.

Gotcha, removed, thanks!

benhoyt · 2023-08-24T02:28:13Z

ops/testing.py

+        if self._exit_code != 0:
+            raise pebble.ExecError(self._command, cast(int, self._exit_code), None, None)
+
+    def wait_output(self) -> Tuple[Optional[AnyStr], Optional[AnyStr]]:


Perhaps it has no effect, but let's change it to the same signature as the real wait_output in this PR to avoid confusion.

benhoyt · 2023-08-24T02:31:37Z

ops/testing.py

+                self._exec_handlers.insert(idx, (prefix, handler))
+                inserted = True
+        if not inserted:
+            self._exec_handlers.append((prefix, handler))


Yeah, I think we should -- the current code is fairly tricky and complex, for no practical benefit in this use case. We're only ever going to have a small handful of registered handlers, so whizzing through them linearly each time is fine. To get simple code, let's use a dict and a _find_exec_handler along the lines of what I suggested.

test/test_testing.py

benhoyt

Excellent, thanks. Love the new simple lookup technique! Just a couple of nits and then we're good to go.

benhoyt · 2023-08-24T03:54:40Z

ops/testing.py

+            raise TypeError("Either handler or result must be provided, but not both.")
+        container_name = container if isinstance(container, str) else container.name
+        if result is None:
+            pass


Ah yes, thanks. Though this confused me a bit at first. How about we put this in an indented block and perform the checks and set the handlers to lambda _: result only if result is not None (then we can skip the slightly awkward inline if-else below):

if result is not None: if isinstance(result, int) and not isinstance(result, bool): result = ExecResult(exit_code=result) elif isinstance(result, (str, bytes)): result = ExecResult(stdout=result) elif not isinstance(result, ExecResult): raise TypeError( f"result must be int, str, bytes, or ExecResult, not {result.__class__.__name__}") handler = lambda _: result

Understood, updated. However, I can't change handler = lambda _: result because of the linting checks.

Ugh, I hate it when our tools get in the way of clean simple code. I guess we could do this, but I'm not sure it's any better. I leave it up to you. I'll probably merge this tomorrow!

# autopep8: off handler = lambda _: result # noqa # autopep8: on

Three annotations for a single statement might be a little bit too much. I'll probably keep the original statement, thanks.

ops/testing.py

benhoyt · 2023-08-24T20:49:14Z

prometheus-k8s-operator unit tests are failing, but that's not related to this PR -- I've made them aware.

Merging! Thanks for your work on this, @weiiwang01.

weiiwang01 added 5 commits August 21, 2023 22:59

Add handle_exec to test harness

d772657

Fix static check and linting issues

6c6d4af

Fix some linting issues

e4c1079

Use list sort instead of bisect due to Python 3.8

32f5b19

Better handler registration

9be8ab9

Revert changes in wait_output and fix docs

33297a9

benhoyt requested changes Aug 23, 2023

View reviewed changes

weiiwang01 and others added 16 commits August 23, 2023 19:19

Update ops/testing.py

648f96d

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

d3abeb7

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

f981383

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

7116791

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

b429c15

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

37c104a

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

2be5318

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

76dfd77

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

78df99a

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

83096f6

Co-authored-by: Ben Hoyt <[email protected]>

Update ops/testing.py

7ff8695

Co-authored-by: Ben Hoyt <[email protected]>

Apply suggestions from code review

5d087bb

Co-authored-by: Ben Hoyt <[email protected]>

Apply suggestions from code review

90a49b7

Update handle_exec document

6742e9b

Remove changes to ops/pebble.py

0e36c4c

Minor handle_exec docstring tweaks and fixes

eea02d6

benhoyt requested changes Aug 24, 2023

View reviewed changes

weiiwang01 added 2 commits August 24, 2023 11:39

simplify the exec handler storage and lookup

b9e9c69

Update exception messages in harness

c570f75

benhoyt approved these changes Aug 24, 2023

View reviewed changes

Minor refactoring

4b603af

Merge branch 'main' into feat-handle-exec

031f566

benhoyt merged commit b1c14d7 into canonical:main Aug 24, 2023
17 of 18 checks passed

weiiwang01 deleted the feat-handle-exec branch August 27, 2023 14:45

benhoyt mentioned this pull request Sep 21, 2023

Enhancement request: simulate command execution in test harness #932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add process execution simulation to test harness #993

Add process execution simulation to test harness #993

weiiwang01 commented Aug 22, 2023 •

edited

Loading

benhoyt commented Aug 22, 2023

weiiwang01 commented Aug 22, 2023

benhoyt left a comment

benhoyt Aug 22, 2023

weiiwang01 Aug 23, 2023

benhoyt Aug 23, 2023

weiiwang01 Aug 23, 2023

benhoyt Aug 24, 2023

weiiwang01 Aug 24, 2023

benhoyt left a comment

benhoyt Aug 24, 2023

weiiwang01 Aug 24, 2023

benhoyt Aug 24, 2023

weiiwang01 Aug 24, 2023

benhoyt Aug 24, 2023

benhoyt Aug 24, 2023

benhoyt left a comment

benhoyt Aug 24, 2023

weiiwang01 Aug 24, 2023

benhoyt Aug 24, 2023

weiiwang01 Aug 24, 2023

benhoyt commented Aug 24, 2023

Add process execution simulation to test harness #993

Add process execution simulation to test harness #993

Conversation

weiiwang01 commented Aug 22, 2023 • edited Loading

benhoyt commented Aug 22, 2023

weiiwang01 commented Aug 22, 2023

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt commented Aug 24, 2023

weiiwang01 commented Aug 22, 2023 •

edited

Loading