Skip to content
This repository has been archived by the owner on Oct 10, 2024. It is now read-only.

DO NOT MERGE: First draft of potential API adjustments #1

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tonyandrewmeyer
Copy link
Owner

A discussion starting point.

Copy link

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

I've gone through this for our discussion this afternoon. Some of my comments are terse -- that's not me being short with you, just got sick of typing so much! :-)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
assert out.unit_status == ActiveStatus('I rule' if leader else 'I am ruled')
ctx = scenario.Context(MyCharm, meta={"name": "foo"})
out = ctx.run(MyCharm.on.start, scenario.State(leader=leader))
assert out.unit_status == ops.ActiveStatus('I rule' if leader else 'I am ruled')
Copy link

Choose a reason for hiding this comment

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

Again, not related to this change, but IMO this would be clearer without introducing parametrized tests here, just as two dumb, hard-coded tests:

def test_status_leader():
    ctx = scenario.Context(MyCharm, meta={"name": "foo"})
    out = ctx.run(MyCharm.on.start, scenario.State(leader=True))
    assert out.unit_status == ops.ActiveStatus('I rule')

def test_status_non_leader():
    ctx = scenario.Context(MyCharm, meta={"name": "foo"})
    out = ctx.run(MyCharm.on.start, scenario.State(leader=False))
    assert out.unit_status == ops.ActiveStatus('I am ruled')

ctx = Context(MyCharm, meta={"name": "foo"})
out = ctx.run(Event("start"), State())
assert out.unit_status == UnknownStatus()
ctx = scenario.Context(MyCharm, meta={"name": "foo"})
Copy link

Choose a reason for hiding this comment

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

Normally Scenario users won't need this meta={...} param, as it'll be picked up automatically. Can we remove it in the examples? Longer-term, we should probably use doctests or something so these examples are runnable.

README.md Outdated Show resolved Hide resolved
README.md Outdated

class MyCharm(...):
...
# So many other changes have been to avoid these string names, so this seems awkward.
Copy link

Choose a reason for hiding this comment

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

Agreed!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Can we make either of these possible?

assert isinstance(state.out.deferred[0], ops.StartEvent)
assert isinstance(state.out.deferred[0].event, ops.StartEvent)     # state.out.deferred[0]: DeferredEvent

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Repository owner deleted a comment from benhoyt Mar 1, 2024
Repository owner deleted a comment from benhoyt Mar 1, 2024
Repository owner deleted a comment from benhoyt Mar 1, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants