fix: make testing.CheckInfo arg types match pebble.CheckInfo

[james-garner-canonical]

The signature of testing.CheckInfo doesn't quite match that of pebble.CheckInfo. While pebble.CheckInfo will accept a str for level, the testing version only accepts a pebble.CheckLevel | None. This is a nicer experience for users of the testing.CheckInfo.level attribute, but leads to a type error when passing a pebble.Check.level to the testing.CheckInfo constructor. This was reported in #1790.

I believe the correct solution is to broaden the types accepted by the testing.CheckInfo constructor without broadening the corresponding level attribute. This PR does so by setting init=False and adding a custom __init__ method to testing.CheckInfo. This makes the argument types match and resolves this type checking error, while retaining the nicer attribute typing.

While investigating this, I noticed that there is a similar issue with pebble.Check.threshold being None-able, while the threshold argument to testing.CheckInfo is not. This differs from the level case in that pebble.CheckInfo does not accept None for the threshold argument. Currently this PR 'fixes' this for testing.CheckInfo by again broadening the types accepted by the constructor, while keeping the attribute the same, however this introduces a new mismatch between the pebble and testing versions of this class. Perhaps this enhancement should be dropped from this PR, or perhaps the PR should also include applying it to pebble.CheckInfo.

Fixes: #1790

---

The file inlined below (adapted from the example in the issue) can be used to validate these changes -- perhaps it's worth adding similar constructs to one of our tests?

# Copyright 2026 Canonical Ltd.
# Add this file as e.g. _tmp.py and validate that type checking continues to pass

import ops
from ops import pebble, testing


def _(my_charm_type: type[ops.CharmBase]):
    ctx = testing.Context(my_charm_type, meta={'name': 'foo', 'containers': {'my_container': {}}})
    layer = pebble.Layer({
        'checks': {'http-check': {'override': 'replace', 'startup': 'enabled', 'threshold': 7}},
    })
    check_info = testing.CheckInfo(
        'http-check',
        status=pebble.CheckStatus.UP,
        level=layer.checks['http-check'].level,
        startup=layer.checks['http-check'].startup,
        threshold=layer.checks['http-check'].threshold,
    )
    container = testing.Container(
        'my_container', check_infos={check_info}, layers={'layer1': layer}
    )
    state = testing.State(containers={container})
    ctx.run(ctx.on.pebble_check_failed(info=check_info, container=container), state=state)

On main we get these errors:

_tmp.py:15:15 - error: Argument of type "CheckLevel | str" cannot be assigned to parameter "level" of type "CheckLevel | None" in function "__init__"
  Type "CheckLevel | str" is not assignable to type "CheckLevel | None"
    Type "str" is not assignable to type "CheckLevel | None"
      "str" is not assignable to "CheckLevel"
      "str" is not assignable to "None" (reportArgumentType)
tmp.py:17:19 - error: Argument of type "int | None" cannot be assigned to parameter "threshold" of type "int" in function "__init__"
  Type "int | None" is not assignable to type "int"
    "None" is not assignable to "int" (reportArgumentType)

[james-garner-canonical]

With init=False, these are now only attribute annotations, so the default values can go, as they're redundant with those defined in the __init__ method.

[tonyandrewmeyer]

There is also a documentation change, from:

To:

I do find it convenient to look at the field and see that the default is None, rather than having to scroll up and look at the __init__. However, I don't think we want to have the default in both places since then there's the risk that they get out of sync.

[james-garner-canonical]

Agreed.

[james-garner-canonical]

This is the key change from previously -- we now accept str as well.

[james-garner-canonical]

This is also a change from the previous behaviour, as noted in the PR description -- accepting None (and converting it to 0 below). Perhaps it should be dropped from this PR, or perhaps pebble.CheckInfo should be updated to match, or perhaps it's fine as-is, I'm not sure.

[benhoyt]

Do we know that converting None (from a pebble.CheckInfo) to 0 is the correct move, from a Pebble point of view?

[james-garner-canonical]

threshold apparently being able to be None originates from pebble.Check (and pebble.CheckDict). I assumed that None and 0 should be treated equivalently because when we turn a Check into a dict we skip all fields with False-y values.

On the pebble side, threshold can't be nil. IIRC Pebble always uses 'omit empty' when sending JSON, so it would only be missing (and thus None on the Python side) if it was set to 0. However, the default threshold is 3, and it doesn't make any sense for the threshold to be 0 (health check fails if there are 0 failures in a row), so I'm guessing the zero-so-omit case never comes up in the real world?

This makes it seem like the right thing is to make pebble.Check.threshold not be None-able, which is a bit of a bigger question, so maybe it's best to leave changes to testing.CheckInfo.threshold out of this PR.

[tonyandrewmeyer]

