docs: improve unit tests of httpbin demo charm#2254
docs: improve unit tests of httpbin demo charm#2254dwilding merged 21 commits intocanonical:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the unit tests for the httpbin demo charm by making them more comprehensive and maintainable. The changes include refactoring tests to use pytest's parametrization feature, adding new test cases for edge conditions, and reorganizing the code for better clarity.
- Introduced pytest parametrization to reduce test duplication and improve coverage of log level validation
- Added new test cases for service status conditions (inactive service, missing service, unavailable container)
- Refactored charm code to use else block for clearer control flow in service status checking
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| examples/httpbin-demo/tests/unit/test_charm.py | Refactored existing tests to use parametrization, added new edge case tests for service status, and improved test naming and documentation |
| examples/httpbin-demo/src/charm.py | Refactored service status checking logic to use else block instead of early return pattern |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -82,6 +80,10 @@ def _on_collect_status(self, event: ops.CollectStatusEvent): | |||
| # It's technically possible (but unlikely) for Pebble to have an internal error. | |||
| logger.error("Unable to fetch service info from Pebble") | |||
| raise | |||
| else: | |||
| if not service.is_running(): | |||
| # We can connect to Pebble in the container, but the service hasn't started yet. | |||
| event.add_status(ops.MaintenanceStatus("waiting for workload")) | |||
There was a problem hiding this comment.
I refactored this part of the charm code after James's comment on similar code elsewhere: #2062 (comment)
| # Assert: | ||
| assert isinstance(state_out.unit_status, testing.BlockedStatus) | ||
| assert invalid_level in state_out.unit_status.message | ||
| assert f"'{user_log_level}'" in state_out.unit_status.message |
There was a problem hiding this comment.
Minor: this assertion doesn't do all that much when the test is parametrised with an empty string. A pair of quotes could come from anywhere.
P.S. this could also be written as:
| assert f"'{user_log_level}'" in state_out.unit_status.message | |
| assert f"{user_log_level!r}" in state_out.unit_status.message |
though I'm not sure which is easier to understand.
There was a problem hiding this comment.
Good point, thanks. How about this - I've changed the charm code to give a different error message when the config value is empty. Then we can split into two (unparameterized) tests:
def test_config_changed_empty():
"""Test a config-changed event when the config is invalid."""
ctx = testing.Context(HttpbinDemoCharm)
container = testing.Container(CONTAINER_NAME, can_connect=True)
state_in = testing.State(containers={container}, config={"log-level": ""})
state_out = ctx.run(ctx.on.config_changed(), state_in)
assert isinstance(state_out.unit_status, testing.BlockedStatus)
assert state_out.unit_status.message.startswith("Empty log level.")
def test_config_changed_invalid():
"""Test a config-changed event when the config is invalid."""
ctx = testing.Context(HttpbinDemoCharm)
container = testing.Container(CONTAINER_NAME, can_connect=True)
state_in = testing.State(containers={container}, config={"log-level": "foobar"})
state_out = ctx.run(ctx.on.config_changed(), state_in)
assert isinstance(state_out.unit_status, testing.BlockedStatus)
assert state_out.unit_status.message.startswith("Invalid log level: 'foobar'.")
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Approving because these are nice improvements that would be fine to merge as-is.
I've commented on some small things I'd change, but feel free to disagree.
examples/httpbin-demo/src/charm.py
Outdated
| if not log_level: | ||
| raise ValueError(f"Empty log level. Valid values are: {', '.join(valid_log_levels)}.") |
There was a problem hiding this comment.
This is nice but seems unnecessary since we'd get a ValueError with a reasonable helpful message anyway: Invalid log level: ''.
Better to keep the code simpler with one less special case? But see also my comment on your thread with Dima.
There was a problem hiding this comment.
Which comment on the thread with Dima? (Sorry if I'm totally missing it!)
There was a problem hiding this comment.
Ugh, I think I didn't submit it correctly and it's lost now.
My thoughts were that the new message.startswith("Invalid log level: '{user_log_level}'.") form should take care of the concern about an empty string coming from anywhere, so a single test could be fine.
I don't really mind the separate tests, but I'm not a big fan of the (imo unnecessary) separate error case.
There was a problem hiding this comment.
I get you, thanks. I agree that message.startswith alone is a better change than splitting into two code paths. I've reverted to a single test_config_changed_invalid test with a parameterised value.
3 participants
This PR makes several improvements to the unit tests of our httpbin demo charm:
log-level.I also made a couple of small changes elsewhere:
ops.pebbleinstead of importingpebblefromopslog-levelis empty