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.

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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4211,6 +4211,7 @@ server cluster UnitTesting = 4294048773 {
attribute boolean timedWriteBoolean = 48;
attribute boolean generalErrorBoolean = 49;
attribute boolean clusterErrorBoolean = 50;
attribute NullablesAndOptionalsStruct nullablesAndOptionalsStruct = 51;
attribute nullable boolean nullableBoolean = 16384;
attribute nullable Bitmap8MaskMap nullableBitmap8 = 16385;
attribute nullable Bitmap16MaskMap nullableBitmap16 = 16386;
Expand Down Expand Up @@ -5340,6 +5341,7 @@ endpoint 1 {
ram attribute timedWriteBoolean;
callback attribute generalErrorBoolean;
callback attribute clusterErrorBoolean;
callback attribute nullablesAndOptionalsStruct;
ram attribute nullableBoolean;
ram attribute nullableBitmap8;
ram attribute nullableBitmap16;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20898,6 +20898,22 @@
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "nullables_and_optionals_struct",
"code": 51,
"mfgCode": null,
"side": "server",
"type": "NullablesAndOptionalsStruct",
"included": 1,
"storageOption": "External",
"singleton": 0,
"bounded": 0,
"defaultValue": "",
"reportable": 1,
"minInterval": 1,
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "unsupported",
"code": 255,
Expand Down Expand Up @@ -25499,4 +25515,4 @@
"deviceIdentifier": 61442
}
]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3531,6 +3531,7 @@ server cluster UnitTesting = 4294048773 {
attribute boolean timedWriteBoolean = 48;
attribute boolean generalErrorBoolean = 49;
attribute boolean clusterErrorBoolean = 50;
attribute NullablesAndOptionalsStruct nullablesAndOptionalsStruct = 51;
attribute nullable boolean nullableBoolean = 16384;
attribute nullable Bitmap8MaskMap nullableBitmap8 = 16385;
attribute nullable Bitmap16MaskMap nullableBitmap16 = 16386;
Expand Down Expand Up @@ -4297,6 +4298,7 @@ endpoint 1 {
ram attribute timedWriteBoolean;
callback attribute generalErrorBoolean;
callback attribute clusterErrorBoolean;
callback attribute nullablesAndOptionalsStruct;
ram attribute nullableBoolean;
ram attribute nullableBitmap8;
ram attribute nullableBitmap16;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20368,6 +20368,22 @@
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "nullables_and_optionals_struct",
"code": 51,
"mfgCode": null,
"side": "server",
"type": "NullablesAndOptionalsStruct",
"included": 1,
"storageOption": "External",
"singleton": 0,
"bounded": 0,
"defaultValue": "",
"reportable": 1,
"minInterval": 1,
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "unsupported",
"code": 255,
Expand Down Expand Up @@ -24953,4 +24969,4 @@
"deviceIdentifier": 61442
}
]
}
}
26 changes: 23 additions & 3 deletions scripts/py_matter_yamltests/matter_yamltests/constraints.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,22 @@ def __init__(self, context, reason):
super().__init__(context, 'notValue', reason)


class ConstraintValue:
def __init__(self, *args):
if len(args):
self._value = args[0]
self._has_value = True
else:
self._value = None
self._has_value = False

def get(self):
return self._value

def has_value(self) -> bool:
return self._has_value


class BaseConstraint(ABC):
'''Constraint Interface'''

Expand All @@ -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

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

value = value.get()
if value is None and self._is_null_allowed:
return

Expand Down Expand Up @@ -202,12 +223,11 @@ def validate(self, value, value_type_name):
if self.check_response(value, value_type_name):
return

reason = self.get_reason(value, value_type_name)
reason = self.get_reason(value.get(), value_type_name)
raise ConstraintHasValueError(self._context, reason)

def check_response(self, value, value_type_name) -> bool:
has_value = value is not None
return self._has_value == has_value
return self._has_value == value.has_value()