threshold can be None in Check (the thing you use to build a layer), but it cannot be None in a CheckInfo (the thing you get from Pebble when you ask about the current status of a check).

It makes no sense for threshold to be None here, because it'll always be in the Pebble response, so we should definitely not make this change. successes can only be None because it's supporting old Pebble versions where we didn't have that field.

For a Check, None means "use the default value". That does make sense, and should also not be changed.

[james-garner-canonical]

threshold can be None in Check (the thing you use to build a layer), but it cannot be None in a CheckInfo (the thing you get from Pebble when you ask about the current status of a check).

...

For a Check, None means "use the default value". That does make sense, and should also not be changed.

I'm fine with dropping all the changes to threshold from this PR, and good to know that it's intended functionality for threshold to be None-able in Check objects (I guess when constructing them manually? Or does Pebble omit threshold from Check too?).

But I'm wondering, if testing.CheckInfo(threshold=my_check.threshold, ...) is a useful pattern, then should passing None to the CheckInfo constructor also mean "use the default value" (3)?

[benhoyt]

Thanks for this. I definitely think it'd be a good idea to add a test of this new behaviour / typing.

A thought on a possible way to simplify: could you define a _CheckInfo dataclass with most of the fields, and then inherit CheckInfo from that to add the special fields, calling the super()'s __init__ so you only need to special case the special fields?

[benhoyt]

Do we know that converting None (from a pebble.CheckInfo) to 0 is the correct move, from a Pebble point of view?

[benhoyt]

I'm having a thinko -- why do we need object.__setattr__ here, instead of just self.foo = foo?

[james-garner-canonical]

Because this dataclass has frozen=True -- the __init__ method that dataclasses would generate with init=True would also use object.__setattr__, and we do the same in __post_init__.

[james-garner-canonical]

I definitely think it'd be a good idea to add a test of this new behaviour / typing.

Will do.

A thought on a possible way to simplify: could you define a _CheckInfo dataclass with most of the fields, and then inherit CheckInfo from that to add the special fields, calling the super()'s __init__ so you only need to special case the special fields?

I think that would work, but I'm not convinced it's truly simpler.

TBH I think we should be moving in the direction of having a custom __init__ for more of the Scenario dataclasses, for better type checking and auto-completion UX in the cases where the acceptable argument types and what the field will actually end up as at runtime diverge -- for example, there are fields that are typed as Iterable[...] because that's all the argument requires, but the attribute is always a frozenset after being initialized.

[benhoyt]

A thought on a possible way to simplify: could you define a _CheckInfo dataclass with most of the fields, and then inherit CheckInfo from that to add the special fields, calling the super()'s __init__ so you only need to special case the special fields?

I think that would work, but I'm not convinced it's truly simpler.

Fair enough. It's definitely more direct and flatter to do what you're suggesting. I don't love the object.__setattr__'s, but oh well.

[tonyandrewmeyer]

TBH I think we should be moving in the direction of having a custom __init__ for more of the Scenario dataclasses, for better type checking and auto-completion UX in the cases where the acceptable argument types and what the field will actually end up as at runtime diverge -- for example, there are fields that are typed as Iterable[...] because that's all the argument requires, but the attribute is always a frozenset after being initialized.

I agree with this (we've talked about this before, I think). Perhaps worth creating a ticket?

To record some history: we had a version with custom __init__ for most of the the State classes, primarily to support keyword-only fields in Python 3.8, but removed it in favour of using __new__. I believe the main reason was that we felt that it was cleaner to maintain in a single place than have to maintain a lot of __init__ methods.

However, we don't need that any more (since we're using 3.10+), so some of the classes are perfectly fine with the dataclasses default __init__. I feel like a custom __init__ to say "you can pass an iterable but you will end up with a set" is valuable enough that maintaining a few __init__'s is worthwhile.

[tonyandrewmeyer]

I haven't tested this out directly, but will do that after the fixes are done.

[tonyandrewmeyer]

There is also a documentation change, from:

To:

I do find it convenient to look at the field and see that the default is None, rather than having to scroll up and look at the __init__. However, I don't think we want to have the default in both places since then there's the risk that they get out of sync.

[tonyandrewmeyer]

threshold can be None in Check (the thing you use to build a layer), but it cannot be None in a CheckInfo (the thing you get from Pebble when you ask about the current status of a check).

It makes no sense for threshold to be None here, because it'll always be in the Pebble response, so we should definitely not make this change. successes can only be None because it's supporting old Pebble versions where we didn't have that field.

For a Check, None means "use the default value". That does make sense, and should also not be changed.

[james-garner-canonical]

Thanks for the feedback. Still need to figure out exactly what to do for threshold -- drop entirely or allow testing.CheckInfo to accept threshold=None as a signal to use the default value (3).

Tests are definitely needed here, I'll add them and re-request review at that point.