-
Notifications
You must be signed in to change notification settings - Fork 121
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
refactor: move the content of ops.testing to ops._private.harness #1369
Conversation
PRs to stop charms using the private class: |
Unfortunately, these are not the only culprits, there's also:
My feeling is that by accessing a private name and not fixing that after the issue in ops was fixed, the charmers should take responsibility for fixing this themselves (it's fairly straightforward). However, if people want to argue that we should offer up PRs for the above as well, I can do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think you could rebase/redo this PR into two commits: one with a pure rename of ops/testing.py
to ops/_private/harness.py
, and one with any actual changes? Otherwise it's very difficult to see the actual changes.
116e30d
to
7e098fe
Compare
7e098fe
to
362f7eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks straight-forward enough -- just a couple of questions for now.
) | ||
|
||
# The Harness testing framework. | ||
_ = ActionFailed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference/advantage of doing this over __all__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__all__
causes Sphinx to get very confused, and I couldn't figure out any way to keep __all__
and de-confuse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'm happy with the changes themselves, but I'm concerned about what this means for our API reference docs. It seems like PR means there are now no docs for ops.testing
-- is that intentional? https://ops--1369.org.readthedocs.build/en/1369/#module-ops.testing
No, when I checked what the docs were like I loaded the wrong file, so I missed this. I'll figure out how to fix it. |
… ops.testing, not ops._private.harness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
pls rebase too |
…nonical#1369) Moves the content of `ops/testing.py` to `ops/_private/harness.py`, and then re-exposes all the names in the `ops.testing` namespace (other than standard library and third-party modules). This allows other testing frameworks (such as Scenario) to be exposed in the `ops.testing` namespace while also building on top of the existing testing code.
Remove mocking of private ops class according to canonical/operator#1369 and canonical/seldon-core-operator#268
Remove mocking of private ops class according to canonical/operator#1369 and canonical/seldon-core-operator#268
Moves the content of
ops/testing.py
toops/_private/harness.py
, and then re-exposes all the names in theops.testing
namespace (other than standard library and third-party modules).This allows other testing frameworks (such as Scenario) to be exposed in the
ops.testing
namespace while also building on top of the existing testing code.