consistency checker gives false positives when working with copied objects

[tonyandrewmeyer]

This is a regression from Scenario 6:

MWE:

import ops
from scenario import Context
def test_relation_evt():
    ctx = Context(ops.CharmBase, meta={"name": "mateusz",
                                   "requires": {"a": {"interface": "b"}}})
    rel = Relation("a", interface="b")
    rel_cpy = replace(rel, remote_app_data={"foo": "bar"})

    ctx.run(ctx.on.relation_changed(rel_cpy), State(relations={rel}))
    # scenario.errors.InconsistentScenarioError: Inconsistent scenario.
    # The following errors were found: cannot emit a_relation_changed because 
    # relation 1 is not in the state.

culprit is this line in the consistency checker:

if event.relation not in state.relations:
    ... <mark as inconsistent>

Proposed solution:

if event.relation.id not in [r.id for r in state.relations]:
    ...

Moved from canonical/ops-scenario#199

[tonyandrewmeyer]

Hmm, I bundled a fix for this in the merge into the operator repository (I was wrongly thinking it was fixing something else). Now that I'm going through this properly and writing tests and so on, I'm not sure if it's the right thing to do or not (the original ops-scenario issue was less certain than the text above).

What I had missed here before is that rel is in the state but the event is bound to rel_cpy. It does seem a bit odd that the relation in the state can have different data than the relation in the event (with both having the same ID) - you could never have this in Juju, and I'm not sure without running it what data the output state would have (assuming the charm didn't change it).

If we don't allow this, then you'd need to do something like:

def test_relation_evt():
    ctx = Context(ops.CharmBase, meta={"name": "mateusz",
                                   "requires": {"a": {"interface": "b"}}})
    rel = Relation("a", interface="b")
    rel_cpy = replace(rel, remote_app_data={"foo": "bar"})
    relations = set(old_state.relations)
    relations.remove(rel)
    relations.add(rel_cpy)
    new_state = replace(old_state, relations=relations)

    ctx.run(ctx.on.relation_changed(rel_cpy), new_state))

This is a good argument for State.patch I think (already mostly made in that PR). That would let you do the change more consistently:

def test_relation_evt():
    ctx = Context(ops.CharmBase, meta={"name": "mateusz",
                                   "requires": {"a": {"interface": "b"}}})
    rel = Relation("a", interface="b")
    new_state = old_state.patch(rel, remote_app_data={"foo": "bar"})

    ctx.run(ctx.on.relation_changed(new_state.get_relation(rel.id)), new_state))

Thoughts @PietroPasotti and @benhoyt ?

[benhoyt]

I guess my main question is: why don't we just encourage test authors to build new data structures, rather than patch existing ones? The definition of old_state isn't shown in the examples above, but something like this:

def test_relation_evt():
    ctx = Context(ops.CharmBase, meta=...)
    rel = Relation("a", interface="b")
    old_state = State(relations={rel})
    ctx.run(ctx.on.relation_changed(rel), old_state)

    rel = Relation("a", interface="b", remote_app_data={"foo": "bar"})
    new_state = State(relations={rel})
    ctx.run(ctx.on_relation_changed(rel), new_state)

    # or perhaps this:
    rel = dataclasses.replace(rel, remote_app_data={"foo": "bar"})
    new_state = State(relations={rel})
    ctx.run(ctx.on_relation_changed(rel), new_state)

This way we're not mucking about with the relations set, but just building new objects each time. Or perhaps we need a realistic example of where the above wouldn't work so well?

[tonyandrewmeyer]

I probably muddied the waters here bringing up the patching case, sorry. Let's leave that for the discussion in a couple of weeks.

For this specific issue: if someone has two objects that have the same ID, and uses one of them to get the event but puts the other one in the state, should the consistency checker complain (the objects don't match) or be ok (there's an object with the right ID)? I think we should complain.

[benhoyt]

Ah, I see the core of the problem now. Yeah, complaining about that does seem like the right thing to do. It's pointing out a code smell, I think.