Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Sep 18, 2024

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 tonyandrewmeyer marked this pull request as ready for review September 20, 2024 00:17
@tonyandrewmeyer
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@dimaqq
Copy link
Contributor

dimaqq commented Sep 20, 2024

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
Copy link
Contributor Author

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 Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
@dimaqq
Copy link
Contributor

dimaqq commented Sep 20, 2024

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
Copy link
Contributor Author

  • 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
Copy link
Contributor Author

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 Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
ops/testing.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some further comments on the docs.

docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated
out = ctx.run(ctx.on.start(), State())
assert out.unit_status == UnknownStatus()

.. autoclass:: ops.testing.ActionFailed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

docs/index.rst Outdated Show resolved Hide resolved
docs/state-transition-testing.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
docs/state-transition-testing.rst Outdated Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
docs/harness.rst Outdated Show resolved Hide resolved
ops/_private/harness.py Outdated Show resolved Hide resolved
ops/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Dima Tisnek <dimaqq@gmail.com>
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@tonyandrewmeyer tonyandrewmeyer merged commit 4a14764 into canonical:main Sep 25, 2024
29 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the pip-install-ops-testing branch September 25, 2024 08:08
tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this pull request Oct 4, 2024
…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 <benhoyt@gmail.com>
Co-authored-by: Dima Tisnek <dimaqq@gmail.com>
tonyandrewmeyer added a commit to tonyandrewmeyer/operator that referenced this pull request Oct 4, 2024
…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 <benhoyt@gmail.com>
Co-authored-by: Dima Tisnek <dimaqq@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants