fix: warn before clearing non-empty container in testing

[james-garner-canonical]

In testing, Context.run will clear the directories created for fake container filesystems before executing the charm code. As #2041 reported, this may be unexpected if the user is trying to populate the container filesystem before calling Context.run. This PR therefore adds a warning if the directory to be cleared has any contents.

Resolves #2041

#2041 suggests that there are legitimate reasons to call Context.run twice with the same Context object:

OTOH one might be doing:

state2 = ctx.run(state1)
ctx.run(state2)

which means all containers will get wiped by the second call, to avoid polluting the env of the second execution with leftovers from the first one. That's why the functionality was designed this way and pretty much the only semi-legitimate reason why we allow the filesystem to be 'nonempty' when we initialize a new run.

However, the Context docstring says:

    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.

If any charm tests were calling Context.run multiple times with the same Context object, and expecting the container directory to be wiped, they will have a new warning in their test output. I think this is OK, because we recommend against using a context this way.

What I'm wondering is if we should go further: this could be a logging ERROR now, and we could make it a runtime exception in our next major release.

[dimaqq]

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?

[james-garner-canonical]

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.

[tonyandrewmeyer]

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.

[dimaqq]

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.

[james-garner-canonical]

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.