search

canonical / operator

Pure Python framework for writing Juju charms
Apache License 2.0
247 stars 119 forks source link

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

Closed tonyandrewmeyer closed 1 month ago

tonyandrewmeyer commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

dimaqq commented 2 months ago

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 commented 2 months ago

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.

dimaqq commented 2 months ago

A point for discussion about the overall approach:

tonyandrewmeyer commented 2 months ago
  • 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 commented 1 month ago

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.

tonyandrewmeyer commented 1 month ago

@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).