Improve coverage with Cypress E2E tests and split test setup by mode (Zigbee/Matter/Multiprotocol)#1680
Conversation
Summary of ChangesHello @tbrkollar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive overhaul of the project's end-to-end testing strategy. It dramatically expands the existing Cypress test suite to cover a wider array of UI components and application flows, ensuring greater stability and reliability. A key architectural change is the segregation of tests and fixtures into distinct modes (Zigbee, Matter, Multiprotocol), allowing for more targeted and efficient testing. Furthermore, improvements have been made to the test execution environment, such as safer coverage collection and better handling of unexpected runtime errors, contributing to a more robust development process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request significantly expands the Cypress end-to-end test suite by introducing new tests for various UI components and application functionalities, including config loading scenarios, endpoint duplication, and comprehensive tests for attributes, clusters, and commands in Zigbee mode. It also adds extensive UI tests for the main layout, toolbar, extensions, notifications, options, and preferences pages, along with a dedicated 404 error page test. The cypress.config.js was updated to support dynamic test execution based on protocol mode (Zigbee, Matter, Multiprotocol) and conditional code coverage, and a new cypress/README.md was added to document the expanded test organization. Additionally, a goBackButton custom command was introduced, and data-cy attributes were added to various UI elements to improve test robustness. Review comments highlighted the need to enable skipped tests, fix a typo in cypress/README.md, improve selector robustness in several UI tests by using data-cy attributes, clarify a comment in tour.cy.js, and refactor repetitive endpoint cleanup logic into a custom command.
| it( | ||
| 'edit endpoint -> Overwrite', | ||
| 'edit endpoint -> delete and add', | ||
| { retries: { runMode: 2, openMode: 2 } }, | ||
| () => { | ||
| cy.get('[data-test="edit-endpoint"]').last().click() | ||
|
|
||
| cy.fixture('data').then((data) => { | ||
| cy.get('[data-test="select-endpoint-input"]') | ||
| .click() | ||
| .type(data.endpoint2.substring(0, 5), { force: true }) | ||
| .type(data.endpoint3.substring(0, 5), { force: true }) | ||
| cy.wait(500) | ||
| cy.get('div').contains(data.endpoint2).click({ force: true }) | ||
| cy.get('div').contains(data.endpoint3).click({ force: true }) | ||
| }) | ||
| cy.get('[data-test="endpoint-title"]').click() // it makes sure that the previous input field has been unselected | ||
| cy.get('button').contains('Save').click() | ||
| cy.wait(500) | ||
| // Test overwrite option | ||
| cy.get('button').contains('Overwrite').click() | ||
| // Test delete and add option | ||
| cy.get('button').contains('Delete and Add').click() | ||
| // Verify the endpoint was updated successfully | ||
| cy.fixture('data').then((data) => { | ||
| cy.get('aside').children().should('contain', data.endpoint2) | ||
| cy.get('aside').children().should('contain', data.endpoint3) | ||
| }) | ||
| cy.get('[data-test="delete-endpoint"]').each(() => { | ||
| cy.get('[data-test="delete-endpoint"]').last().click() | ||
| cy.wait(300) | ||
| cy.get('#delete_endpoint').click() | ||
| }) | ||
| cy.wait(300) | ||
| cy.get('#delete_last_endpoint').click() | ||
| } | ||
| ) | ||
| it('create a new endpoint', () => { | ||
| // Verify the endpoint was created successfully | ||
| cy.fixture('data').then((data) => { | ||
| cy.get('aside').children().should('contain', data.endpoint1) | ||
| }) | ||
| }) | ||
| it( | ||
| 'edit endpoint -> delete and add', | ||
| 'edit endpoint -> Overwrite', | ||
| { retries: { runMode: 2, openMode: 2 } }, | ||
| () => { | ||
| cy.get('[data-test="edit-endpoint"]').last().click() | ||
|
|
||
| cy.fixture('data').then((data) => { | ||
| cy.get('[data-test="select-endpoint-input"]') | ||
| .click() | ||
| .type(data.endpoint3.substring(0, 5), { force: true }) | ||
| .type(data.endpoint2.substring(0, 5), { force: true }) | ||
| cy.wait(500) | ||
| cy.get('div').contains(data.endpoint3).click({ force: true }) | ||
| cy.get('div').contains(data.endpoint2).click({ force: true }) | ||
| }) | ||
| cy.get('[data-test="endpoint-title"]').click() // it makes sure that the previous input field has been unselected | ||
| cy.get('button').contains('Save').click() | ||
| cy.wait(500) | ||
| // Test delete and add option | ||
| cy.get('button').contains('Delete and Add').click() | ||
| // Test overwrite option | ||
| cy.get('button').contains('Overwrite').click() | ||
| // Verify the endpoint was updated successfully | ||
| cy.fixture('data').then((data) => { | ||
| cy.get('aside').children().should('contain', data.endpoint3) | ||
| cy.get('aside').children().should('contain', data.endpoint2) | ||
| }) | ||
| } | ||
| ) |
There was a problem hiding this comment.
The order of the tests edit endpoint -> delete and add and edit endpoint -> Overwrite has been swapped compared to the original file. While not strictly an error, maintaining a consistent or logical flow (e.g., create, then overwrite, then delete) can improve test readability and maintainability. Consider reordering them to match the original logical flow if possible.
| return false | ||
| } | ||
| // Don't prevent other errors from failing the test | ||
| return true |
There was a problem hiding this comment.
The comment // Don't prevent other errors from failing the test is slightly misleading. The return true in the else block means that if an uncaught exception not matching the undefined.map() condition occurs, Cypress will fail the test, which is generally desired behavior. The global Cypress.on('uncaught:exception', (err, runnable) => { return false }) already prevents all uncaught exceptions from failing tests. This specific beforeEach handler is more granular. Clarify the comment to reflect that only specific errors are ignored, and others will still cause a failure.
// Don't prevent other errors from being ignored by this specific handler
// (they will still be caught by the global handler if present, or fail the test otherwise)
return true| cy.get('body').then(($body) => { | ||
| if ($body.find('[data-test="delete-endpoint"]').length > 0) { | ||
| // Delete all endpoints except the last one using each() | ||
| cy.get('[data-test="delete-endpoint"]').each(() => { | ||
| cy.get('[data-test="delete-endpoint"]').last().click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('#delete_endpoint').should('exist').click({ force: true }) | ||
| cy.wait(500) | ||
| }) | ||
| // Delete the last endpoint (which shows different dialog) | ||
| cy.wait(300) | ||
| cy.get('[data-test="delete-endpoint"]').then(($remaining) => { | ||
| if ($remaining.length > 0) { | ||
| cy.get('[data-test="delete-endpoint"]') | ||
| .last() | ||
| .click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('#delete_last_endpoint') | ||
| .should('exist') | ||
| .click({ force: true }) | ||
| cy.wait(500) | ||
| } | ||
| }) | ||
| } | ||
| }) |
| cy.addEndpoint(data.endpoint4) | ||
|
|
||
| // Use search functionality | ||
| cy.get('input[placeholder*="Search"], input[type="search"]').then( |
There was a problem hiding this comment.
| cy.get('body').then(($body) => { | ||
| if ($body.find('[data-test="delete-endpoint"]').length > 0) { | ||
| // Delete all endpoints except the last one using each() | ||
| cy.get('[data-test="delete-endpoint"]').each(() => { | ||
| cy.get('[data-test="delete-endpoint"]').last().click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('#delete_endpoint').should('exist').click({ force: true }) | ||
| cy.wait(500) | ||
| }) | ||
| // Delete the last endpoint (which shows different dialog) | ||
| cy.wait(300) | ||
| cy.get('[data-test="delete-endpoint"]').then(($remaining) => { | ||
| if ($remaining.length > 0) { | ||
| cy.get('[data-test="delete-endpoint"]') | ||
| .last() | ||
| .click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('#delete_last_endpoint') | ||
| .should('exist') | ||
| .click({ force: true }) | ||
| cy.wait(500) | ||
| } | ||
| }) | ||
| } | ||
| }) |
|
|
||
| // Search for commands | ||
| cy.get('input[placeholder*="Search"], input[type="search"]').then( | ||
| ($inputs) => { |
There was a problem hiding this comment.
| cy.get('body').then(($body) => { | ||
| if ($body.find('[data-test="delete-endpoint"]').length > 0) { | ||
| // Delete all endpoints except the last one using each() | ||
| cy.get('[data-test="delete-endpoint"]').each(() => { | ||
| cy.get('[data-test="delete-endpoint"]').last().click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('#delete_endpoint').should('exist').click({ force: true }) | ||
| cy.wait(500) | ||
| }) | ||
| // Delete the last endpoint (which shows different dialog) | ||
| cy.wait(300) | ||
| cy.get('[data-test="delete-endpoint"]').then(($remaining) => { | ||
| if ($remaining.length > 0) { | ||
| cy.get('[data-test="delete-endpoint"]') | ||
| .last() | ||
| .click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('#delete_last_endpoint') | ||
| .should('exist') | ||
| .click({ force: true }) | ||
| cy.wait(500) | ||
| } | ||
| }) | ||
| } | ||
| }) |
paulr34
left a comment
There was a problem hiding this comment.
This looks great overall. For the typo fixes Gemini suggested, it would probably be best to use “Commit suggestion” so those get applied cleanly.
Since this PR is changing the Cypress tests themselves, it would also be good to run the app manually and verify the behavior. Even if the changes are correct, there’s some inherent risk when modifying tests, so a quick manual check helps confirm everything is behaving as expected.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
brdandu
left a comment
There was a problem hiding this comment.
Changes look fine to me. The code review suggestions do make sense so please resolve them and lets get this merged in for better code coverage
| describe('Scenario 1: Zigbee + Matter templates pre-loaded', () => { | ||
| it('Should verify Zigbee config loads correctly when both templates are pre-loaded', () => { | ||
| // This test runs in multiprotocol mode where both templates are pre-loaded | ||
| if (Cypress.env('mode') === 'multiprotocol') { |
There was a problem hiding this comment.
Avoid using strings across the code. Better to use predefined macros or create your own
There was a problem hiding this comment.
This test case will be in my next PR. It only works in multiprotocol mode, so I marked it as skipped.
There was a problem hiding this comment.
I’ve pushed a commit for this.
| }) | ||
| }) | ||
|
|
||
| it('create a new endpoint', () => { |
There was a problem hiding this comment.
it was duplicated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Centralize zigbee/matter/multiprotocol mode values in cypress/support/mode.js and
replace hard-coded Cypress.env('mode') string comparisons across specs and
cypress.config.js with Cypress.Mode.*.
Also fix Cypress zap-config spec warning assertions.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Improved overall code coverage by adding a large set of Cypress E2E tests (especially UI coverage).
Split the Cypress E2E setup by Zigbee, Matter, and Multiprotocol modes (separate specs + mode-specific fixtures).
Made coverage collection safer and more reliable by only enabling Cypress coverage hooks when coverage is explicitly turned on, and by combining Jest + Cypress coverage into one report with a simple threshold check.
#1428
#956