docs: separate test_workload_version_is_set in k8s tutorial chapter 3#2638
Conversation
…leaning up; mention why we need charm fixture in all integration tests; simplify integration test section of chapter 3
| ```diff | ||
| - juju.wait(jubilant.all_active) | ||
| + juju.wait(jubilant.all_blocked) | ||
| ``` |
There was a problem hiding this comment.
I hadn't thought of using a diff - we rarely use them (I see only one other instance in the Ops docs). Definitely not against it! I'd like to see @tonyandrewmeyer's opinion on whether the intent is clear enough.
There was a problem hiding this comment.
I'm not keen on the diff, because it seems harder to both maintain and to use, although perhaps easier to read. For maintenance, we want to load this in from the actual files, and I don't think that can be done as a diff, so we would have to manually re-create the diff every time the code changes. For use, I think it's much simpler to hit the copy button and then paste than to apply a patch.
However, if this is the only line that's changing, then I think we are showing too much of the file. Just the test would be enough.
There was a problem hiding this comment.
This is a convincing reason not to do a diff. I'm good if we show the new version of the test_deploy function, which can retain the comment about why we're waiting for blocked status.
There was a problem hiding this comment.
Thanks for the feedback. I added this function back in 8b27c59.
It's a good point on copy-and-paste experience for readers.
…he new integration test
|
Another observation I have, we instructed the readers to run integration tests in 2 ways: here, and In the |
Yeah, I think it's fine to remove this - good thinking. Earlier in the tutorial we established that commands are run in the VM, even if the reader is editing files using their regular editor. |
| - `juju` - A Jubilant object for interacting with a temporary Juju model. This fixture is provided by the `pytest-jubilant` plugin. | ||
|
|
||
| ### Run the test | ||
| Both tests depend on the `charm` fixture even though only `test_deploy` uses the fixture. This ensures that the tests fail immediately if a `.charm` file isn't available. |
There was a problem hiding this comment.
I guess this is because we don't have abort_on_fail yet. I'm not a huge fan of this approach, but perhaps it's ok for a tutorial, where you don't want to have to build something more complex.
It also seems a bit strange if you skip setup, where you presumably have the deployed app already, and don't actually need the charm, although probably it is findable and the cost is low, since it needs to already have been packed.
Is this a pattern we are creating or something we see in real charms? I see we already do this with the database test, is this just documenting something we already decided to do?
There was a problem hiding this comment.
It's the approach we previously standardised on in our docs and example charms.
| ```diff | ||
| - juju.wait(jubilant.all_active) | ||
| + juju.wait(jubilant.all_blocked) | ||
| ``` |
There was a problem hiding this comment.
I'm not keen on the diff, because it seems harder to both maintain and to use, although perhaps easier to read. For maintenance, we want to load this in from the actual files, and I don't think that can be done as a diff, so we would have to manually re-create the diff every time the code changes. For use, I think it's much simpler to hit the copy button and then paste than to apply a patch.
However, if this is the only line that's changing, then I think we are showing too much of the file. Just the test would be enough.
…er-3-k8s-tutorial
Fixes #2646 This PR fixes the `test_workload_version_set` integration tests in the Example Charms in `example/k8-*` ## The bug From Chapter 3 onward, `test_deploy` waits for the charm to block, which happens immediately on the first config-changed event, as there is no database yet. This is before Pebble is ready. Then the tests move on to `test_workload_version_is_set`, which fails because Pebble is still not ready. You can see it in the CI run https://github.com/canonical/operator/actions/runs/29348881449/job/87139670139#step:9:79 ``` tests/integration/test_charm.py::test_deploy ... INFO jubilant.wait:_juju.py:1491 wait: status changed: - .apps['fastapi-demo'].app_status.current = 'waiting' - .apps['fastapi-demo'].app_status.message = 'installing agent' - .apps['fastapi-demo'].units['fastapi-demo/0'].workload_status.current = 'waiting' - .apps['fastapi-demo'].units['fastapi-demo/0'].workload_status.message = 'installing agent' - .apps['fastapi-demo'].units['fastapi-demo/0'].juju_status.current = 'allocating' + .apps['fastapi-demo'].app_status.current = 'blocked' + .apps['fastapi-demo'].app_status.message = 'Waiting for database relation' + .apps['fastapi-demo'].units['fastapi-demo/0'].workload_status.current = 'blocked' + .apps['fastapi-demo'].units['fastapi-demo/0'].workload_status.message = 'Waiting for database relation' ... PASSED tests/integration/test_charm.py::test_workload_version_is_set -------------------------------- live log call --------------------------------- INFO jubilant:_juju.py:398 cli: juju status --model jubilant-0a5d413e-test-charm --format json FAILED tests/integration/test_charm.py::test_database_integration ``` ## Why this happened. In #2638, I changed the order of `test_workload_version_is_set` to before `test_database_integration`. Previously, it ran after `test_database_integration`, so everything was okay. ## The fix We wanted to make `test_workload_version_is_set` independent from `test_database_integration`. This is okay because the workload version depends on Pebble readiness, and not the database integration. Therefore, I use `juju.wait` until the workload version is available from juju status. This fits nicely into how we use Jubilant in charm integration tests. ## I validated this on my fork `test_workload_version_is_set` passed in CI run for Example Charm Integration Tests: https://github.com/tromai/operator/actions/runs/29554228387 I also validated that the CI still pass whether `test_workload_version_is_set` is before or after `test_database_integration`.
This PR brought
test_workload_version_is_setback to life in chapter 3, and propagate that change to chapter 4 and 5. It also updated the exampleexamples/k8s-*/tests/integration/test_charm.pyfiles.Doc preview
It was raised by David: