Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

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

wants to merge 2 commits into from

Conversation

vivien-apple
Copy link
Contributor

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 vivien-apple self-assigned this Feb 27, 2023
bzbarsky-apple
bzbarsky-apple reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

Does this match existing YAML semantics in terms of how constraints other than "hasValue" are applied (or not) to missing optional values?
@vivien-apple
Copy link
Contributor Author

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.
@github-actions github-actions bot removed the tools label Feb 28, 2023
@vivien-apple vivien-apple changed the title Yaml tests constraint has value Feb 28, 2023
tehampson
tehampson reviewed Feb 28, 2023
View reviewed changes
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, add type hinting value as ConstraintValue
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()]
Copy link
Contributor

Choose a reason for hiding this comment

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

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
scripts/tests/chiptest/__init__.py
@@ -125,6 +125,13 @@ def _GetSlowTests() -> Set[str]:
}


def _GetPythonYamlOnlyTests() -> Set[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is currently making this a test that only works in python and not codegen?
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these PICS values changing?
@bzbarsky-apple
Copy link
Contributor

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
Copy link

stale bot commented Apr 30, 2023

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 stale bot added the stale Stale issue or PR label Apr 30, 2023
@bzbarsky-apple bzbarsky-apple removed the stale Stale issue or PR label May 2, 2023
@woody-apple woody-apple added this to the 1.2 milestone Jun 23, 2023
@stale
Copy link

stale bot commented Sep 17, 2023

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 stale bot added the stale Stale issue or PR label Sep 17, 2023
@bzbarsky-apple bzbarsky-apple removed the stale Stale issue or PR label Sep 18, 2023
@woody-apple woody-apple added this to the 1.3 Test Complete milestone Oct 19, 2023
@stale Stale
Copy link

stale bot commented Mar 13, 2024

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 stale bot added the stale Stale issue or PR label Mar 13, 2024
@bzbarsky-apple bzbarsky-apple removed the stale Stale issue or PR label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app controller darwin examples scripts tests
4 participants
@vivien-apple @bzbarsky-apple @tehampson @woody-apple