Repository: canonical/operator
Pull request #213 (2020-04-03 20:28:46)

Added breakpoint manual call to the framework.

facundobatista: Comments addressed, and added a welcome message to the user. Thanks!

Patch:
diff --git a/ops/framework.py b/ops/framework.py
index 03e1c88..848f847 100755
--- a/ops/framework.py
+++ b/ops/framework.py
@@ -1,4 +1,4 @@
-# Copyright 2019 Canonical Ltd.
+# Copyright 2019-2020 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -12,14 +12,18 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import inspect
-import pickle
-import marshal
-import types
-import sqlite3
 import collections
 import collections.abc
+import inspect
 import keyword
+import marshal
+import os
+import pdb
+import pickle
+import re
+import sqlite3
+import sys
+import types
 import weakref
 from datetime import timedelta
 
@@ -472,6 +476,16 @@ class SQLiteStorage:
                 yield tuple(row)
 
 
+# the message to show to the user when a pdb breakpoint goes active
+_BREAKPOINT_WELCOME_MESSAGE = """
+Starting pdb to debug charm operator.
+Run `h` for help, `c` to continue, or `exit`/CTRL-d to abort.
+Future breakpoints may interrupt execution again.
+More details at https://discourse.jujucharms.com/t/debugging-charm-hooks
+
+"""
+
+
 class Framework(Object):
 
     on = FrameworkEventSet()
@@ -506,6 +520,13 @@ class Framework(Object):
             self._stored = StoredStateData(self, '_stored')
             self._stored['event_count'] = 0
 
+        # Hook into builtin breakpoint, so if Python >= 3.7, devs will be able to just do
+        # breakpoint(); if Python < 3.7, this doesn't affect anything
+        sys.breakpointhook = self.breakpoint
+
+        # Flag to indicate that we already presented the welcome message in a debugger breakpoint
+        self._breakpoint_welcomed = False
+
     def close(self):
         self._storage.close()
 
@@ -734,6 +755,43 @@ class Framework(Object):
         if not deferred:
             self._storage.drop_snapshot(last_event_path)
 
+    def breakpoint(self, name=None):
+        """Add breakpoint, optionally named, at the place where this method is called.
+
+        For the breakpoint to be activated the JUJU_DEBUG_AT environment variable
+        must be set to "all" or to the specific name parameter provided, if any. In every
+        other situation calling this method does nothing.
+
+        The framework also provides a standard breakpoint named "hook", that will
+        stop execution when a hook event is about to be handled.
+
+        For those reasons, the "all" and "hook" breakpoint names are reserved.
+        """
+        # If given, validate the name comply with all the rules
+        if name is not None:
+            if not isinstance(name, str):
+                raise TypeError('breakpoint names must be strings')
+            if name in ('hook', 'all'):
+                raise ValueError('breakpoint names "all" and "hook" are reserved')
+            if not re.match(r'^[a-z0-9]([a-z0-9\-]*[a-z0-9])?$', name):
+                raise ValueError('breakpoint names must look like "foo" or "foo-bar"')
+
+        debug_at = os.environ.get('JUJU_DEBUG_AT')
+        if not debug_at:
+            return
+
+        indicated_breakpoints = debug_at.split(',')
+        if 'all' in indicated_breakpoints or name in indicated_breakpoints:
+            # Present the welcome message (only once!)
+            if not self._breakpoint_welcomed:
+                self._breakpoint_welcomed = True
+                print(_BREAKPOINT_WELCOME_MESSAGE, file=sys.stderr, end='')
+
+            # If we call set_trace() directly it will open the debugger *here*, so indicating
+            # it to use our caller's frame
+            code_frame = inspect.currentframe().f_back
+            pdb.Pdb().set_trace(code_frame)
+
 
 class StoredStateChanged(EventBase):
     pass


Test patch:
diff --git a/test/test_framework.py b/test/test_framework.py
index 08369ee..699b64b 100644
--- a/test/test_framework.py
+++ b/test/test_framework.py
@@ -12,15 +12,20 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import unittest
-import tempfile
-import shutil
-import gc
 import datetime
