Declarative PIXITs in python tests #395 #36817
Conversation
|
Review changes with Changed Files
|
github-actions
bot
added
tests
matter-1.4-te2-script-change
| def validate_bool(value: Any) -> bool: | ||
| if isinstance(value, str): | ||
| value_lower = value.lower() | ||
| if value_lower not in ('true', 'false', '1', '0', 'yes', 'no'): |
There was a problem hiding this comment.
only true false supported
|
|
||
| @staticmethod | ||
| def validate_float(value: Any) -> bool: | ||
| float(value) |
There was a problem hiding this comment.
- value is
strfor all validators validate_xxx-->is_xxx_pixit_value_valid
|
|
||
| @staticmethod | ||
| def validate_string(value: Any) -> bool: | ||
| str(value) |
There was a problem hiding this comment.
For all these you need to wrap the possibly-failing conversion in a try: except ValueError before true/false returned.
Also, when returning False, you are losing the reason for why it failed validation, which would have come out fo the exception.
Recommend moving all of these validators to raise exceptions rather than be bool return. In that case, it could remain validate_xxx
There was a problem hiding this comment.
fixed and converted bool returns to exceptions
| return True | ||
|
|
||
| @classmethod | ||
| def validate_value(cls, value: Any, pixit_def: PIXITDefinition) -> tuple[bool, Optional[str]]: |
There was a problem hiding this comment.
Avoid raw tuple returns. Raise an exception with message on failure
There was a problem hiding this comment.
Thank you for this, I was afraid that with using tuple there readability degrades a bit. Removed tuple return
| if missing or invalid: | ||
| error = "" | ||
| if missing: | ||
| error += "\nMissing required PIXITs:\n" + "\n".join(missing) |
There was a problem hiding this comment.
Please avoid multi-line emssages
| default: Any = None | ||
|
|
||
| @staticmethod | ||
| def is_pixit(arg_name: str) -> bool: |
There was a problem hiding this comment.
actually, we probably want to validate all the flags. There are some flags that should be exposed that aren't called PIXIT.whatever
There was a problem hiding this comment.
Do you suggest to rename all methods to use some more generic words?
There was a problem hiding this comment.
Yeah. Naming is one of the hardest things, but I'm tempted to go with Parameter? I know you hate the generic names so @andy31415 - thoughts?
src/python_testing/matter_testing_infrastructure/chip/testing/matter_testing.py
Outdated
Show resolved
Hide resolved
|
|
||
| # Validate PIXITs before proceeding | ||
| validator = PIXITValidator() | ||
| valid, error = validator.validate_pixits(test_info[0].pixits, matter_test_config.global_test_params) |
There was a problem hiding this comment.
Does this validate only for the first test in the list?
might be better in the setup_test function for each test.
There was a problem hiding this comment.
Yeah, thank you for noticing this issue, I've moved validation to setup_test
| @@ -612,6 +612,142 @@ def test_skipped(self, filename: str, name: str): | |||
| logging.info(f"Skipping test from {filename}: {name}") | |||
|
|
|||
|
|
|||
| class PIXITType(Enum): | |||
| INT = "int" | |||
There was a problem hiding this comment.
This approach in general is OK, but I do want to consider this in the larger context of verifying that the test invocations are correct in a broader way. We can land this in stages, but it is important to consider how the other checks are going to be integrated here.
In particular, test authors need an easy way to declare certain other test requirements that also need verification. Ex. Endpoint project-chip/matter-test-scripts#450, which we should consider mandating with no default for tests that are run against a single endpoint, no --endpoint flag for tests that run against the entire node at once. Need a way to declare that a test requires a commissioned device, whether or not that commissioning happens as a part of the test invocation. Some tests require passcodes, discriminators etc, even for tests where the commissioning happens external to the test. Tests that DO use PICS (hopefully fewer and fewer) should have a PICS file specified (and this get extra difficult in the context of the test harness)
Anyway, worth thinking through how these extended checks would fit in with this design. I don't see anything in particular blocking these options right now.
It's also worth thinking through how to expose this properly to test authors. ie, is there a way that we can alert test authors that are using test flags that they should be providing these functions?
| @@ -612,6 +612,142 @@ def test_skipped(self, filename: str, name: str): | |||
| logging.info(f"Skipping test from {filename}: {name}") | |||
|
|
|||
|
|
|||
| class PIXITType(Enum): | |||
There was a problem hiding this comment.
I know I used PIXIT in the issue, but I think this applies generally to test flags. But I can't think of a great name for just "Flags" that doesn't sound too generic. Ideas welcome.
There was a problem hiding this comment.
To be honest, I also don't have many ideas. TestFlag? TestFlagType? TestParamType?
… validation would run only for first test
Co-authored-by: C Freeman <[email protected]>
|
PR #36817: Size comparison from b0d0614 to d097888 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| PIXITValidator.validate_hex_pixit_value("0x1234") # Should pass | ||
| PIXITValidator.validate_hex_pixit_value("1234") # Should pass | ||
| PIXITValidator.validate_hex_pixit_value("ABCD") # Should pass | ||
| PIXITValidator.validate_hex_pixit_value("abcd") # Should pass |
There was a problem hiding this comment.
one of the other formats we sometimes get is hex:12ab. I don't know if that is handled correctly in the argument parser though.
| default: Any = None | ||
|
|
||
| @staticmethod | ||
| def is_pixit(arg_name: str) -> bool: |
There was a problem hiding this comment.
Yeah. Naming is one of the hardest things, but I'm tempted to go with Parameter? I know you hate the generic names so @andy31415 - thoughts?
| ) | ||
|
|
||
| PIXITValidator.validate_value(None, optional_pixit) # Should pass as it's optional | ||
|
|
There was a problem hiding this comment.
Can you also please add a test for the validate_pixits function to ensure they get mapped properly?
project-chip/matter-test-scripts#395
Added a common mechanism for tests to declare their optional and required PIXIT values and override flags and ensure the required values are supplied. Add information about optional override flags with descriptions in the error outputs of tests.
Example: