-
Notifications
You must be signed in to change notification settings - Fork 125
docs: add a how-to guide for migrating away from Harness #2062
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
Conversation
| event.add_status(ops.MaintenanceStatus("waiting for container")) | ||
| event.add_status(ops.ActiveStatus()) | ||
|
|
||
| ... # Handle other events. |
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.
I was originally going to include an action that executes a command in the workload container. I removed that because I wasn't confident in the approach, and this PR was getting big enough already. I might add that back in a follow-up PR
dimaqq
left a comment
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.
Only a partial review.
I feel that as the examples get longer and longer, at some point the doc becomes tedious.
Could the examples be trimmed to testing one thing at a time, with the mocking step, but without the actual mocks?
A tutorial would be self-contained, while the how-to guide could be more focused.
WDYT?
Rule of thumb: the doc renders 8 pages long for me (reasonable font, large monitor). At this point it could be split into 4~6 separate docs.
| def _on_get_value_action(self, event: ops.ActionEvent) -> None: | ||
| """Handle the get-value action.""" | ||
| if event.params["value"] == "please fail": | ||
| event.fail("Action failed, as requested") | ||
| else: | ||
| event.set_results({"out-value": event.params["value"]}) |
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.
I'd drop this entirely, or if it's really needed, then:
| def _on_get_value_action(self, event: ops.ActionEvent) -> None: | |
| """Handle the get-value action.""" | |
| if event.params["value"] == "please fail": | |
| event.fail("Action failed, as requested") | |
| else: | |
| event.set_results({"out-value": event.params["value"]}) | |
| def _on_get_value_action(self, event: ops.ActionEvent) -> None: | |
| # See above |
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.
Yeah, this makes sense, thanks. I've replaced the whole method by:
... # _on_get_value_action is unchanged.
tonyandrewmeyer
left a comment
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.
A few small suggestions, but overall I think this works well. I think this is as long as we would want to start with - if we get a lot of common "how to convert X" questions in the future then we could add more, but I don't think it's worth trying to be exhaustive.
It's a shame that we can't do a see-more to the PE guide, but it seems like that's internal only. There's some overlap, but also some additional content (although the custom events bit is a little out of date now). Their approach of using tabs to have the harness/scenario code uses less space, but I think your approach here tells the story more clearly without having to flick back and forth.
It does feel a little heavy on collect-status, but as that's a significant difference, perhaps that's ok. I'm happy with it as-is.
| For more information about state-transition testing, see: | ||
|
|
||
| - [](#write-unit-tests-for-a-charm) | ||
| - [The reference docs](/reference/ops-testing), especially [](ops.testing.State) and [](ops.testing.CharmEvents) |
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.
Interesting to choose CharmEvents here. I would have thought that Context and State were the two you'd most what to see. I guess CharmEvents has all of the events and they aren't 100% the same as in ops, although very similar.
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.
I've reworked this bit, partly after your feedback. To make the links easier to find (and to make the intro shorter) I've added a dedicated See more section at the end of the doc. In that, I've linked to Context, State, and CharmEvents.
james-garner-canonical
left a comment
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.
This is looking great to me, modulo the existing suggestions.
|
Thanks all for the feedback! I've addressed everything, but won't merge until next week (when Tony is back). While applying the feedback, I made a couple of changes to reduce the cognitive load of things that aren't relevant to the core point. Thanks Dima for your comments around this.
|
tonyandrewmeyer
left a comment
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.
Re-approving, in case you were after that (the recent changes all look good to me).
This PR adds a how-to guide for migrating unit tests away from Harness. I've not linked the new guide from any other docs yet, or removed How to write legacy unit tests for a charm. I'll leave those tasks for a follow-up PR.
Preview doc