-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add Unit Tests for Element Conformance Warnings in ZAP File Import #1518
base: master
Are you sure you want to change the base?
Add Unit Tests for Element Conformance Warnings in ZAP File Import #1518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but the messaging needs some work to make it easier for users to understand
// Their element conformance warnings should be added to the notification table. | ||
expect( | ||
notificationMessages.includes( | ||
'On endpoint 1, cluster: On/Off, command: OffWithEffect conforms to LT and is mandatory based on state of feature: LT.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message seems very confusing on what the user needs to(should) do. Change it to
On endpoint 1, cluster: On/Off, [incoming or outgoing] command: OffWithEffect needs to be enabled to ensure mandatory conformance with the enabled Lighting(LT) feature.
Or
'On endpoint 1, cluster: On/Off, [incoming or outgoing] command: OffWithEffect has mandatory conformance to Lighting(LT) feature and should be enabled when LT is enabled'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complicated conformance expressions have to be considered in the message, for example, LT | AB | element1
. I will improve the message as below to cover all cases:
'On endpoint 1, cluster: On/Off, [incoming or outgoing] command: OffWithEffect has mandatory conformance to LT | AB | element1
and should be enabled when feature LT
is enabled, feature AB
is disabled, and element element1
is disabled'.
).toBeTruthy() | ||
expect( | ||
notificationMessages.includes( | ||
'On endpoint 1, cluster: On/Off, attribute: OnTime conforms to LT and is mandatory based on state of feature: LT.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These also should be updated as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the console warnings are already thrown out as well apart from the UI ones
importexport.test.js
to validate that non-conforming elements trigger warnings in the notification table when importing a ZAP file.multi-protocol.test.js
to account for element conformance warnings.Note:
This PR added conformance data to ZAP XML for testing purpose, which will make the “Device Type Features” button visible when running ZAP locally. However, in older Matter SDK versions without conformance data, the button will not be visible.