Skip to content

fix: warn before clearing non-empty container in testing #2365

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.

Sign up for GitHub

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

Open
james-garner-canonical wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: warn before clearing non-empty container in testing #2365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06ba824
fix: warn before clearing non-empty container in testing
james-garner-canonical Mar 5, 2026
05c75fb
test: add a test of the other way around
james-garner-canonical Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions testing/src/scenario/mocking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,14 @@ def __init__(

# wipe just in case
if container_root.exists():
if any(container_root.iterdir()):
logger.warning(
'Container %r has a non-empty filesystem that will be wiped before this run.'
' If you are trying to mock filesystem contents, use Mounts instead.'
' If you are reusing a Context instance across multiple ctx.run() calls,'
' note that the filesystem is cleared between runs.',
container_name,
)
# Path.rmdir will fail if root is nonempty
shutil.rmtree(container_root)

Expand Down
51 changes: 51 additions & 0 deletions testing/tests/test_e2e/test_pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import dataclasses
import datetime
import io
import logging
from pathlib import Path

import pytest
Expand Down Expand Up @@ -880,3 +881,53 @@ def test_layers_merge_in_plan(layer1_name, layer2_name):
assert log_target.labels == {'foo': 'bar'}
assert log_target.override == 'merge'
assert log_target.location == 'https://loki2.example.com'


def test_warning_on_non_empty_container(caplog: pytest.LogCaptureFixture):
class MyCharm(CharmBase):
def __init__(self, framework: Framework):
super().__init__(framework)
self.framework.observe(self.on.start, self._on_start)

def _on_start(self, _: object):
self.unit.get_container('mycontainer').push('/foo.txt', 'hello')

ctx = Context(
MyCharm,
meta={'name': 'foo', 'containers': {'mycontainer': {}}},
)
container = Container(name='mycontainer', can_connect=True)
state = State(containers={container})

# First run populates the container root with a file.
ctx.run(ctx.on.start(), state)

# Second run should warn that the container root is non-empty.
with caplog.at_level(logging.WARNING, logger='ops-scenario.mocking'):
ctx.run(ctx.on.start(), state)

assert any(
'mycontainer' in record.message and 'non-empty' in record.message
for record in caplog.records
)


def test_no_warning_on_empty_container(caplog: pytest.LogCaptureFixture):
ctx = Context(
CharmBase,
meta={'name': 'foo', 'containers': {'mycontainer': {}}},
)
container = Container(name='mycontainer', can_connect=True)
state = State(containers={container})

# First run creates the container root.
ctx.run(ctx.on.start(), state)

# Second run should not warn since the container root is empty.
with caplog.at_level(logging.WARNING, logger='ops-scenario.mocking'):
ctx.run(ctx.on.start(), state)
Comment on lines +923 to +928
Copy link
Copy Markdown
Contributor

@dimaqq dimaqq Mar 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reuse the Context like this at times. I'm not sure I want to see warnings.
(to be fair, not start+start, rather start+config-changed+pebble-ready)

Would there always be a warning (because scenario dropped some files in), or only if my python-code-under-test explicitly modified container contents?

Copy link
Copy Markdown
Contributor Author

@james-garner-canonical james-garner-canonical Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if your code under test explicitly modifies container contents, as this test asserts.

It would be good to see some examples of Context reuse if you have any handy, given the docs say:

    Note that you can't call ``run()`` multiple times. The context 'pauses' ops
    right before emitting the event, but otherwise this is a regular test; you
    can't emit multiple events in a single charm execution.

Copy link
Copy Markdown
Collaborator

@tonyandrewmeyer tonyandrewmeyer Mar 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to see some examples of Context reuse if you have any handy, given the docs say:

    Note that you can't call ``run()`` multiple times. The context 'pauses' ops
    right before emitting the event, but otherwise this is a regular test; you
    can't emit multiple events in a single charm execution.

I think what that means (maybe the wording should be clearer? I think it's pretty old and probably aimed more at how Scenario used to be) is that you cannot do something like:

ctx = Context(MyCharm)
with ctx(ctx.on.config_changed(), State()):
    ctx.run(ctx.on.update_status(), State()

A "single charm execution" is, effectively, ctx.run. You can't run multiple events inside a "run".

But, like Dima, I feel it's legitimate to do:

ctx = Context(MyCharm)
s2 = ctx.run(ctx.on.relation_joined(rel), s1)
s3 = ctx.run(ctx.on.relation_changed(rel), s2)

However, I think Pietro disagrees, based on the linked issue.

However, if we're only warning when there are files that will be destroyed, I think that's a reasonable compromise.

Copy link
Copy Markdown
Contributor

@dimaqq dimaqq Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to see some examples of Context reuse if you have any handy

test_workload_version_is_setted in zookeeper-k8s-operator/tests/unit/test_charm.py

See, Pietro is a reconciler, he'd have a fresh Context every time.
Delta charmers are not so lucky, they have build charm inner state bit by bit.

Copy link
Copy Markdown
Contributor Author

@james-garner-canonical james-garner-canonical Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe container directory contents should be captured in the output State so that passing a State along between multiple run calls with the same Context matches real behaviour.


assert not any(
'mycontainer' in record.message and 'non-empty' in record.message
for record in caplog.records
)