feat: enable interactive debugging via breakpoint in testing

[james-garner-canonical]

This PR enables interactive debugging via breakpoint calls in charm code in state-transition tests, by preventing Ops from redefining sys.breakpointhook when charm code is executed with testing.Context.run.

Resolves #2166.

Issue

In main, Ops unconditionally overrides sys.breakpointhook with the ops.Framework.breakpoint method. This happens when ops._main._Manager._make_framework calls ops.Framework.set_breakpointhook.

Framework.breakpoint does nothing unless JUJU_DEBUG_AT is set. This means you can deploy a charm that calls breakpoint (or self.framework.breakpoint), and those breakpoints will be a no-op until you use juju debug-code.

A consequence of this is that breakpoint calls in charm code that are being driven by ops.testing.Context.run are no-ops, as reported in #2166.

Fix

This PR updates the CapturingFramework of ops.testing to override the set_breakpointhook method with an implementation that does nothing. This means that in tests using Context.run, breakpoints in charm code will work the same as breakpoints in the test code itself -- dropping the user into an interactive debugger session (or not, if PYTHONBREAKPOINT=0; or raising an error if running tests across multiple processes with pytest-xdist).

Implications

Searching for breakpoint in our known charms shows that it is common to set PYTHONBREAKPOINT to pdb.set_trace or ipdb.set_trace, but that breakpoint calls in Python code are not checked in.

To set breakpoints in charm code that are not triggered during state-transition tests, but will be active when running juju debug-code, users can either set PYTHONBREAKPOINT=0 when running state-transition tests, or use self.framework.breakpoint instead.

Documentation

I will be improving the docs for debugging a charm in a separate PR. Unless there's a great place to fit this in right now, I think we can defer documenting this change to that PR.

[tonyandrewmeyer]

This solution works for me.

I wonder if it would be cleaner if framework.set_breakpointhook had the logic to check the environment variable, but that is more of an invasive change, and maybe the hook breakpoint wouldn't work with that? I haven't looked at the Ops code in detail.

[james-garner-canonical]

I wonder if it would be cleaner if framework.set_breakpointhook had the logic to check the environment variable, but that is more of an invasive change, and maybe the hook breakpoint wouldn't work with that? I haven't looked at the Ops code in detail.

I'm not sure I understand how that approach would work.

Do you mean if ops.Framework.set_breakpointhook only changes sys.breakpointhook when JUJU_DEBUG_AT is set? I think that would be a concerning change because we'd go from breakpoint being a no-op in charm code unless JUJU_DEBUG_AT was set, to triggering the default breakpoint handler if it's not set. I could see that working if Juju set PYTHONBREAKPOINT according to whether it was running with juju debug-code or not, but the only hits in the juju repo are in test charms.

[tonyandrewmeyer]

Do you mean if ops.Framework.set_breakpointhook only changes sys.breakpointhook when JUJU_DEBUG_AT is set?

Yes. But like I said I hadn't looked into it to see if it would work or not, it just seemed like maybe the framework approach wasn't the right one given how this is used. But your explanation makes sense, and I'm still good with doing this specifically in Scenario.

[dimaqq]

nice!!