-
+import gc
+import inspect
+import io
+import os
+import shutil
+import sys
+import tempfile
+import unittest
+from unittest.mock import patch
 from pathlib import Path
 
 from ops.framework import (
+    _BREAKPOINT_WELCOME_MESSAGE,
     BoundStoredState,
     CommitEvent,
     EventBase,
@@ -1369,3 +1374,156 @@ class TestStoredState(unittest.TestCase):
         self.assertEqual(parent._stored.bar, 4)
         # TODO: jam 2020-01-30 is there a clean way to tell that
         #       parent._stored._data.dirty is False?
+
+
+@patch('sys.stderr', new_callable=io.StringIO)
+class BreakpointTests(unittest.TestCase):
+
+    def setUp(self):
+        tmpdir = Path(tempfile.mkdtemp())
+        self.addCleanup(shutil.rmtree, str(tmpdir))
+
+        self.framework = Framework(tmpdir / "framework.data", tmpdir, None, None)
+        self.addCleanup(self.framework.close)
+
+    def test_ignored(self, fake_stderr):
+        # It doesn't do anything really unless proper environment is there.
+        assert 'JUJU_DEBUG_AT' not in os.environ
+
+        with patch('pdb.Pdb.set_trace') as mock:
+            self.framework.breakpoint()
+        self.assertEqual(mock.call_count, 0)
+        self.assertEqual(fake_stderr.getvalue(), "")
+
+    def test_pdb_properly_called(self, fake_stderr):
+        # The debugger needs to leave the user in the frame where the breakpoint is executed,
+        # which for the test is the frame we're calling it here in the test :).
+        with patch.dict(os.environ, {'JUJU_DEBUG_AT': 'all'}):
+            with patch('pdb.Pdb.set_trace') as mock:
+                this_frame = inspect.currentframe()
+                self.framework.breakpoint()
+        self.assertEqual(mock.call_count, 1)
+        self.assertEqual(mock.call_args, ((this_frame,), {}))
+
+    def test_welcome_message(self, fake_stderr):
+        # Check that an initial message is shown to the user when code is interrupted.
+        with patch.dict(os.environ, {'JUJU_DEBUG_AT': 'all'}):
+            with patch('pdb.Pdb.set_trace'):
+                self.framework.breakpoint()
+        self.assertEqual(fake_stderr.getvalue(), _BREAKPOINT_WELCOME_MESSAGE)
+
+    def test_welcome_message_not_multiple(self, fake_stderr):
+        # Check that an initial message is NOT shown twice if the breakpoint is exercised
+        # twice in the same run.
+        with patch.dict(os.environ, {'JUJU_DEBUG_AT': 'all'}):
+            with patch('pdb.Pdb.set_trace'):
+                self.framework.breakpoint()
+                self.assertEqual(fake_stderr.getvalue(), _BREAKPOINT_WELCOME_MESSAGE)
+                self.framework.breakpoint()
+                self.assertEqual(fake_stderr.getvalue(), _BREAKPOINT_WELCOME_MESSAGE)
+
+    def test_builtin_breakpoint_hooked(self, fake_stderr):
+        # Verify that the proper hook is set.
+        with patch.dict(os.environ, {'JUJU_DEBUG_AT': 'all'}):
+            with patch('pdb.Pdb.set_trace') as mock:
+                # Calling through sys, not breakpoint() directly, so we can run the
+                # tests with Py < 3.7.
+                sys.breakpointhook()
+        self.assertEqual(mock.call_count, 1)
+
+    def test_breakpoint_names(self, fake_stderr):
+        # Name rules:
+        # - must start and end with lowercase alphanumeric characters
+        # - only contain lowercase alphanumeric characters, or the hyphen "-"
+        good_names = [
+            'foobar',
+            'foo-bar-baz',
+            'foo-------bar',
+            'foo123',
+            '778',
+            '77-xx',
+            'a-b',
+            'ab',
+            'x',
+        ]
+        for name in good_names:
+            with self.subTest(name=name):
+                self.framework.breakpoint(name)
+
+        bad_names = [
+            '',
+            '.',
+            '-',
+            '...foo',
+            'foo.bar',
+            'bar--'
+            'FOO',
+            'FooBar',
+            'foo bar',
+            'foo_bar',
+            '/foobar',
+            'break-here-☚',
+        ]
+        msg = 'breakpoint names must look like "foo" or "foo-bar"'
+        for name in bad_names:
+            with self.subTest(name=name):
+                with self.assertRaises(ValueError) as cm:
+                    self.framework.breakpoint(name)
+                self.assertEqual(str(cm.exception), msg)
+
+        reserved_names = [
+            'all',
+            'hook',
+        ]
+        msg = 'breakpoint names "all" and "hook" are reserved'
+        for name in reserved_names:
+            with self.subTest(name=name):
+                with self.assertRaises(ValueError) as cm:
+                    self.framework.breakpoint(name)
+                self.assertEqual(str(cm.exception), msg)
+
+        not_really_names = [
+            123,
+            1.1,
+            False,
+        ]
+        for name in not_really_names:
+            with self.subTest(name=name):
+                with self.assertRaises(TypeError) as cm:
+                    self.framework.breakpoint(name)
+                self.assertEqual(str(cm.exception), 'breakpoint names must be strings')
+
+    def check_trace_set(self, envvar_value, breakpoint_name, call_count):
+        """Helper to check the diverse combinations of situations."""
+        with patch.dict(os.environ, {'JUJU_DEBUG_AT': envvar_value}):
+            with patch('pdb.Pdb.set_trace') as mock:
+                self.framework.breakpoint(breakpoint_name)
+        self.assertEqual(mock.call_count, call_count)
+
+    def test_unnamed_indicated_all(self, fake_stderr):
+        # If 'all' is indicated, unnamed breakpoints will always activate.
+        self.check_trace_set('all', None, 1)
+
+    def test_unnamed_indicated_hook(self, fake_stderr):
+        # Special value 'hook' was indicated, nothing to do with any call.
+        self.check_trace_set('hook', None, 0)
+
+    def test_named_indicated_specifically(self, fake_stderr):
+        # Some breakpoint was indicated, and the framework call used exactly that name.
+        self.check_trace_set('mybreak', 'mybreak', 1)
+
+    def test_named_indicated_somethingelse(self, fake_stderr):
+        # Some breakpoint was indicated, but the framework call was not with that name.
+        self.check_trace_set('some-breakpoint', None, 0)
+
+    def test_named_indicated_ingroup(self, fake_stderr):
+        # A multiple breakpoint was indicated, and the framework call used a name among those.
+        self.check_trace_set('some,mybreak,foobar', 'mybreak', 1)
+
+    def test_named_indicated_all(self, fake_stderr):
+        # The framework indicated 'all', which includes any named breakpoint set.
+        self.check_trace_set('all', 'mybreak', 1)
+
+    def test_named_indicated_hook(self, fake_stderr):
+        # The framework indicated the special value 'hook', nothing to do with any named call.
+        self.check_trace_set('hook', 'mybreak', 0)