Here’s a summary of how I approached coding the “Less frequent reloads and restarts” feature for prometheus (pull/352), using BDD.
Step 1: increase test coverage specifically around the feature
Before starting coding the feature I re-read the charm code and write test stubs that are related to the feature.
In this particular case I think the feature could have its own dedicated test class.
class TestPebbleLayer(unittest.TestCase):
"""Test the pebble layer is kept up-to-date (situational awareness)."""
def setUp(self):
pass
def test_workload_restarts_when_some_config_options_change(self):
"""Some config options go in as cli args and require workload restart."""
self.fail("TODO")
def test_workload_hot_reloads_when_some_config_options_change(self):
"""Some config options go into the config file and require a reload (not restart)."""
self.fail("TODO")
def test_no_restart_nor_reload_when_nothing_changes(self):
"""When nothing changes, calling `_configure()` shouldn't result in downtime."""
self.fail("TODO")
Confirm the tests fail:
tox -e unit
# ...
FAILED tests/unit/test_charm.py::TestPebbleLayer::test_no_restart_nor_reload_when_nothing_changes
FAILED tests/unit/test_charm.py::TestPebbleLayer::test_workload_hot_reloads_when_some_config_options_change
FAILED tests/unit/test_charm.py::TestPebbleLayer::test_workload_restarts_when_some_config_options_change
Step 2: document feature details
With the “loose-BDD” style, the feature is expressed as inline Gherkin comments.
class TestPebblePlan(unittest.TestCase):
"""Test the pebble plan is kept up-to-date (situational awareness)."""
def setUp(self):
pass
def test_no_restart_nor_reload_when_nothing_changes(self):
"""When nothing changes, calling `_configure()` shouldn't result in downtime."""
# GIVEN a pebble plan
# WHEN manually calling _configure again
# THEN pebble service is unchanged
# AND workload (re)start is NOT attempted
# AND reload is not invoked
# WHEN update-status is emitted
# THEN pebble service is unchanged
# AND workload (re)start is NOT attempted
# AND reload is not invoked
self.fail("TODO")
def test_workload_restarts_when_some_config_options_change(self):
"""Some config options go in as cli args and require workload restart."""
# WHEN web_external_url is set
# THEN pebble service is updated
# AND workload is restarted
# BUT reload is not invoked
# WHEN web_external_url is changed
# THEN pebble service is updated
# AND workload is restarted
# BUT reload is not invoked
# WHEN web_external_url is unset
# THEN pebble service is updated
# AND workload is restarted
# BUT reload is not invoked
self.fail("TODO")
def test_workload_hot_reloads_when_some_config_options_change(self):
"""Some config options go into the config file and require a reload (not restart)."""
# WHEN evaluation_interval is changed
# THEN a reload is invoked
# BUT pebble service is unchanged
# AND workload (re)start is NOT attempted
self.fail("TODO")
At this point it might be a good idea to initiate a preliminary review to make sure the feature was really understood and captured correctly.
Step 3: write the simplest test first
def raise_if_called(*_, **__):
raise RuntimeError("This should not have been called")
@k8s_resource_multipatch
@patch("lightkube.core.client.GenericSyncClient")
@patch.multiple(
"ops.testing._TestingPebbleClient",
autostart_services=raise_if_called,
replan_services=raise_if_called,
start_services=raise_if_called,
stop_services=raise_if_called,
restart_services=raise_if_called,
)
@patch("prometheus_server.Prometheus.reload_configuration")
def test_no_restart_nor_reload_when_nothing_changes(self, reload_config_patch, *_):
"""When nothing changes, calling `_configure()` shouldn't result in downtime."""
# GIVEN a pebble plan
initial_plan = self.harness.get_container_pebble_plan("prometheus")
self.assertTrue(self.container.get_service("prometheus").is_running())
# WHEN manually calling _configure again
self.harness.charm._configure(None)
# THEN pebble service is unchanged
current_plan = self.harness.get_container_pebble_plan("prometheus")
self.assertEqual(initial_plan.to_dict(), current_plan.to_dict())
self.assertTrue(self.container.get_service("prometheus").is_running())
# AND workload (re)start is NOT attempted
# (Patched pebble client would raise if (re)start was attempted. Nothing else to do here.)
# AND reload is not invoked
reload_config_patch.assert_not_called()
# WHEN update-status is emitted
self.harness.charm.on.update_status.emit()
# THEN pebble service is unchanged
current_plan = self.harness.get_container_pebble_plan("prometheus")
self.assertEqual(initial_plan.to_dict(), current_plan.to_dict())
self.assertTrue(self.container.get_service("prometheus").is_running())
# AND workload (re)start is NOT attempted
# (Patched pebble client would raise if (re)start was attempted. Nothing else to do here.)
# AND reload is not invoked
reload_config_patch.assert_not_called()
Since the feature is not yet implemented, the test fails as expected:
$ tox -e unit -- -k test_no_restart_nor_reload_when_nothing_changes
# ...
AssertionError: Expected 'reload_configuration' to not have been called. Called 1 times.
Step 4: depends
On one hand, the next natural step is to try to implement the feature so the test passes, then implement the next test etc. On the other hand, it could be more effective to first implement all the tests and only then to implement the feature. In this particular case I prefer the latter.