ci: enable doctests #1991
ci: enable doctests #1991
Conversation
| Example usage: | ||
| # this is how we test that attempting to write a remote app's |
There was a problem hiding this comment.
This is private, so only someone developing Harness would see the change.
| is completed. | ||
| Usage: | ||
There was a problem hiding this comment.
This is private, so only someone developing Ops would see it.
| --branch -m pytest --ignore={[vars]tst_path}smoke \ | ||
| --branch -m pytest \ | ||
| --ignore={[vars]tst_path}smoke \ |
There was a problem hiding this comment.
I originally had the doctests running for coverage and then changed my mind on that. However, I think it is nicer to keep the ignores on separate lines.
| >>> from lib.charms.tempo_coordinator_k8s.v0.charm_tracing import trace_charm | ||
| >>> from lib.charms.tempo_coordinator_k8s.v0.tracing import charm_tracing_config | ||
| >>> @trace_charm(tracing_endpoint="my_endpoint", cert_path="cert_path") | ||
| >>> class MyCharm(...): | ||
| >>> _cert_path = "/path/to/cert/on/charm/container.crt" | ||
| >>> def __init__(self, ...): | ||
| >>> self.tracing = TracingEndpointRequirer(...) | ||
| >>> self.my_endpoint, self.cert_path = charm_tracing_config( | ||
| ... self.tracing, self._cert_path) | ||
| from lib.charms.tempo_coordinator_k8s.v0.charm_tracing import trace_charm | ||
| from lib.charms.tempo_coordinator_k8s.v0.tracing import charm_tracing_config | ||
| @trace_charm(tracing_endpoint="my_endpoint", cert_path="cert_path") | ||
| class MyCharm(...): | ||
| _cert_path = "/path/to/cert/on/charm/container.crt" | ||
| def __init__(self, ...): | ||
| self.tracing = TracingEndpointRequirer(...) | ||
| self.my_endpoint, self.cert_path = charm_tracing_config( | ||
| self.tracing, self._cert_path | ||
| ) |
There was a problem hiding this comment.
Technically, I should provide a clean for this upstream, but we're considering reimplementing this library rather than vendoring it, and there's the "move all the interface libraries" project as well, so it doesn't seem worth doing.
There was a problem hiding this comment.
My 2c:
- let's not run doctests on vendored code
- testing charm libs is outside of ops scope (albeit great for community)
- doc-testing charm libs is even more so (albeit helps to keep charm lib doc strings in sync with charm lib code: chore: fix charm lib doc string haproxy-operator#163)
There was a problem hiding this comment.
The challenge here is that pytest's doctest integration doesn't provide much in the way of configuration (pytest-doctestplus is better, but still doesn't have exactly what we want). There's no "ignore" option so to do this we'd have to have a lot of glob statements that match the files to be tested and don't match the files to not be tested.
benhoyt
left a comment
benhoyt
left a comment
There was a problem hiding this comment.
Looks good to me, thanks. Can you show what a failing doctest looks like?
In the summary: And in the details: |
Co-authored-by: Dave Wilding <tech@dpw.me>
Co-authored-by: Dave Wilding <tech@dpw.me>
5 participants
In #1975, we'd like to include a couple of docstrings. This PR clears the way for that, as well as any other doctests we wish to use in the future.
The PR tweaks some docstrings that are in private methods, so not in in our reference documentation. In addition:
ActionEvent.set_resultschanges from a block of>>>text to a set of bullet points. If this was doctest'ed then it'd actually call the method, but doing that withset_resultsitself would be complicated. This seems just as clear to me.pebble.Client.exechas a couple of lines to disable doctest. This could be a doctest, but that would require a real Pebble, which we currently split off into a different set of tests. This is a bit ugly, but puttingdoctest: +SKIPon most of the lines is uglier (in my opinion), and I don't think there's a cleaner way to skip a single docstring.