def get_reason(self, value, value_type_name) -> str:
if self._has_value:
Expand Down
23 changes: 15 additions & 8 deletions scripts/py_matter_yamltests/matter_yamltests/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from enum import Enum, auto

from . import fixes
from .constraints import get_constraints, is_typed_constraint
from .constraints import ConstraintValue, get_constraints, is_typed_constraint
from .definitions import SpecDefinitions
from .errors import TestStepError, TestStepKeyError, TestStepValueNameError
from .pics_checker import PICSChecker
Expand Down Expand Up @@ -738,7 +738,7 @@ def _response_values_validation(self, expected_response, received_response, resu
break

received_value = received_response.get('value')
if not self.is_attribute and not self.is_event:
if isinstance(value, dict) and 'name' in value:
expected_name = value.get('name')
if expected_name not in received_value:
result.error(check_type, error_name_does_not_exist.format(
Expand Down Expand Up @@ -785,14 +785,15 @@ def _response_constraints_validation(self, expected_response, received_response,
if 'constraints' not in value:
continue

received_value = received_response.get('value')
if not self.is_attribute and not self.is_event:
expected_name = 'value'
if isinstance(value, dict) and 'name' in value:
received_value = received_response.get(expected_name)
expected_name = value.get('name')
if received_value is None or expected_name not in received_value:
received_value = None
if expected_name in received_value:
received_value = ConstraintValue(received_value.get(
expected_name) if received_value else None)
else:
received_value = received_value.get(
expected_name) if received_value else None
received_value = ConstraintValue()

if self._test.response_mapping:
response_type_name = self._test.response_mapping.get(
Expand All @@ -802,6 +803,12 @@ def _response_constraints_validation(self, expected_response, received_response,
# If there is a constraint check for the type it is likely an incorrect
# constraint check by the test writter.
response_type_name = None
else:
if expected_name in received_response:
received_value = ConstraintValue(
received_response.get(expected_name))
else:
received_value = ConstraintValue()

constraints = get_constraints(value['constraints'])

Expand Down
27 changes: 27 additions & 0 deletions scripts/tests/chiptest/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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?

"""List of tests that does not have a codegen version."""
return {
"TestConstraintHasValue",
}


def _GetInDevelopmentTests() -> Set[str]:
"""Tests that fail in YAML for some reason.

Expand Down Expand Up @@ -191,6 +198,7 @@ def _hardcoded_python_yaml_tests():
manual_tests = _GetManualTests()
flaky_tests = _GetFlakyTests()
slow_tests = _GetSlowTests()
python_yaml_only_tests = _GetPythonYamlOnlyTests()
in_development_tests = _GetInDevelopmentTests()

for path in _AllYamlTests():
Expand All @@ -207,6 +215,9 @@ def _hardcoded_python_yaml_tests():
if path.name in slow_tests:
tags.add(TestTag.SLOW)

if path.name in python_yaml_only_tests:
tags.add(TestTag.PYTHON_YAML_ONLY)

if path.name in in_development_tests:
tags.add(TestTag.IN_DEVELOPMENT)

Expand All @@ -223,6 +234,22 @@ def AllYamlTests():
yield test


def AllChipToolPythonTests(chip_tool: str):
for test in tests_with_command(chip_tool, is_manual=False):
yield test

for test in tests_with_command(chip_tool, is_manual=True):
yield test

for test in _GetPythonYamlOnlyTests():
yield TestDefinition(
run_name=test,
name=test,
target=target_for_name(test),
tags=set([TestTag.PYTHON_YAML_ONLY])
)


def AllChipToolTests(chip_tool: str):
for test in tests_with_command(chip_tool, is_manual=False):
yield test
Expand Down
1 change: 1 addition & 0 deletions scripts/tests/chiptest/test_definition.py
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ class TestTag(Enum):
MANUAL = auto() # requires manual input. Generally not run automatically
SLOW = auto() # test uses Sleep and is generally slow (>=10s is a typical threshold)
FLAKY = auto() # test is considered flaky (usually a bug/time dependent issue)
PYTHON_YAML_ONLY = auto() # test does not have a codegen version
IN_DEVELOPMENT = auto() # test may not pass or undergoes changes

def to_s(self):
Expand Down
2 changes: 2 additions & 0 deletions scripts/tests/run_test_suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

elif runtime == TestRunTime.CHIP_TOOL_PYTHON:
all_tests = [test for test in chiptest.AllChipToolPythonTests(chip_tool)]
else:
all_tests = [test for test in chiptest.AllChipToolTests(chip_tool)]

Expand Down
25 changes: 25 additions & 0 deletions src/app/clusters/test-cluster-server/test-cluster-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ class TestAttrAccess : public AttributeAccessInterface
CHIP_ERROR WriteNullableStruct(AttributeValueDecoder & aDecoder);
CHIP_ERROR ReadListFabricScopedAttribute(AttributeValueEncoder & aEncoder);
CHIP_ERROR WriteListFabricScopedAttribute(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder);
CHIP_ERROR ReadNullablesAndOptionalsStructAttribute(AttributeValueEncoder & aEncoder);
CHIP_ERROR WriteNullablesAndOptionalsStructAttribute(AttributeValueDecoder & aDecoder);
};

TestAttrAccess gAttrAccess;
Expand Down Expand Up @@ -137,6 +139,8 @@ SimpleEnum gSimpleEnums[kAttributeListLength];
size_t gSimpleEnumCount = 0;
Structs::NullablesAndOptionalsStruct::Type gNullablesAndOptionalsStruct;

Structs::NullablesAndOptionalsStruct::Type gNullablesAndOptionalsStructNotInList;

CHIP_ERROR TestAttrAccess::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
switch (aPath.mAttributeId)
Expand Down Expand Up @@ -174,6 +178,9 @@ CHIP_ERROR TestAttrAccess::Read(const ConcreteReadAttributePath & aPath, Attribu
case WriteOnlyInt8u::Id: {
return StatusIB(Protocols::InteractionModel::Status::UnsupportedRead).ToChipError();
}
case NullablesAndOptionalsStruct::Id: {
return ReadNullablesAndOptionalsStructAttribute(aEncoder);
}
default: {
break;
}
Expand Down Expand Up @@ -216,6 +223,9 @@ CHIP_ERROR TestAttrAccess::Write(const ConcreteDataAttributePath & aPath, Attrib
case ClusterErrorBoolean::Id: {
return StatusIB(Protocols::InteractionModel::Status::Failure, 17).ToChipError();
}
case NullablesAndOptionalsStruct::Id: {
return WriteNullablesAndOptionalsStructAttribute(aDecoder);
}
default: {
break;
}
Expand Down Expand Up @@ -524,6 +534,21 @@ CHIP_ERROR TestAttrAccess::WriteListNullablesAndOptionalsStructAttribute(const C
return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
}

CHIP_ERROR TestAttrAccess::ReadNullablesAndOptionalsStructAttribute(AttributeValueEncoder & aEncoder)
{
return aEncoder.Encode(gNullablesAndOptionalsStructNotInList);
}

CHIP_ERROR TestAttrAccess::WriteNullablesAndOptionalsStructAttribute(AttributeValueDecoder & aDecoder)
{
// We only support setting the NullableOptionalInt field for now.
Structs::NullablesAndOptionalsStruct::DecodableType temp;
ReturnErrorOnFailure(aDecoder.Decode(temp));

gNullablesAndOptionalsStructNotInList.nullableOptionalInt = temp.nullableOptionalInt;
return CHIP_NO_ERROR;
}

CHIP_ERROR TestAttrAccess::ReadStructAttribute(AttributeValueEncoder & aEncoder)
{
return aEncoder.Encode(gStructAttributeValue);
Expand Down
Loading