fix: treat remote unit zero as explicit#2454
Conversation
gcomneno
force-pushed
the
contrib/issue-2341-scenario-remote-unit-zero
branch
from
d8e8bac to
b941d6c
Compare
|
Hi! Just gently following up on this one. I rebased the branch onto current
The focused test run passes with Happy to adjust anything else if useful. |
tonyandrewmeyer
left a comment
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
Apologies for the delay here; the last 3-4 weeks have been very busy with other things.
The fix looks correct to me -- zero is indeed a valid ID and we were wrongly checking false-iness.
Thanks!
tonyandrewmeyer
changed the title
|
@copilot please fix the linting (run |
| layer = ops.pebble.Layer({ | ||
| 'checks': { | ||
| 'chk1': { | ||
| 'override': 'replace', | ||
| 'level': 'ready', | ||
| 'startup': 'disabled', | ||
| 'threshold': 42, | ||
| 'http': {'url': 'http://localhost:5000/version'}, | ||
| layer = ops.pebble.Layer( | ||
| { | ||
| 'checks': { | ||
| 'chk1': { | ||
| 'override': 'replace', | ||
| 'level': 'ready', | ||
| 'startup': 'disabled', | ||
| 'threshold': 42, | ||
| 'http': {'url': 'http://localhost:5000/version'}, | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Spurious change?
There was a problem hiding this comment.
I think perhaps this is why the linter is unhappy, but for some reason Copilot has not followed up on my request to fix it, and I was too lazy to check out the branch and do it myself.
@gcomneno can you run tox -e format and then tox -e lint? You should get the same result as in CI (the format should fix whatever it is). Generally we shouldn't end up with style changes like the above.
| def test_relation_changed_remote_unit_zero_is_explicit(): | ||
| relation = Relation('foo', remote_units_data={0: {}}) | ||
| assert_consistent( | ||
| State(relations={relation}), | ||
| _Event('foo_relation_changed', relation=relation, relation_remote_unit_id=0), | ||
| _CharmSpec(MyCharm, {'requires': {'foo': {'interface': 'bar'}}}), | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Is there still an implicit remote unit zero case that should raise a consistency checker error? Can we add a test for it?
|
Thanks, good catch. I ran For the extra negative coverage: I also verified the focused tests with:
|
3 participants
Fixes #2341.
This updates the Scenario consistency checker to distinguish between a missing remote unit id and an explicit remote unit id of
0.0is a valid Juju unit id, so the checker should not treat it as falsy/absent.Changes:
relation_remote_unit_id is Noneinstead of relying on truthinessrelation_remote_unit_id=0Validation:
uv tool run --with tox-uv tox -e unit -- testing/tests/test_consistency_checker.py -k "relation_not_in_state or remote_unit_zero" -rs -vv --tb=shortuv tool run --with tox-uv tox -e unit -- testing/tests/test_consistency_checker.py -rsruff check testing/src/scenario/_consistency_checker.py testing/tests/test_consistency_checker.py