Fixed unstable requests regression test #8841
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request modifies the Cypress end-to-end tests for the requests page, focusing on simplifying notification handling and enhancing error resilience. The changes remove a specific function for closing export notifications and introduce a more direct approach to managing notifications. Additionally, the test suite now includes an improved error handling mechanism that simulates network errors and verifies the UI's response to request failures. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/cypress/e2e/features/requests_page.js (2)
350-352: Add explicit wait and specific notification targetingWhile the current implementation works, it could be more reliable by:
- Waiting for notifications to appear
- Specifically targeting export-related notifications
Consider this more robust approach:
-cy.get('.ant-notification-notice').each((notification) => { - cy.wrap(notification).find('span[aria-label="close"]').click(); -}); +cy.get('.ant-notification-notice') + .should('exist') + .contains('Export job') + .parents('.ant-notification-notice') + .each((notification) => { + cy.wrap(notification).find('span[aria-label="close"]').click(); + cy.wrap(notification).should('not.exist'); + });
Line range hint
354-374: LGTM! Consider adding more error scenariosThe error handling test is well implemented. It effectively verifies UI stability and error message display when the export request fails.
Consider adding tests for other HTTP error codes (e.g., 401, 403, 404) to ensure comprehensive error handling coverage. For example:
it('Export task. Request returns 403 forbidden', () => { cy.intercept('GET', '/api/requests/**', { statusCode: 403, body: 'Forbidden' }); // ... rest of the test });
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cypress/e2e/features/requests_page.js (1)
Line range hint
341-369: Enhance test coverage for request status verificationWhile the test covers basic success and error scenarios, it could be more thorough in verifying request states and metadata.
Consider adding these test cases:
it('Export job shows correct progress states and metadata', () => { // Intercept with delay to observe progress state cy.intercept('GET', '/api/requests/**', (req) => { req.on('response', (res) => { res.setDelay(2000); }); }); cy.getJobIDFromIdx(0).then((jobID) => { cy.exportJob({ type: 'dataset', format: 'CVAT for images', archiveCustomName: 'progress_test' }); // Verify request card shows "in progress" state cy.get(`.cvat-requests-card:contains("Job #${jobID}")`) .should('exist') .within(() => { cy.get('.cvat-request-item-progress-progress').should('exist'); cy.contains('Export Dataset').should('exist'); cy.contains('CVAT for images').should('exist'); // Verify request transitions to success cy.get('.cvat-request-item-progress-success', { timeout: 10000 }) .should('exist'); }); }); });This adds verification of:
- Request progress states
- Request type and format metadata
- State transitions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8841 +/- ##
========================================
Coverage 73.89% 73.90%
========================================
Files 409 409
Lines 43932 43932
Branches 3986 3986
========================================
+ Hits 32465 32467 +2
+ Misses 11467 11465 -2
|
|
| @@ -322,9 +322,11 @@ context('Requests page', () => { | |||
|
|
|||
| cy.getJobIDFromIdx(0).then((jobID) => { | |||
| const closeExportNotification = () => { | |||
There was a problem hiding this comment.
Why can't we use closeNotification custom command?
There was a problem hiding this comment.
We are not using default command for two reasons:
- It expects notification to have specific class (curretly notifications related to requests dont have specific assigned classes)
- Even if we add the classes, this test has a bit unique scenario as two notifications are expected. The command will fail as it expects that no notifications with provided class are present after closing operation.
Motivation and context
The regression test fails from time to time because notification of successful export is not closed. Probably because we use contains command which selects the same element twice
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
Chores