feat: optionally install Scenario with `ops[testing]` and expose the names in ops.testing #1381

tonyandrewmeyer · 2024-09-18T10:27:10Z

Add a new optional install testing, e.g. pip install ops[testing]. This pulls in a compatible version of ops-scenario, and exposes the Scenario names in the ops.testing namespace, alongside Harness.

pip install ops[harness] is also supported to ease the (far off, presumably 3.0) transition to Harness being moved out of the base install. It currently installs no extra dependencies, so is the same as pip install ops but a forward-looking charm would use pip install ops[harness] (or pip install ops[harness, testing]) if using Harness.

Requires ops-scenario 7.0.5, which has the required adjustments to support insertion into ops.testing.

The ActionFailed name exists in both ops._private.harness and scenario.context. This is handled by adjusting the Harness class to support the functionality of both and monkeypatching that into Scenario until Scenario starts using it. It's compatible with both Harness and Scenario, but will have empty data in an attribute (which attribute depends on which framework is used).

The Container name in ops.testing, which is only present for backwards compatibility, is also overwritten if ops-scenario is installed. If anyone is actually using ops.testing.Container instead of ops.Container then they'll need to fix their code before using ops[testing] (or ops-scenario combined with the release of ops with this change).

A very basic unit test is added to make sure that Scenario tests work (all the actual Scenario tests are in the ops-scenario repo) if ops-scenario/ops[testing] is installed (this is the case for tox -e unit). A test is also added to ensure that all of the Scenario names are documented, since automodule isn't used any more.

Also adjusts the documentation to include the new framework in ops.testing.

tonyandrewmeyer · 2024-09-20T00:19:04Z

@PietroPasotti @benhoyt @dimaqq I imagine this PR will have a bit of back-and-forth before it's completely done. Would you please give it an initial look-over? Probably focusing on the main approaches rather than specifics for now, and then once everyone is happy with that there can be a second review for approval that goes deeper. Thanks!

tonyandrewmeyer · 2024-09-20T00:20:56Z

(I'll look into the failing test with Python 3.10 - I hadn't checked different Pythons locally, so will do that).

dimaqq · 2024-09-20T01:53:55Z

CI fail: TypeError: EventBase.__init__() got an unexpected keyword argument 'app'

3.12 was not run / did not get to complete because 3.10 failed.

tonyandrewmeyer · 2024-09-20T02:34:54Z

CI fail: TypeError: EventBase.__init__() got an unexpected keyword argument 'app'

3.12 was not run / did not get to complete because 3.10 failed.

Yeah, the tests all run locally for me with 3.8, 3.9, 3.10, 3.11, 3.12. Still looking into this, but it seems unlikely that it needs to block a high-level review.

docs/requirements.txt

ops/testing.py

dimaqq · 2024-09-20T05:39:14Z

A point for discussion about the overall approach:

do we want to keep separate pypi:ops-scenario going forward?
or do shall we vendor scenario code and rename things?

tonyandrewmeyer · 2024-09-20T06:01:38Z

do we want to keep separate pypi:ops-scenario going forward?

or do shall we vendor scenario code and rename things?

We have to have a separate package in order to (cleanly) have one set of code that gets used when building charms and an augmented set of code that gets used when testing charms. I very strongly feel that the test framework should not be in the bundled charm. I think the correct way to do that is to have a package for testing (eventually two: one for Harness and one for Scenario) rather than things like charmcraft ripping out the folder when packing.

We are intending to add the contents of the ops-scenario repository to the operator repository, but my preference is to do that as a separate PR after this one. We would then publish two packages from the operator repository. I still feel that's going to be a better maintenance burden than having two repos. I don't really see a benefit in renaming things as part of this, but that can be discussed when we get to it.

We could publish Scenario under a new name, like ops-testing. I don't see much value in that either, though, and I like having a small nod to the history of how this framework came to be.

tonyandrewmeyer · 2024-09-22T22:31:38Z

I can reproduce the failing test locally if I turn pytest-xdist off (probably other small numbers work too). I assume some other test is leaking into the one that's failing, so it only fails when both of those tests are executed in the same runner.

ops/charm.py

ops/testing.py

docs/index.rst

benhoyt

Adding some further comments on the docs.

docs/index.rst

benhoyt · 2024-09-23T02:38:53Z

docs/index.rst

+        out = ctx.run(ctx.on.start(), State())
+        assert out.unit_status == UnknownStatus()
+
+.. autoclass:: ops.testing.ActionFailed


It's a pity we have to list these out manually? Is there any way to avoid that, so that we don't forget to add new classes here when we add them to the code?

