Skip to content
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.

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

fix: require the same object to be in the testing state as in the event #1468

Conversation

tonyandrewmeyer
Copy link
Contributor

This was incorrectly changed during the import of the ops-scenario code to this repo. As discussed in #1429, having an object in the state that has the same identifier as the object in the event is insufficient and could end up being confusing: it should be the same actual object.

As well as reverting the previous change, adjust the error messages so that they're clearer if this case is triggered.

Fixes #1429

Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but I have to trust you on the minute semantics difference.

Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

Makes sense, I see this is just rolling back an incorrect change and better documenting the behaviour in error messages

@james-garner-canonical
Copy link
Contributor

Why did a bunch of tests get cancelled tho?

@tonyandrewmeyer
Copy link
Contributor Author

Why did a bunch of tests get cancelled tho?

Yeah, something weird is happening there - the cancelled ones are because one failed, which is for some strange "environment inconsistent" thing, which I have not seen before, and doesn't appear related to the changes.

I'll look into it (and it obviously blocks merging anyway).

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Nov 27, 2024

The code looks good to me, but I have to trust you on the minute semantics difference.

Basically, it's this:

In main:

>>> import dataclasses
>>> import ops
>>> from ops import testing
>>> ctx = testing.Context(ops.CharmBase, meta={'name': 'n', 'requires': {'foo': {'interface': 'i'}}})
>>> r = testing.Relation('foo')
>>> r2 = dataclasses.replace(r, interface='x')
>>> r.id, r2.id
(1, 1)
>>> ctx.run(ctx.on.relation_changed(r), testing.State(relations={r2}))
State(config={}, relations=frozenset({Relation(endpoint='foo', interface='x', id=1, local_app_data={}, local_unit_data={'egress-subnets': '192.0.2.0', 'ingress-address': '192.0.2.0', 'private-address': '192.0.2.0'}, remote_app_name='remote', limit=1, remote_app_data={}, remote_units_data={0: {'egress-subnets': '192.0.2.0', 'ingress-address': '192.0.2.0', 'private-address': '192.0.2.0'}})}), networks=frozenset(), containers=frozenset(), storages=frozenset(), opened_ports=frozenset(), leader=False, model=Model(name='qzYc1M0vTmxETBYBRce4', uuid='888c4943-2e56-4913-bf1b-7bb760fbb562', type='kubernetes', cloud_spec=None), secrets=frozenset(), resources=frozenset(), planned_units=1, deferred=[], stored_states=frozenset({StoredState(name='_stored', owner_path=None, content={'event_count': 4}, _data_type_name='StoredStateData')}), app_status=UnknownStatus(), unit_status=UnknownStatus(), workload_version='')
>>> 

In the branch:

>>> import dataclasses
>>> import ops
>>> from ops import testing
>>> ctx = testing.Context(ops.CharmBase, meta={'name': 'n', 'requires': {'foo': {'interface': 'i'}}})
>>> r = testing.Relation('foo')
>>> r2 = dataclasses.replace(r, interface='x')
>>> r.id, r2.id
(1, 1)
>>> ctx.run(ctx.on.relation_changed(r), testing.State(relations={r2}))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tameyer/code/operator/testing/src/scenario/context.py", line 665, in run
    with self._run(event=event, state=state) as manager:
  File "/usr/lib/python3.12/contextlib.py", line 137, in __enter__
    return next(self.gen)
           ^^^^^^^^^^^^^^
  File "/home/tameyer/code/operator/testing/src/scenario/context.py", line 687, in _run
    with runtime.exec(
  File "/usr/lib/python3.12/contextlib.py", line 137, in __enter__
    return next(self.gen)
           ^^^^^^^^^^^^^^
  File "/home/tameyer/code/operator/testing/src/scenario/runtime.py", line 435, in exec
    check_consistency(state, event, self._charm_spec, self._juju_version)
  File "/home/tameyer/code/operator/testing/src/scenario/_consistency_checker.py", line 125, in check_consistency
    raise InconsistentScenarioError(
scenario.errors.InconsistentScenarioError: Inconsistent scenario. The following errors were found: cannot emit foo_relation_changed because relation 2 is not in the state (a relation with the same ID is not sufficient - you must pass the object in the state to the event).
>>> 

In main, r and r2 have the same ID, so they are somewhat the same relation as far as Juju would be concerned, but they are different objects in the test with different values for other attributes, so it's weird (and potentially problematic) that you can use one to 'bind' the event, and put the other one in the state.

@tonyandrewmeyer
Copy link
Contributor Author

Why did a bunch of tests get cancelled tho?

Yeah, something weird is happening there - the cancelled ones are because one failed, which is for some strange "environment inconsistent" thing, which I have not seen before, and doesn't appear related to the changes.

I'll look into it (and it obviously blocks merging anyway).

Not sure what happened here. One test failed with a weird environment error that seems unrelated to the changes, and when I re-ran it, it passed. Then there were several others that were cancelled since that one failed, but they all passed when I re-ran them.

I'm assuming this was some GitHub workflows glitch.

@tonyandrewmeyer tonyandrewmeyer merged commit 5ecee69 into canonical:main Nov 27, 2024
29 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the consistency-check-comparisons-142 branch November 27, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consistency checker gives false positives when working with copied objects
3 participants