Update the hasValue constraint test by adding a dedicated file #25344

vivien-apple · 2023-02-27T16:00:17Z

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.

bzbarsky-apple

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

vivien-apple · 2023-02-28T11:20:58Z

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.

tehampson · 2023-02-28T13:46:57Z

scripts/py_matter_yamltests/matter_yamltests/constraints.py

@@ -124,6 +140,11 @@ def __init__(self, context, types: list, is_null_allowed: bool = False):
        self._context = context

    def validate(self, value, value_type_name):


nit, add type hinting value as ConstraintValue

tehampson · 2023-02-28T13:57:48Z

scripts/tests/run_test_suite.py

@@ -181,6 +181,8 @@ def main(context, dry_run, log_level, target, target_glob, target_skip_glob,
    # Figures out selected test that match the given name(s)
    if runtime == TestRunTime.CHIP_REPL_PYTHON:
        all_tests = [test for test in chiptest.AllYamlTests()]


Should we expect to converge on using the same subset of test? What is preventing us from using the same subset right now? The only thing that is not enabled for chip-repl is Test_TC_OO_2_4.yaml which is due to a real issue with the test which is only masked with the chip-tool due to some delays happening during runtime.

Andrei's argument is right now we have an allow list with src/app/tests/suites/ciTests.json when really we should have a blocklist somewhat to what we are doing already in this file going forward

tehampson · 2023-02-28T13:58:28Z

scripts/tests/chiptest/__init__.py

@@ -125,6 +125,13 @@ def _GetSlowTests() -> Set[str]:
    }


+def _GetPythonYamlOnlyTests() -> Set[str]:


What is currently making this a test that only works in python and not codegen?

tehampson · 2023-02-28T14:00:58Z

src/app/tests/suites/certification/ci-pics-values

@@ -1823,8 +1823,8 @@ DESC.C.A0002=1
 DESC.C.A0003=1

 # Secure Channel
-MCORE.SC.SII_COMM_DISCOVERY_KEY=1
-MCORE.SC.SAI_COMM_DISCOVERY_KEY=1
+MCORE.SC.SII_COMM_DISCOVERY_KEY=0


Why are these PICS values changing?

bzbarsky-apple · 2023-02-28T18:42:07Z

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?

stale · 2023-04-30T10:46:58Z

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 · 2023-09-17T07:21:24Z

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 · 2024-03-13T10:05:14Z

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.

andy31415 · 2024-10-11T13:42:30Z

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

vivien-apple self-assigned this Feb 27, 2023

github-actions bot added app controller darwin examples scripts tests tools labels Feb 27, 2023

bzbarsky-apple reviewed Feb 27, 2023

View reviewed changes

vivien-apple force-pushed the YamlTests_ConstraintHasValue branch from 4a0e3e9 to 1e254f7 Compare February 28, 2023 11:24

github-actions bot removed the tools label Feb 28, 2023

vivien-apple added 2 commits February 28, 2023 12:25

Update the hasValue constraint test by adding a dedicated file

da0d451

Update generated code

344d8da

vivien-apple force-pushed the YamlTests_ConstraintHasValue branch from 1e254f7 to 344d8da Compare February 28, 2023 11:26

vivien-apple changed the title ~~Yaml tests constraint has value~~ Update the hasValue constraint test by adding a dedicated file Feb 28, 2023

tehampson reviewed Feb 28, 2023

View reviewed changes

stale bot added the stale Stale issue or PR label Apr 30, 2023

bzbarsky-apple removed the stale Stale issue or PR label May 2, 2023

woody-apple added this to the 1.2 milestone Jun 23, 2023

stale bot added the stale Stale issue or PR label Sep 17, 2023

bzbarsky-apple removed the stale Stale issue or PR label Sep 18, 2023

woody-apple modified the milestones: 1.2, 1.3 Feature Complete, 1.3, 1.3 Release Oct 19, 2023

woody-apple added this to the 1.3 Test Complete milestone Oct 19, 2023

stale bot added the stale Stale issue or PR label Mar 13, 2024

bzbarsky-apple removed the stale Stale issue or PR label Mar 13, 2024

woody-apple modified the milestones: 1.3 Test Complete, 1.4 Release May 6, 2024

andy31415 closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the hasValue constraint test by adding a dedicated file #25344

Update the hasValue constraint test by adding a dedicated file #25344

vivien-apple commented Feb 27, 2023

bzbarsky-apple left a comment

vivien-apple commented Feb 28, 2023

tehampson Feb 28, 2023

tehampson Feb 28, 2023

tehampson Feb 28, 2023

tehampson Feb 28, 2023

bzbarsky-apple commented Feb 28, 2023

stale bot commented Apr 30, 2023

stale bot commented Sep 17, 2023

stale bot commented Mar 13, 2024

andy31415 commented Oct 11, 2024

		@@ -124,6 +140,11 @@ def __init__(self, context, types: list, is_null_allowed: bool = False):
		self._context = context

		def validate(self, value, value_type_name):

		@@ -125,6 +125,13 @@ def _GetSlowTests() -> Set[str]:
		}


		def _GetPythonYamlOnlyTests() -> Set[str]:

Update the hasValue constraint test by adding a dedicated file #25344

Update the hasValue constraint test by adding a dedicated file #25344

Conversation

vivien-apple commented Feb 27, 2023

Problem

bzbarsky-apple left a comment

Choose a reason for hiding this comment

vivien-apple commented Feb 28, 2023

tehampson Feb 28, 2023

Choose a reason for hiding this comment

tehampson Feb 28, 2023

Choose a reason for hiding this comment

tehampson Feb 28, 2023

Choose a reason for hiding this comment

tehampson Feb 28, 2023

Choose a reason for hiding this comment

bzbarsky-apple commented Feb 28, 2023

stale bot commented Apr 30, 2023

stale bot commented Sep 17, 2023

stale bot commented Mar 13, 2024

andy31415 commented Oct 11, 2024