search

canonical / ops-scenario

State-transition testing SDK for Operator Framework Juju charms.
Apache License 2.0
10 stars 7 forks source link

Minor issues I found when reviewing the new 7.0 API #175

Closed benhoyt closed 6 days ago

benhoyt commented 3 weeks ago

Just opening an issue here for lack of a better place. The 7.0 API is looking really good IMO! Here are the (minor) issues I found:

tonyandrewmeyer commented 3 weeks ago
  • BindAddress and Network have public method hook_tool_output_fmt; can those be private?
  • do we need custom exception types for things like MetadataNotFoundError? seems like just an Exception with a message would be fine, as you're not going to catch this

I do think a custom exception class does make the error more immediately clear than a generic Exception with message. But I don't feel strongly about it, and indeed you're not going to be catching it so a custom class certainly isn't required. Thoughts @PietroPasotti ?

  • State's docstring refers to "State.status", but there's only unit_status and app_status
  • State has with_can_connect(), with_leadership(), with_unit_status() -- should we drop those?

I had wondered about this too - they don't offer a huge amount over just doing the .replace call yourself - especially with_unit_status which doesn't seem like it would often be needed (I don't think charms are reading the status that often?). @PietroPasotti thoughts on this, or more info about why they were added?

  • should the state.next_*() functions be public?

The use-case is that it makes it easier to use dataclasses.replace - the README has an example for relations. I think that still makes sense.

I had originally thought the same applied to notices, e.g. you set up a notice and then you want to do replace(occurrences=2) or something, but you'd actually want the same ID in that case, not the "next" one. I guess maybe you might have a bunch of notices that have mostly similar content but need different IDs, so the replace method would be convenient. However, I think on balance that it would make more sense to make this private and then if we find that there is a use-case we can make it public later.

There's a lot less to a Storage, so it's probably not worthwhile there. If you're simulating adding multiple storage instances to a machine charm, then storage2 = Storage(name) is actually simpler than storage2 = dataclasses.replace(storage1, index=next_storage_index()).

  • should satate.normalize_name() be public?
  • there's some funky ``'s (double backticks) in the doc output of the Context docstring

Is that fixed in #169? It's a bit vague for me to be sure, but I would guess so - I read over the actual docs there reasonably carefully (although still missed the State.status example above). Dora is also going to review that PR so will hopefully catch anything bad.

  • is Context.output_state needed? Won't you always get that from .run()?

Good question. Thoughts @PietroPasotti ? .output was removed from the context manager similarly. It seems like this could indeed be private.

  • scenario.consistency_checker is in the API docs output, as is scenario.capture_events -- those aren't intended to be public, right?

I was looking at this in #169 also, because there's currently this slightly odd situation where almost everything that is public is in the top-level scenario namespace. However:

PietroPasotti commented 3 weeks ago
  • do we need custom exception types for things like MetadataNotFoundError? seems like just an Exception with a message would be fine, as you're not going to catch this

I do think a custom exception class does make the error more immediately clear than a generic Exception with message. But I don't feel strongly about it, and indeed you're not going to be catching it so a custom class certainly isn't required. Thoughts @PietroPasotti ?

Weak preference for custom exceptions just because it makes it more immediately clear from the exception name what the error is about.

  • State has with_can_connect(), with_leadership(), with_unit_status() -- should we drop those?

I had wondered about this too - they don't offer a huge amount over just doing the .replace call yourself - especially with_unit_status which doesn't seem like it would often be needed (I don't think charms are reading the status that often?). @PietroPasotti thoughts on this, or more info about why they were added?

That was meant as syntactic sugar for doing variations of the .replace() call but deal with nested objects. This can go for now. I don't remember what we concluded RE the 'remap' api #20, but that would essentially solve this same problem in a more generic and elegant way.

  • should the state.next_*() functions be public?

The use-case is that it makes it easier to use dataclasses.replace - the README has an example for relations. I think that still makes sense.

I had originally thought the same applied to notices, e.g. you set up a notice and then you want to do replace(occurrences=2) or something, but you'd actually want the same ID in that case, not the "next" one. I guess maybe you might have a bunch of notices that have mostly similar content but need different IDs, so the replace method would be convenient. However, I think on balance that it would make more sense to make this private and then if we find that there is a use-case we can make it public later.

I think the use case is still present. We could hide them if we were to expose a .copy(keep_id:bool) method on all State objects that are supposed to have unique juju-given ids.

  • is Context.output_state needed? Won't you always get that from .run()?

Good question. Thoughts @PietroPasotti ? .output was removed from the context manager similarly. It seems like this could indeed be private.

It will force people to pass around the reference to the output state as returned from .run(), but I guess that's a good thing.

  • scenario.consistency_checker is in the API docs output, as is scenario.capture_events -- those aren't intended to be public, right?

I was looking at this in #169 also, because there's currently this slightly odd situation where almost everything that is public is in the top-level scenario namespace. However:

  • scenario.context has: DEFAULT_JUJU_VERSION, InvalidEventError, ContextSetupError, AlreadyEmittedError. This might change depending on the "custom exceptions" question above, but at the moment if you want to catch these you would need to import them from scenario.context, not scenario. However, I don't think anyone should be catching these, because they are indications that your test is broken, not that your charm is broken. So it seems like they should be documented and public so that people know what it means when your test crashes with one, but that they don't belong in top-level scenario. For DEFAULT_JUJU_VERSION, I'm thinking that it should probably either not exist (and the default value just goes into the Context.__init__ signature, or it should be a class variable on Context.
  • scenario.state has: ATTACH_ALL_STORAGES, CREATE_ALL_RELATIONS, BREAK_ALL_RELATIONS, DETACH_ALL_STORAGES, BUILTIN_EVENTS, FRAMEWORK_EVENTS, SECRET_EVENTS, META_EVENTS. Since events aren't referred to by name when using run any more, these are less useful. @PietroPasotti would you be ok with these being removed from the public API?

Yes.

  • [ ] scenario.state also has: ACTION_EVENT_SUFFIX, PEBBLE_READY_EVENT_SUFFIX, PEBBLE_CUSTOM_NOTICE_EVENT_SUFFIX, PEBBLE_CHECK_FAILED_EVENT_SUFFIX, PEBBLE_CHECK_RECOVERED_EVENT_SUFFIX, RELATION_EVENTS_SUFFIX, STORAGE_EVENTS_SUFFIX. I think these should be private.

Yes. Although one could argue that not being in the scenario namespace already is private enough for a testing framework?

  • scenario.state also has:

    • DEFAULT_JUJU_DATABAG: maybe a class attribute of RelationBase?

Fine with this.

  • AnyJson, AnyRelation, CharmType, PathLike, Port, RawDataBagContents, RawSecretRevisionContents, RelationBase, UnitID: these are really there for typing, so should be public I think. But then I'm thinking they probably should actually be in the top-level scenario namespace?

Not sure what's the best practice here. It'd be odd to have stuff like this in toplevel scenario. We could consider putting them in a types.py module, and were one to need them, they'd have to import scenario.types?

  • [ ] JujuLogLine: I think this should actually just move to the top-level namespace.

I don't see anyone actually importing this, but if it's for consistency...?

  • MetadataNotFoundError: much like the errors in context.py, I don't think anyone ought to catch this, so it doesn't belong in scenario but I think it should be public.

+1

  • scenario.consistency_checker has some public and some private methods, and you can explicitly do a consistency check like scenario.consistency_checker.check_consistency(state, ctx.on.start(), ctx.charm_spec, "3.5.4"). However, since it's automatically done, I'm not sure why you would. What's your suggestion here? An empty __all__, or making the module name _consistency_checker or making all the methods private, or leaving them as they are and just excluding the module from the docs?

yeah I don't think anyone will want to directly invoke the consistency checker unless you're doing fancy stuff like filtering out automatically-generated state/event combinations like I was trying to do in the hypothesis branch. Fine with 'hiding it'.

Side-thought: perhaps we should add a with consistency_checker_disabled() context, more user-friendly than the envvar.

  • scenario.capture_events only has the capture_events context manager: this does have a clearer use-case. I wonder if it should just be moved to the top-level scenario namespace?

I think we could remove the module altogether and put the context manager in runtime.py (IIRC that's where it's used?) Also, it probably should be made private. The only purpose of keeping it public is that it offers some more 'fine-tuning' of what's being captured than what Context offers. We could make Context capture all events, and let the user do the filtering themselves.

tonyandrewmeyer commented 3 weeks ago

I don't remember what we concluded RE the 'remap' api #20,

I think it was "this a good idea that we have no time to figure out now but should look into later".

but that would essentially solve this same problem in a more generic and elegant way.

:+1:

I think the use case is still present.

I agree for relations, but do you think there is for notices and storage? Storage seems too simple, and notices don't seem to fit because of the way Pebble collapses them into a single notice (per type/key).

We could hide them if we were to expose a .copy(keep_id:bool) method on all State objects that are supposed to have unique juju-given ids.

In, say, a _DCBase parent class? :joy:

  • AnyJson, AnyRelation, CharmType, PathLike, Port, RawDataBagContents, RawSecretRevisionContents, RelationBase, UnitID: these are really there for typing, so should be public I think. But then I'm thinking they probably should actually be in the top-level scenario namespace?

Not sure what's the best practice here. It'd be odd to have stuff like this in toplevel scenario. We could consider putting them in a types.py module, and were one to need them, they'd have to import scenario.types?

Hmm, scenario.types is interesting. I like that it makes it clear that these aren't classes you should be making objects of. It does feel cleaner to me than throwing everything in scenario, and I like that it's not encouraging people to import stuff from scenario.state.

  • [ ] JujuLogLine: I think this should actually just move to the top-level namespace.

I don't see anyone actually importing this, but if it's for consistency...?

I guess it's most likely to be used for types as well. I also don't see anyone pre-populating the Context with log lines.

Side-thought: perhaps we should add a with consistency_checker_disabled() context, more user-friendly than the envvar.

Do you have any sense of how often people have to do this? I guess the only cases you know about would be when someone asks about something and you have to suggest disabling it temporarily because it's a bug.

I think we could remove the module altogether and put the context manager in runtime.py

:+1:

Also, it probably should be made private.

:+1:

PietroPasotti commented 3 weeks ago

I don't remember what we concluded RE the 'remap' api #20,

I think it was "this a good idea that we have no time to figure out now but should look into later".

Ok then let's add this to the 'reasons why that's a good idea list'.

I think the use case is still present.

I agree for relations, but do you think there is for notices and storage? Storage seems too simple, and notices don't seem to fit because of the way Pebble collapses them into a single notice (per type/key).

Mm yes, it's only relations.

We could hide them if we were to expose a .copy(keep_id:bool) method on all State objects that are supposed to have unique juju-given ids.

In, say, a _DCBase parent class? 😂

And that, indeed, is where this was getting at. Caught me red-handed :P

  • [ ] JujuLogLine: I think this should actually just move to the top-level namespace.

I don't see anyone actually importing this, but if it's for consistency...?

I guess it's most likely to be used for types as well. I also don't see anyone pre-populating the Context with log lines.

Nope, that's not something we should allow/encourage.

Side-thought: perhaps we should add a with consistency_checker_disabled() context, more user-friendly than the envvar.

Do you have any sense of how often people have to do this? I guess the only cases you know about would be when someone asks about something and you have to suggest disabling it temporarily because it's a bug.

Yes that is the only use case. In principle the checker is there to help you ensure you're not making up juju-impossible scenarios, but imagine the frustration if you can't turn it off if you KNOW it's a false positive.