refactor(cypress): move hard-coded cypress test data to fixtures #4465
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of Cypress test files across multiple workflows. The primary change is the transition from hardcoded values to dynamic data sourced from JSON fixture files. Each test file in the Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
cypress/cypress/e2e/B-catalyst-workflow.cy.ts (1)
Line range hint
8-28: Updateitblocks to usefunctionsyntax for fixture data accessThe test cases are currently using arrow functions (
() => {}), which do not have their ownthiscontext. Since you're loading fixture data intothis.datain thebeforeEachhook, you need to usefunction ()syntax in theitblocks to accessthis.data.Apply this diff to fix the issue:
-it('Test login for catalyst users and non-catalyst users', () => { +it('Test login for catalyst users and non-catalyst users', function () { cy.loginLecturer() cy.wait(1000) cy.loginFreeUser() cy.wait(1000) cy.loginIndividualCatalyst() cy.wait(1000) cy.loginInstitutionalCatalyst() }) -it('Test that the creation buttons for practice quizzes and micro learnings are only available to catalyst users', () => { +it('Test that the creation buttons for practice quizzes and micro learnings are only available to catalyst users', function () { cy.loginLecturer() cy.wait(1000) cy.get('[data-cy="create-practice-quiz"]').should('not.be.disabled') cy.get('[data-cy="create-microlearning"]').should('not.be.disabled') cy.loginFreeUser() cy.wait(1000) cy.get('[data-cy="create-practice-quiz"]').should('be.disabled') cy.get('[data-cy="create-microlearning"]').should('be.disabled') cy.loginIndividualCatalyst() cy.wait(1000) cy.get('[data-cy="create-practice-quiz"]').should('not.be.disabled') cy.get('[data-cy="create-microlearning"]').should('not.be.disabled') cy.loginInstitutionalCatalyst() cy.wait(1000) cy.get('[data-cy="create-practice-quiz"]').should('not.be.disabled') cy.get('[data-cy="create-microlearning"]').should('not.be.disabled') })cypress/cypress/e2e/G-microlearning-workflow.cy.ts (1)
Line range hint
620-637: Potential issue withthiscontext insidethencallbacksInside the
thencallback ofcy.task('getMicroLearningInfo'), thethiscontext may not be preserved. Capturethisin a variable before the callback to accessthis.datasafely.Apply this diff:
it('Check if the drafted microlearning can be accessed by the lecturer through the activity preview', function () { + const self = this; cy.task('getMicroLearningInfo', { mlName: self.data.running.nameNew }).then( (quiz: { id: string; courseId: string }) => { // check if the query was successful if (quiz === null) { throw new Error('Microlearning not found') } // visit the activity preview with the manager cookie being active cy.visit( `${Cypress.env('URL_STUDENT')}/course/${quiz.courseId}/microlearning/${quiz.id}` ) // verify that the microlearning can be answered through the activity preview answerMicroLearningPreview() } ) })
♻️ Duplicate comments (2)
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (1)
620-637:⚠️ Potential issueReiteration of potential issue with
thiscontext insidethencallbacksSimilar to the earlier occurrence,
this.datainside thethencallback may not be accessible. Capturethisin a variable before the callback to ensurethis.datais available.cypress/cypress/e2e/G-microlearning-workflow.cy.ts (1)
1123-1127:⚠️ Potential issuePotential issue with Cypress commands inside
forEachloopsUsing Cypress commands inside a
forEachloop can be problematic. Refactor usingcy.wrapand.each()to ensure proper execution.
🧹 Nitpick comments (14)
cypress/cypress/e2e/D-questions-workflow.cy.ts (1)
Line range hint
20-1497: Refactor repetitive test code into reusable helper functionsThe test cases for creating and editing different question types contain similar code patterns. Refactoring these patterns into reusable helper functions would reduce code duplication and improve maintainability.
cypress/cypress/e2e/C-control-workflow.cy.ts (1)
13-14: Consider adding test data validation.While the fixture data is loaded correctly, consider adding validation to ensure required test data properties exist.
beforeEach('Load fixture for this test case', function () { cy.fixture('C-control.json').then((data) => { + // Validate required test data + const requiredProps = ['questionTitle', 'questionContent', 'quizName', 'quizDisplayName'] + requiredProps.forEach(prop => { + expect(data).to.have.property(prop, `Missing required property: ${prop}`) + }) this.data = data }) })cypress/cypress/fixtures/I-bookmarking.json (2)
7-9: Consider using more descriptive choice content.The current choice content ("50%", "100%", "75%") lacks context. Consider using more descriptive content that reflects real-world scenarios.
- { "content": "50%", "correct": true }, - { "content": "100%" }, - { "content": "75%" } + { "content": "Half of the maximum score", "correct": true }, + { "content": "Full score" }, + { "content": "Three quarters of the score" }
24-25: Simplify flag messages for better maintainability.The flag messages are verbose and contain redundant information. Consider simplifying them while maintaining clarity.
- "flag1": "Test flagging question on practice quiz Bookmarking practice quiz", - "flag2": "Test flagging question on practice quiz Bookmarking practice quiz new" + "flag1": "Practice quiz - Initial flag", + "flag2": "Practice quiz - Updated flag"- "flag1": "Test flagging question on microlearning Bookmarking microlearning", - "flag2": "Test flagging question on microlearning Bookmarking microlearning new" + "flag1": "Microlearning - Initial flag", + "flag2": "Microlearning - Updated flag"Also applies to: 30-31
cypress/cypress/fixtures/H-practice-quiz.json (2)
44-48: Use consistent number formatting in NR options.The
minandmaxvalues are strings whileaccuracyis a number in the NR options. Consider using consistent number formatting."min": "0", "max": "100", "unit": "%", - "accuracy": "2", + "accuracy": 2,
5-5: Use more meaningful question titles.The current question titles (e.g., "SC q9f8w3h2", "MC p2n6v8j1") appear to be random strings. Consider using more descriptive titles that reflect the question's purpose or content.
- "title": "SC q9f8w3h2", + "title": "Basic Single Choice Question",Also applies to: 15-15, 20-20, 31-31, 41-41, 55-55, 62-62, 67-67
cypress/cypress/fixtures/G-microlearning.json (1)
67-72: Simplify display name patterns.The display names follow a repetitive pattern of appending " (Display)" or " (NEW!)". Consider using a more maintainable approach by storing only the base name and constructing display names programmatically in the tests.
This could be implemented by adding a helper function in your test utilities to generate display names consistently.
Also applies to: 95-97
cypress/cypress/fixtures/F-live-quiz.json (2)
2-87: Question data structure needs type validation.The question data structure is well-organized but lacks explicit type definitions. Consider adding a
typefield to each question object to make the data structure more self-documenting and robust.Example for SC1:
"SC1": { + "type": "SINGLE_CHOICE", "title": "SC Title LQ Test 1",
88-103: Improve course1 quiz data organization.The quiz configuration mixes different concerns (basic info, points, duplicated data). Consider restructuring for better maintainability.
"course1": { "name": "Testkurs", "quiz": { + "basic": { "name": "Live Quiz 1", "displayName": "Live Quiz 1 (Display)", "description": "Live Quiz 1 Description", + }, + "updated": { "nameNew": "Live Quiz 1 NEW", "displayNameNew": "Live Quiz 1 (Display) NEW", "descriptionNew": "Live Quiz 1 Description NEW", "nameDupl": "Live Quiz 1 NEW (Copy)", + }, + "scoring": { "defaultPoints": 50, "defaultCorrectPoints": 100, "maxBonusPoints": 200, "timeToZeroBonus": 100 + } } }cypress/cypress/fixtures/J-group-activity.json (1)
112-130: Improve grading comments structure.The current structure with null values and separate arrays for comments makes the data harder to maintain.
Consider restructuring to combine related data:
-"comments1": [ - "Great job at question 1!", - null, - null, - "Great job at question 4!", - "Good job at question 5!", - null -], -"comments2": [ - "This is not correct for question 1...", - null, - "This is not correct for question 3...", - null, - null, - "This is not correct for question 6..." -], +"comments": [ + { + "question": 1, + "firstReview": "Great job at question 1!", + "secondReview": "This is not correct for question 1..." + }, + { + "question": 4, + "firstReview": "Great job at question 4!", + "secondReview": null + } +]cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (1)
569-572: Avoid using arbitrarycy.wait()commandsUsing hard-coded waits like
cy.wait(2000)can lead to flaky tests. Instead, use commands likecy.get().should()to wait for elements to reach the desired state.Also applies to: 771-773
cypress/cypress/e2e/G-microlearning-workflow.cy.ts (1)
656-656: Avoid using arbitrarycy.wait()commandsHard-coded waits like
cy.wait(2000)can make tests flaky. Use Cypress commands to wait for specific conditions instead.Also applies to: 738-738
cypress/cypress/e2e/F-live-quiz-workflow.cy.ts (1)
753-771: Avoid using arbitrarycy.wait()commandsUsing
cy.wait()with hard-coded durations can lead to unreliable tests. Replace with commands that wait for specific conditions or elements.Also applies to: 779-779, 787-787
cypress/cypress/e2e/E-course-workflow.cy.ts (1)
24-30: LGTM! Well-structured test data management.The implementation consistently uses fixture data across various test scenarios, improving maintainability. Consider adding TypeScript interfaces for the fixture data structure to improve type safety and IDE support.
Example interface structure:
interface CourseFixture { course1: { name: string; displayName: string; displayNameNew: string; description: string; }; course2: { name: string; displayName: string; group1: string; group2: string; }; // ... other properties }Also applies to: 92-95, 235-237, 250-252, 525-528
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
cypress/cypress/e2e/A-login-workflow.cy.ts(4 hunks)cypress/cypress/e2e/B-catalyst-workflow.cy.ts(1 hunks)cypress/cypress/e2e/C-control-workflow.cy.ts(3 hunks)cypress/cypress/e2e/D-questions-workflow.cy.ts(26 hunks)cypress/cypress/e2e/E-course-workflow.cy.ts(13 hunks)cypress/cypress/e2e/F-live-quiz-workflow.cy.ts(24 hunks)cypress/cypress/e2e/G-microlearning-workflow.cy.ts(16 hunks)cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts(10 hunks)cypress/cypress/e2e/I-bookmarking-workflow.cy.ts(3 hunks)cypress/cypress/e2e/J-group-activity-workflow.cy.ts(21 hunks)cypress/cypress/fixtures/A-login.json(1 hunks)cypress/cypress/fixtures/B-catalyst.json(1 hunks)cypress/cypress/fixtures/C-control.json(1 hunks)cypress/cypress/fixtures/D-questions.json(1 hunks)cypress/cypress/fixtures/E-course.json(1 hunks)cypress/cypress/fixtures/F-live-quiz.json(1 hunks)cypress/cypress/fixtures/G-microlearning.json(1 hunks)cypress/cypress/fixtures/H-practice-quiz.json(1 hunks)cypress/cypress/fixtures/I-bookmarking.json(1 hunks)cypress/cypress/fixtures/J-group-activity.json(1 hunks)cypress/cypress/fixtures/example.json(0 hunks)cypress/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- cypress/cypress/fixtures/example.json
✅ Files skipped from review due to trivial changes (4)
- cypress/cypress/fixtures/B-catalyst.json
- cypress/cypress/fixtures/C-control.json
- cypress/cypress/fixtures/A-login.json
- cypress/cypress/fixtures/E-course.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: SonarCloud
🔇 Additional comments (17)
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (2)
14-19: Good use ofbeforeEachhook and function syntax for fixture data accessThe introduction of the
beforeEachhook to load fixture data enhances the maintainability of the tests. Usingfunction ()instead of arrow functions ensures thatthis.datais accessible within the test cases, following best practices in Cypress.
Line range hint
22-1264: Consistent replacement of hardcoded values with fixture dataReplacing hardcoded constants with
this.datareferences throughout the test cases improves flexibility and scalability. This change aligns with the PR objectives and enhances the robustness of the tests.cypress/cypress/e2e/C-control-workflow.cy.ts (1)
1-6: LGTM! Good use of function syntax for properthisbinding.The
beforeEachhook correctly loads the fixture data, and the test suite uses function syntax instead of arrow functions to ensure properthisbinding for accessing fixture data.cypress/package.json (1)
19-19: LGTM! The new script enhances test execution flexibility.The addition of
test:run:onescript allows running individual test specs, which is useful for testing specific workflows after the refactoring to use fixture data.cypress/cypress/fixtures/D-questions.json (1)
64-68: Consider edge cases in solution ranges.The solution ranges include
nullvalues which might need validation in the application code. Consider adding validation or documenting the expected behavior.cypress/cypress/fixtures/F-live-quiz.json (1)
61-70: Ensure consistent data types for numeric values.In the NR2 options, numeric values are stored as strings. This might cause type coercion issues in the tests.
Consider using numeric types instead:
"options": { - "min": "0", - "max": "100", + "min": 0, + "max": 100, "unit": "%", - "accuracy": "2", + "accuracy": 2, "solutionRanges": [ - { "min": "0", "max": "25" }, - { "min": "75", "max": "100" } + { "min": 0, "max": 25 }, + { "min": 75, "max": 100 } ] },cypress/cypress/fixtures/J-group-activity.json (1)
33-42: Inconsistent numeric value types in NR options.Similar to F-live-quiz.json, numeric values are stored as strings in the numerical response options.
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (2)
Line range hint
712-727: Reiteration of potential issue withthiscontext insidethencallbacksAs in previous instances,
this.datamay not be accessible inside thethencallback ofcy.task('getPracticeQuizInfo'). Consider capturingthisin a variable.
Line range hint
1110-1128: Reiteration of issue with Cypress commands insideforEachloopsAs previously noted, using Cypress commands within a
forEachloop can cause issues. Refactor usingcy.wrapand.each()to ensure proper command execution.cypress/cypress/e2e/G-microlearning-workflow.cy.ts (1)
Line range hint
896-914: Reiteration of issue withthiscontext insidethencallbacks
this.datamay not be accessible inside thethencallback. Capturethisin a variable before the callback.cypress/cypress/e2e/F-live-quiz-workflow.cy.ts (1)
Line range hint
980-982: Ensure proper use ofthiscontext inside callbacksWhen using
thencallbacks,thismay not be preserved. Although not currently usingthisinside these callbacks, be cautious if you plan to accessthis.datawithin them.Also applies to: 1000-1002, 1004-1006
cypress/cypress/e2e/A-login-workflow.cy.ts (3)
5-9: LGTM! Well-structured fixture loading.The beforeEach hook correctly uses function() syntax to preserve the test context and follows Cypress best practices.
148-148: LGTM! Good use of fixture data.The test correctly uses fixture data for the new password value, maintaining consistency with the PR's objective.
Also applies to: 162-165
176-176: LGTM! Consistent use of fixture data.The password verification step maintains consistency by using the same fixture data.
cypress/cypress/e2e/I-bookmarking-workflow.cy.ts (2)
3-6: LGTM! Smart use of dynamic dates.Computing dates dynamically based on the current year prevents test failures during year transitions. The implementation is clean and well-documented.
8-13: LGTM! Comprehensive fixture data implementation.The test data has been consistently moved to fixtures, improving maintainability. The implementation follows a clear pattern and maintains good structure.
Also applies to: 19-21, 25-27, 35-41, 48-55
cypress/cypress/e2e/E-course-workflow.cy.ts (1)
3-11: LGTM! Consistent implementation pattern.The setup follows the same pattern as other test files, with well-documented dynamic date computation and proper fixture loading.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
cypress-fixtures
|
| Run status |
|
| Run duration | 14m 04s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
180
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (1)
Line range hint
1085-1110: Replace hardcoded waits with proper assertions.The use of
cy.wait(2000)could lead to flaky tests. Instead, wait for specific conditions or network requests to complete.-cy.get('[data-cy="submit-group-activity"]').click() -cy.wait(2000) +// Wait for specific conditions instead of arbitrary timeouts +cy.get('[data-cy="submit-group-activity"]').click() +cy.intercept('POST', '**/submit-group-activity').as('submitActivity') +cy.wait('@submitActivity') +cy.get('[data-cy="submission-success"]').should('be.visible')
🧹 Nitpick comments (5)
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (5)
Line range hint
419-507: Enhance type safety in helper functions.While the helper functions are well-structured, the type safety could be improved:
- The
answerGroupActivityfunction parameters should have explicit type annotations- The
checkInputsDisabledfunction could benefit from a return type annotation- Consider using an interface for the scores and comments arrays in
checkGradingVisualization-function answerGroupActivity({ NRAnswer, FTAnswer }) { +interface GroupActivityAnswers { + NRAnswer: string | number; + FTAnswer: string; +} +function answerGroupActivity({ NRAnswer, FTAnswer }: GroupActivityAnswers): void { -function checkInputsDisabled() { +function checkInputsDisabled(): void { -function checkGradingVisualization( - scores: string[], - maxPoints: string[], - comments: string[], - gradingComment?: string -) { +interface GradingVisualization { + scores: string[]; + maxPoints: string[]; + comments: string[]; + gradingComment?: string; +} +function checkGradingVisualization({ + scores, + maxPoints, + comments, + gradingComment, +}: GradingVisualization): void {
1203-1220: Optimize cleanup performance.The current cleanup implementation uses UI interactions for each question deletion. Consider using Cypress's direct API calls for cleanup to improve test execution speed.
-questions.forEach((title) => { - cy.get(`[data-cy="delete-question-${title}"]`).click() - cy.get('[data-cy="confirm-question-deletion"]').click() - cy.get(`[data-cy="element-item-${title}"]`).should('not.exist') -}) +// Use cy.request for bulk cleanup +cy.request({ + method: 'POST', + url: '/api/questions/bulk-delete', + body: { titles: questions }, +}).then((response) => { + expect(response.status).to.eq(200) +})
726-740: Improve assertion reliability with proper timeout handling.The assertions for checking UI elements could be more robust with explicit timeout configurations and better retry strategies.
-cy.get(`[data-cy="course-button-${this.data.course}"]`).click() -cy.get('[data-cy="student-course-existing-group-0"]').click() -cy.get( - `[data-cy="group-activity-${this.data.running.displayName}"]` -).contains(messages.pwa.groupActivity.submitted) +// Add timeout and better assertions +cy.get(`[data-cy="course-button-${this.data.course}"]`, { timeout: 10000 }).should('be.visible').click() +cy.get('[data-cy="student-course-existing-group-0"]', { timeout: 10000 }).should('be.visible').click() +cy.get( + `[data-cy="group-activity-${this.data.running.displayName}"]` +).should('be.visible') + .and('contain', messages.pwa.groupActivity.submitted)
585-593: Move hardcoded test data to fixtures.Consider moving all test data, including validation messages and expected states, to the fixture file for better maintainability.
-cy.get('[data-cy="flag-element-textarea"]').type( - this.data.running.flagging.text -) -cy.get('[data-cy="cancel-flag-element"]').click() -cy.get('[data-cy="flag-element-1-button"]').click() -cy.get('[data-cy="submit-flag-element"]').should('be.disabled') -cy.get('[data-cy="flag-element-textarea"]').type( - this.data.running.flagging.text -) +// Move validation states to fixture +cy.get('[data-cy="flag-element-textarea"]').type(this.data.running.flagging.text) +cy.get('[data-cy="cancel-flag-element"]').click() +cy.get('[data-cy="flag-element-1-button"]').click() +cy.get('[data-cy="submit-flag-element"]').should('have.attr', 'disabled', this.data.validation.submitDisabled) +cy.get('[data-cy="flag-element-textarea"]').type(this.data.running.flagging.text)
Line range hint
776-911: Split complex test cases for better maintainability.The grading test case handles multiple responsibilities including grading submission, validation, and state checks. Consider splitting it into smaller, focused test cases.
Example split:
it('Should validate grading inputs correctly', function () { // Input validation tests }) it('Should save grading progress correctly', function () { // Grading progress tests }) it('Should handle submission switching correctly', function () { // Submission switching tests }) it('Should finalize grading correctly', function () { // Grading finalization tests })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/cypress/e2e/J-group-activity-workflow.cy.ts(21 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: cypress-run
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (2)
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (2)
3-13: Well-structured dynamic date calculations!The approach of calculating dates dynamically based on the current year ensures that tests remain valid over time without requiring manual updates.
14-20: Good fixture setup with proper context binding!The use of function syntax instead of arrow functions ensures proper
thisbinding for accessing fixture data throughout the tests.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
cypress/cypress/e2e/D-questions-workflow.cy.ts (1)
Line range hint
11-1483: Enhance test maintainability with custom commands and utilities.The test implementation could benefit from several improvements:
- Extract common verification patterns into custom commands
- Break down long test cases into smaller, focused ones
- Move magic strings/numbers to constants or fixtures
Example custom command for question creation:
// cypress/support/commands.ts Cypress.Commands.add('createQuestion', (type, data) => { cy.get('[data-cy="create-question"]').click() cy.get('[data-cy="select-question-type"]').click() cy.get(`[data-cy="select-question-type-${type}"]`).click() cy.get('[data-cy="insert-question-title"]').type(data.title) // ... more common setup })Example utility for verification:
// cypress/support/utils.ts export const verifyQuestionInPool = (data) => { cy.get(`[data-cy="element-item-${data.title}"]`).within(() => { cy.contains(data.content) cy.contains(data.title) // ... more common verifications }) }cypress/cypress/e2e/G-microlearning-workflow.cy.ts (1)
Line range hint
82-584: Refactor long test case into smaller, focused tests.The microlearning creation test is too long (500+ lines) and handles multiple concerns. This makes it harder to maintain and debug failures.
Consider splitting it into smaller, focused test cases:
- Basic microlearning creation
- Stack management (creation, reordering)
- Display name and description handling
- Settings configuration
Example structure:
describe('Microlearning Creation', () => { it('creates basic microlearning with name and display name', function() { // Basic creation steps }) it('manages stacks correctly', function() { // Stack management steps }) it('handles display names and descriptions', function() { // Display name and description steps }) it('configures settings correctly', function() { // Settings configuration steps }) })
🧹 Nitpick comments (11)
cypress/cypress/support/commands.ts (1)
Line range hint
761-784: Consider extracting repeated click-type patterns into helper functions.The clue handling implementation could be more DRY. Consider extracting the repeated click-type pattern into a helper function.
+ function fillClueField(selector: string, value: string) { + cy.get(selector).click().type(value) + } cy.wrap(clues).each((clue: GroupActivityClueType) => { cy.get('[data-cy="add-group-activity-clue"]').click() cy.get('[data-cy="group-activity-clue-type"]').click() cy.get( `[data-cy="group-activity-clue-type-${clue.type === 'text' ? 'string' : 'number'}"]` ).click() - cy.get('[data-cy="group-activity-clue-name"]').click().type(clue.name) - cy.get('[data-cy="group-activity-clue-display-name"]') - .click() - .type(clue.displayName) + fillClueField('[data-cy="group-activity-clue-name"]', clue.name) + fillClueField('[data-cy="group-activity-clue-display-name"]', clue.displayName)cypress/cypress/e2e/F-live-quiz-workflow.cy.ts (1)
Line range hint
1136-1164: Consider using test hooks for cleanup.While the cleanup is thorough, it could be more robust by:
- Moving cleanup logic to
afterorafterEachhooks- Adding error handling for cleanup operations
Example refactor:
after(function() { // Wrap cleanup in a try-catch to ensure it runs even if tests fail try { cy.loginLecturer() cy.get(`[data-cy="live-quizzes"]`).click() // Delete quiz if it exists cy.get('body').then(($body) => { if ($body.find(`[data-cy="delete-live-quiz-${this.data.course2.quiz.name}"]`).length) { cy.get(`[data-cy="delete-live-quiz-${this.data.course2.quiz.name}"]`).click() cy.get(`[data-cy="confirm-deletion-qa-feedbacks"]`).click() cy.get(`[data-cy="activity-confirmation-modal-confirm"]`).click() } }) // Clean up questions const questions = [/* ... */] cy.wrap(questions).each((question: string) => { cy.get('body').then(($body) => { if ($body.find(`[data-cy="element-item-${question}"]`).length) { cy.get(`[data-cy="delete-question-${question}"]`).click() cy.get('[data-cy="confirm-question-deletion"]').click() } }) }) } catch (error) { cy.log('Cleanup failed:', error) } })Also applies to: 1167-1187
cypress/cypress/e2e/D-questions-workflow.cy.ts (1)
1486-1501: Improve cleanup strategy robustness.While the cleanup is thorough, consider these improvements:
- Move to an
after()hook to ensure cleanup runs even if tests fail- Handle cases where questions might already be deleted
- Consider using bulk deletion if the API supports it
Example improved cleanup:
after(function () { const questions = [ this.data.CT.titleEdited, this.data.FC.titleEdited, // ... other questions ] // Option 1: More robust individual deletion questions.forEach((title) => { cy.get('body').then($body => { if ($body.find(`[data-cy="delete-question-${title}"]`).length) { cy.get(`[data-cy="delete-question-${title}"]`).click() cy.get('[data-cy="confirm-question-deletion"]').click() cy.get(`[data-cy="element-item-${title}"]`).should('not.exist') } }) }) // Option 2: Bulk deletion if API supports it // cy.request('DELETE', '/api/questions/bulk', { titles: questions }) })cypress/cypress/e2e/G-microlearning-workflow.cy.ts (2)
3-11: Extract date constants into test fixtures.The date-related constants contain magic numbers and are hardcoded. Consider moving these to the fixture file for better maintainability.
-// dates - hard-coded in test due to dependency on current year -const currentYear = new Date().getFullYear() -const runningStartOLD = `${currentYear - 1}-01-01T02:00` -const runningEndOLD = `${currentYear}-12-31T18:00` -const runningStart = `${currentYear - 2}-01-01T02:00` -const runningEnd = `${currentYear + 1}-12-31T18:00` -const runningEndExtended = `${currentYear + 5}-12-31T18:00` -const runningExtendedText = `End: 31.12.${currentYear + 5}, 18:00`Move these to the fixture file and access them via
this.data.dates:{ "dates": { "runningStartOLD": "${currentYear - 1}-01-01T02:00", "runningEndOLD": "${currentYear}-12-31T18:00", "runningStart": "${currentYear - 2}-01-01T02:00", "runningEnd": "${currentYear + 1}-12-31T18:00", "runningEndExtended": "${currentYear + 5}-12-31T18:00", "runningExtendedText": "End: 31.12.${currentYear + 5}, 18:00" } }
Line range hint
720-773: Add error handling for network failures.The test cases for student interactions should handle potential network failures when submitting responses.
Add error handling using Cypress's failure hooks:
cy.on('fail', (error) => { // Handle submission failures if (error.message.includes('Failed to submit response')) { // Retry submission or fail gracefully } throw error })cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (3)
15-71: Consider adding error handling for question creationWhile the question creation is well-structured, consider adding assertions to verify successful creation of each question type before proceeding to the next one.
cy.createQuestionSC({ title: this.data.questions.SC1.title, content: this.data.questions.SC1.content, choices: this.data.questions.SC1.choices, -}) +}).then(() => { + cy.get(`[data-cy="element-item-${this.data.questions.SC1.title}"]`).should('exist') +})
157-168: Consider using Cypress drag-drop plugin for more reliable testsThe current drag and drop implementation using
dataTransfermight be flaky in certain browsers. Consider using@4tw/cypress-drag-dropplugin for more reliable drag and drop operations.-const dataTransfer = new DataTransfer() -cy.get(`[data-cy="element-item-${this.data.questions.SC2.title}"]`) - .contains(this.data.questions.SC2.title) - .trigger('dragstart', { - dataTransfer, - }) -cy.get('[data-cy="drop-elements-stack-1"]').trigger('drop', { - dataTransfer, -}) +cy.get(`[data-cy="element-item-${this.data.questions.SC2.title}"]`) + .drag('[data-cy="drop-elements-stack-1"]')
Line range hint
467-567: Consider modularizing the quiz answering helper functionThe
answerRunningPracticeQuizfunction is quite long and handles multiple question types. Consider breaking it down into smaller, type-specific helper functions for better maintainability.Example refactor:
function answerSCQuestion() { cy.get('[data-cy="student-stack-submit"]').should('be.disabled') cy.get('[data-cy="sc-1-answer-option-2"]').click() cy.get('[data-cy="student-stack-submit"]').click() cy.get('[data-cy="student-stack-continue"]').click() } function answerMCQuestion() { // Similar implementation for MC questions } // Main function becomes more readable function answerRunningPracticeQuiz() { answerSCQuestion() answerMCQuestion() // ... other question types }cypress/cypress/e2e/I-bookmarking-workflow.cy.ts (3)
3-6: Consider more robust date handling for edge cases.The dynamic year computation improves maintainability, but the hardcoded months and times might cause issues:
- Tests could fail when running near year boundaries
- Timezone differences might affect test reliability around midnight
Consider using a date library like
dayjsto handle dates more robustly:-const currentYear = new Date().getFullYear() -const mlStartDate = `${currentYear}-01-01T02:00` -const mlEndDate = `${currentYear}-12-31T18:00` +const dayjs = require('dayjs') +const now = dayjs() +const mlStartDate = now.startOf('year').format('YYYY-MM-DDTHH:mm') +const mlEndDate = now.endOf('year').format('YYYY-MM-DDTHH:mm')
16-28: Add verification steps after question creation.While the question creation looks good, consider adding assertions to verify that the questions were created successfully.
Add these verifications after each question creation:
cy.createQuestionSC({ title: this.data.question1.title, content: this.data.question1.content, choices: this.data.question1.choices, }) +cy.get(`[data-cy="element-item-${this.data.question1.title}"]`).should('exist') cy.createQuestionMC({ title: this.data.question2.title, content: this.data.question2.content, choices: this.data.question2.choices, }) +cy.get(`[data-cy="element-item-${this.data.question2.title}"]`).should('exist')
32-56: Add verification steps after activity creation.The activity creation looks good, but consider adding assertions to verify successful creation.
Add these verifications:
cy.createPracticeQuiz({ name: this.data.PQ.name, displayName: this.data.PQ.displayName, courseName: this.data.course, stacks: [ { elements: [this.data.question1.title] }, { elements: [this.data.question2.title] }, ], }) +cy.get(`[data-cy="practice-quiz-${this.data.PQ.name}"]`).should('exist') cy.createMicroLearning({ name: this.data.ML.name, displayName: this.data.ML.displayName, courseName: this.data.course, startDate: mlStartDate, endDate: mlEndDate, stacks: [ { elements: [this.data.question1.title, this.data.question2.title] }, ], }) +cy.get(`[data-cy="microlearning-${this.data.ML.name}"]`).should('exist')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cypress/cypress/e2e/D-questions-workflow.cy.ts(24 hunks)cypress/cypress/e2e/F-live-quiz-workflow.cy.ts(24 hunks)cypress/cypress/e2e/G-microlearning-workflow.cy.ts(16 hunks)cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts(10 hunks)cypress/cypress/e2e/I-bookmarking-workflow.cy.ts(3 hunks)cypress/cypress/e2e/J-group-activity-workflow.cy.ts(22 hunks)cypress/cypress/support/commands.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/cypress/e2e/J-group-activity-workflow.cy.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
🔇 Additional comments (14)
cypress/cypress/support/commands.ts (3)
Line range hint
163-168: Great improvement in handling asynchronous operations!The change from
forEachtocy.wrap().each()ensures proper command chaining in Cypress. This is crucial because:
forEachruns synchronously and doesn't respect Cypress's command queuecy.wrap().each()ensures commands are properly chained and executed sequentiallyAlso applies to: 174-179, 218-223, 229-234, 374-384, 436-440
Line range hint
550-563: Well-handled drag-and-drop sequencing!The use of
cy.wrap().each()is particularly important here as it ensures drag-and-drop operations are performed sequentially and reliably. This prevents race conditions that could occur with the previousforEachimplementation.Also applies to: 566-581
696-702: Good addition of type safety with GroupActivityClueType interface!The new interface improves code maintainability and provides better type checking for group activity clues.
cypress/cypress/e2e/F-live-quiz-workflow.cy.ts (2)
3-8: Well-structured test suite setup!The setup follows best practices by:
- Using function syntax for proper
thiscontext binding- Loading test data from fixtures for better maintainability
Line range hint
1182-1187: ReplaceforEachwithcy.wrap().each()for reliable command execution.Using
forEachwith Cypress commands can lead to unpredictable behavior. Usecy.wrap().each()for reliable command execution.Apply this diff to fix the issue:
- cy.wrap(questions).each((question: string) => { - cy.get(`[data-cy="element-item-${question}"]`).should('exist') - cy.get(`[data-cy="delete-question-${question}"]`).click() - cy.get('[data-cy="confirm-question-deletion"]').click() - cy.get(`[data-cy="element-item-${question}"]`).should('not.exist') - }) + cy.wrap(questions).each((question: string) => { + cy.get(`[data-cy="element-item-${question}"]`).should('exist') + cy.get(`[data-cy="delete-question-${question}"]`).click() + cy.get('[data-cy="confirm-question-deletion"]').click() + cy.get(`[data-cy="element-item-${question}"]`).should('not.exist') + })cypress/cypress/e2e/D-questions-workflow.cy.ts (1)
3-8: Well-structured test setup!Good practices observed:
- Using function syntax for proper
thiscontext binding- Loading test data from fixtures for better maintainability
- Ensuring clean state with beforeEach hook
cypress/cypress/e2e/G-microlearning-workflow.cy.ts (2)
14-19: LGTM! Proper fixture loading implementation.The fixture loading is correctly implemented using function syntax for proper
thisbinding, which is essential for accessing fixture data throughout the test suite.
22-78: LGTM! Well-structured question creation with fixture data.The question creation tests are well-organized and make good use of fixture data for different question types. The implementation:
- Uses appropriate custom commands for each question type
- Maintains consistency in data structure
- Provides good test coverage for different question types
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (3)
7-12: Well-structured test suite setup!The use of function syntax and fixture loading in
beforeEachfollows Cypress best practices for proper context binding and test data management.
572-589: Fixthiscontext in task callbackThe same issue with
thiscontext in callbacks exists here as previously identified.
789-807:⚠️ Potential issueReplace
forEachwithcy.wrap().each()for Cypress commandsUsing
forEachwith Cypress commands is an anti-pattern that can lead to race conditions and flaky tests.Apply this diff to fix the issue:
- cy.wrap(questions).each((title) => { - cy.get(`[data-cy="delete-question-${title}"]`).click() - cy.get('[data-cy="confirm-question-deletion"]').click() - cy.get(`[data-cy="element-item-${title}"]`).should('not.exist') - }) + cy.wrap(questions).each((title) => { + cy.get(`[data-cy="delete-question-${title}"]`).click() + cy.get('[data-cy="confirm-question-deletion"]').click() + cy.get(`[data-cy="element-item-${title}"]`).should('not.exist') + })Likely invalid or redundant comment.
cypress/cypress/e2e/I-bookmarking-workflow.cy.ts (3)
8-13: Well-structured test setup with proper fixture loading!Good practices observed:
- Using
functionsyntax for properthisbinding- Loading fixtures in
beforeEachhook- Separating test data from test logic
240-250: Well-implemented cleanup functionality!Good practices observed:
- Systematic cleanup of test data
- Proper use of
cy.wrap().each()for iteration- Verification of successful deletion
75-90: 🛠️ Refactor suggestionImprove test stability for flagging interactions.
The flagging tests might be flaky due to fixed wait times. Consider using Cypress's built-in retry-ability instead.
Replace fixed waits with proper assertions:
-cy.wait(500) +cy.get('[data-cy="upvote-element-0-button"]') + .should('have.class', 'voted') // adjust class name based on your implementationLikely invalid or redundant comment.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 13m 39s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
180
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
Chores
Tests