I do agree. I couldn't get automodule to work and also use the ops.testing name rather than leaking ops._private.harness or scenario. It's somewhat working against the automodule intention since the usual desired behaviour is to document anything defined in the module but not anything that the module pulls in to use - but here we have basically nothing defined in the module, and want to document (almost) everything that's pulled in.

There could be some combination of Sphinx configuration options that make it work, but I wasn't able to find them with the limited amount of time I allowed for it. It's also possible that we could create our own automodule directive (subclassing or patching the original one, probably) that would solve our issues - but this seemed like it would be a lot more work.

For now, I've added a test_infra test to make it less likely that we forget to maintain this, and a comment about why we do this, and hopefully someone in the future will make this nicer.

tonyandrewmeyer · 2024-09-23T23:44:52Z

@tmihoc the code changes won't be interesting to you, but you might like to review the doc changes? It's probably easiest to look over the generated docs since it pulls in the definitions from ops-scenario as well (which you already reviewed, of course).

…s.testing.

…top level?).

…rk around our trickery. Drop modindex (only has ops and ops.pebble, seems pointless) and search (it's on the left-hand side all the time, no point in a blank page) as drive-bys.

…sses.

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

benhoyt

Overall looking great, just comments on doc that we've discussed.

docs/index.rst

docs/state-transition-testing.rst

docs/index.rst

docs/state-transition-testing.rst

ops/testing.py

docs/harness.rst

ops/_private/harness.py

ops/__init__.py

… for now, to ease transition.

Co-authored-by: Dima Tisnek <[email protected]>

benhoyt

Approving! Left a couple of minor take-or-leave comments.

docs/index.rst

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

…names in ops.testing (canonical#1381) Add a new optional install `testing`, e.g. `pip install ops[testing]`. This pulls in a compatible version of `ops-scenario`, and exposes the Scenario names in the `ops.testing` namespace, alongside Harness. `pip install ops[harness]` is also supported to ease the (far off, presumably 3.0) transition to Harness being moved out of the base install. It currently installs no extra dependencies, so is the same as `pip install ops` but a forward-looking charm would use `pip install ops[harness]` (or `pip install ops[harness, testing]`) if using Harness. Requires ops-scenario 7.0.5, which has the required adjustments to support insertion into `ops.testing`. The `ActionFailed` name exists in both `ops._private.harness` and `scenario.context`. This is handled by adjusting the Harness class to support the functionality of both and monkeypatching that into Scenario until Scenario starts using it. It's compatible with both Harness and Scenario, but will have empty data in an attribute (which attribute depends on which framework is used). The `Container` name in `ops.testing`, which is only present for backwards compatibility, is also overwritten if ops-scenario is installed. If anyone is actually using `ops.testing.Container` instead of `ops.Container` then they'll need to fix their code before using `ops[testing]` (or ops-scenario combined with the release of ops with this change). A very basic unit test is added to make sure that Scenario tests work (all the actual Scenario tests are in the ops-scenario repo) if ops-scenario/ops[testing] is installed (this is the case for `tox -e unit`). A test is also added to ensure that all of the Scenario names are documented, since `automodule` isn't used any more. Also adjusts the documentation to include the new framework in ops.testing. --------- Co-authored-by: Ben Hoyt <[email protected]> Co-authored-by: Dima Tisnek <[email protected]>

tonyandrewmeyer marked this pull request as ready for review September 20, 2024 00:17

tonyandrewmeyer requested review from dimaqq and benhoyt September 20, 2024 00:17

dimaqq reviewed Sep 20, 2024

View reviewed changes

docs/requirements.txt Show resolved Hide resolved

ops/testing.py Show resolved Hide resolved

tonyandrewmeyer mentioned this pull request Sep 22, 2024

test: properly mock the environment #1385

Merged

tonyandrewmeyer requested a review from dimaqq September 22, 2024 23:10

benhoyt reviewed Sep 23, 2024

View reviewed changes

tonyandrewmeyer requested a review from benhoyt September 23, 2024 23:56

tonyandrewmeyer added 13 commits September 24, 2024 12:46

Offer pip install ops[testing] and if available expose Scenario in op…

21c0396

…s.testing.

Use an explicit reference to help Sphinx out.

cd0ecbf

Create a new class rather than inheriting from both the originals.

4eedb72

Add types in the test class.

b61469b

Use explicit references to ops so that copying the docstrings works.

7ee4fe5

CharmEvents should be exposed in ops.testing (maybe also in scenario …

40323f2

…top level?).

Add Scenario and Harness explicitly rather than via automodule, to wo…

55f0910

…rk around our trickery. Drop modindex (only has ops and ops.pebble, seems pointless) and search (it's on the left-hand side all the time, no point in a blank page) as drive-bys.

Adjust the monkeypatching to also include runtime.

e7870cd

Fix markup. Note that Harness is included by default.

41a9854

Merge in the Scenario customisations.

2b60277

Re-pin with ops-scenario added.

c647130

Add ops-scenario to the docs dependencies.

5f21f94

Use __all__ and move ActionFailed to Harness.

d6c7d2d

tonyandrewmeyer and others added 11 commits September 24, 2024 12:47

Add a test to ensure that we don't forget to document ops.testing cla…

8905747

…sses.

Minor tweaks, mostly based on review.

98f0e30

Update docs/index.rst

3decc03

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

Update docs/index.rst

3a6c58f

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

Update docs/index.rst

b92d4e3

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

Tweak, per review.

cbf52b3

WiP docs on separate pages.

67b5a7a

Split the testing classes into separate pages.

1f5801e

Fix merge.

151ec9f

Update test for doc split.

04bac0b

Fix rebase.

08c95f8

tonyandrewmeyer force-pushed the pip-install-ops-testing branch from ec4d39d to 08c95f8 Compare September 24, 2024 00:50

benhoyt reviewed Sep 24, 2024

View reviewed changes

dimaqq approved these changes Sep 24, 2024

View reviewed changes

ops/_private/harness.py Outdated Show resolved Hide resolved

ops/__init__.py Outdated Show resolved Hide resolved

tonyandrewmeyer mentioned this pull request Sep 24, 2024

docs: add top-level intro and module-level intros #1320

Merged

5 tasks

tonyandrewmeyer added 4 commits September 24, 2024 14:19

Allow 'pip install ops[harness], which is the same as pip install ops…

cf61118

… for now, to ease transition.

Typo from previous PR.

8b1d1ee

Add a PendingDeprecationWarning for Harness.

d0ce232

Tweaks per review.

24686d3

tonyandrewmeyer requested a review from benhoyt September 24, 2024 02:20

Update ops/_private/harness.py

bac91d7

Co-authored-by: Dima Tisnek <[email protected]>

benhoyt approved these changes Sep 24, 2024

View reviewed changes

docs/index.rst Outdated Show resolved Hide resolved

docs/index.rst Outdated Show resolved Hide resolved

tonyandrewmeyer and others added 2 commits September 24, 2024 14:45

Update docs/index.rst

2472e7c

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

Move some index content to the harness page.

bc05df8

tonyandrewmeyer merged commit 4a14764 into canonical:main Sep 25, 2024
29 checks passed

tonyandrewmeyer deleted the pip-install-ops-testing branch September 25, 2024 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: optionally install Scenario with `ops[testing]` and expose the names in ops.testing #1381

feat: optionally install Scenario with `ops[testing]` and expose the names in ops.testing #1381

tonyandrewmeyer commented Sep 18, 2024 •

edited

Loading

tonyandrewmeyer commented Sep 20, 2024

tonyandrewmeyer commented Sep 20, 2024

dimaqq commented Sep 20, 2024

tonyandrewmeyer commented Sep 20, 2024

dimaqq commented Sep 20, 2024

tonyandrewmeyer commented Sep 20, 2024

tonyandrewmeyer commented Sep 22, 2024

benhoyt left a comment

benhoyt Sep 23, 2024

tonyandrewmeyer Sep 23, 2024

tonyandrewmeyer commented Sep 23, 2024

benhoyt left a comment

benhoyt left a comment

feat: optionally install Scenario with ops[testing] and expose the names in ops.testing #1381

feat: optionally install Scenario with ops[testing] and expose the names in ops.testing #1381

Conversation

tonyandrewmeyer commented Sep 18, 2024 • edited Loading

tonyandrewmeyer commented Sep 20, 2024

tonyandrewmeyer commented Sep 20, 2024

dimaqq commented Sep 20, 2024

tonyandrewmeyer commented Sep 20, 2024

dimaqq commented Sep 20, 2024

tonyandrewmeyer commented Sep 20, 2024

tonyandrewmeyer commented Sep 22, 2024

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt Sep 23, 2024

Choose a reason for hiding this comment

tonyandrewmeyer Sep 23, 2024

Choose a reason for hiding this comment

tonyandrewmeyer commented Sep 23, 2024

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

feat: optionally install Scenario with `ops[testing]` and expose the names in ops.testing #1381

feat: optionally install Scenario with `ops[testing]` and expose the names in ops.testing #1381

tonyandrewmeyer commented Sep 18, 2024 •

edited

Loading