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

return GUI data for selected endpoint and not just the first one in the list #1520

Merged
merged 9 commits into from
Feb 14, 2025

Conversation

paulr34
Copy link
Collaborator

@paulr34 paulr34 commented Feb 6, 2025

No description provided.

Copy link
Collaborator

@brdandu brdandu left a comment

Choose a reason for hiding this comment

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

Add unit tests for the issue this change is solving.

@paulr34 paulr34 force-pushed the bugFix_sharingZigbeeData branch from a47dde6 to 610545d Compare February 11, 2025 13:47
@paulr34 paulr34 requested a review from brdandu February 11, 2025 20:33
@paulr34 paulr34 force-pushed the bugFix_sharingZigbeeData branch from 22114a3 to b3174e1 Compare February 14, 2025 16:43
Copy link
Collaborator

@brdandu brdandu left a comment

Choose a reason for hiding this comment

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

I do not understand cypress that well so take a look at the comments and take appropriate actions.

cy.fixture('data').then((data) => {
this.data = data
// If mode is "matter", set skipTest flag to true
if (this.data.mode === 'matter') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to run it for zigbee than not matter. If a new protocol does not have attribute reporting then this test will fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure good idea

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, thank you


// Create and validate the first endpoint
cy.fixture('data').then((data) => {
cy.addEndpoint(data.endpoint4, data.cluster1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand this as I am not familiar with what is happening here. Comment says first endpoint and cy.addEndpoint(data.endpoint4, data.cluster1) says data.endpoint4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this is just how cypress is setup for all tests but I agree it should be fixed and cleaned up. I will work on a follow up PR to fix Cypress in general

'.q-virtual-scroll__content > :nth-child(1) > :nth-child(2) > .q-toggle > .q-toggle__inner > .q-toggle__thumb'
).click()
// Check to make sure the button is still visible after clicking
cy.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope this test does not just get the next item in the list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sorry I'm confused what you mean here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just don't really know how this checks in the UI when an attribute vanishes. If there are 2 attributes in the attribute reporting and one of them vanishes then does the second one just show up in the first one's place and pass this test?
Is it better to test by checking the number of attributes in attribute reporting before and after enablement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, it gets a specific one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't just get the first one in the list but I will triple check

'.q-virtual-scroll__content > :nth-child(1) > :nth-child(2) > .q-toggle > .q-toggle__inner > .q-toggle__thumb'
).click()
// Check to make sure the button is still visible after clicking
cy.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just don't really know how this checks in the UI when an attribute vanishes. If there are 2 attributes in the attribute reporting and one of them vanishes then does the second one just show up in the first one's place and pass this test?
Is it better to test by checking the number of attributes in attribute reporting before and after enablement?

Copy link
Collaborator

@ethanzhouyc ethanzhouyc left a comment

Choose a reason for hiding this comment

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

In the following PR to clean up Cypress test, we should rename these endpoints with their actual names. These random numbers are so confusing to read

"endpoint1": "Billing Unit (0x0203)",
 "endpoint2": "CBA Config Tool (0x0005)",
 "endpoint3": "Chatting Station (0x0601)",
 "endpoint4": "CBA Thermostat (0x0301)",

@paulr34
Copy link
Collaborator Author

paulr34 commented Feb 14, 2025

In the following PR to clean up Cypress test, we should rename these endpoints with their actual names. These random numbers are so confusing to read


"endpoint1": "Billing Unit (0x0203)",

 "endpoint2": "CBA Config Tool (0x0005)",

 "endpoint3": "Chatting Station (0x0601)",

 "endpoint4": "CBA Thermostat (0x0301)",

I agree. Good idea. I'll whip something up for next week

@paulr34 paulr34 merged commit 1b52e98 into project-chip:master Feb 14, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants