connectedhomeip icon indicating copy to clipboard operation
connectedhomeip copied to clipboard

Update the hasValue constraint test by adding a dedicated file

Open vivien-apple opened this issue 1 year ago • 5 comments

Problem

matter_yamltests is mixing null values and missing values. This PR add the necessary bits to differentiate both and also adds a new attribute of type struct to unittesting in order to properly validate that optional in struct are handled properly.

This is a draft until #25343 lands since chip-tool does not clean up optional fields properly at the moment.

vivien-apple avatar Feb 27 '23 16:02 vivien-apple

Does this match existing YAML semantics in terms of how constraints other than "hasValue" are applied (or not) to missing optional values?

For the moment this is stricter in the sense that a missing value with a a constraint other than hasValue will generate an error. This is because of the lines at the beginning of BaseConstraint.validate:

        if not value.has_value():
            reason = f"The constraint expects a value but there isn't one."
            self._raise_error(reason)

Removing those should bring us back to the same behaviour. I would like to run the CI against this PR to check if we do have some constraints on missing fields that were unexpected, but we can definitively remove it afterward.

vivien-apple avatar Feb 28 '23 11:02 vivien-apple

I would like to run the CI against this PR to check if we do have some constraints on missing fields that were unexpected

Running CI is not enough. We can have YAMLs that have such constraints on fields that all-clusters-app (which is what we use in CI) sends but that other devices might not send. If we change semantics here we need to look at all the relevant places in the YAML and make sure that it's correctly expressing the intent of the test plan.

The real check is: are there optionals that have constraints other than hasValue listed at all?

bzbarsky-apple avatar Feb 28 '23 18:02 bzbarsky-apple

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Apr 30 '23 10:04 stale[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Sep 17 '23 07:09 stale[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Mar 13 '24 10:03 stale[bot]

This is an old PR that does not seem to be updated, is in draft and has several merge conflicts. Closing as stale.

andy31415 avatar Oct 11 '24 13:10 andy31415