Development: Add exercise update announcement e2e tests
#9784
Conversation
…rticipants getting informed
muradium
changed the title
Development: Playwright exercise update announcement e2e testsDevelopment: Exercise update announcement e2e tests
github-actions
bot
added
tests
client
WalkthroughThe changes in this pull request primarily enhance the exercise group management interface and exam participation functionalities. Modifications include the addition of new methods and classes for managing exercise edits and improving test coverage for dynamic content updates. The exercise management interface has been streamlined for better accessibility, while new test cases and utility methods have been introduced to support instructors in modifying problem statements and notifying participants. Overall, the updates focus on improving code maintainability and user experience within the exam management system. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
src/test/playwright/support/pageobjects/exam/EditExamPage.ts (1)
1-4: Consider expanding the Page Object to support exercise update scenariosGiven that this PR focuses on testing exercise problem statement updates during exams, the class should include methods for:
- Modifying exercise problem statements
- Verifying notification displays
- Checking problem statement differences
Would you like assistance in implementing these additional methods to align with the PR objectives?
src/test/playwright/support/pageobjects/exam/ExamExerciseGroupsPage.ts (1)
62-64: LGTM with suggestions for robustness.The implementation follows the page object pattern and aligns well with the PR's objective of testing exercise updates. Consider these improvements for better reliability:
- Add a more specific selector for the edit button
- Add error handling for non-existent elements
Consider this enhancement:
async clickEditExercise(groupID: number, exerciseID: number) { - await this.page.locator(`#group-${groupID} #exercise-${exerciseID}`).locator('.btn', { hasText: 'Edit' }).click(); + const exerciseContainer = this.page.locator(`#group-${groupID} #exercise-${exerciseID}`); + await expect(exerciseContainer).toBeVisible({ timeout: 5000 }); + const editButton = exerciseContainer.locator('[data-testid="edit-exercise-btn"]', { hasText: 'Edit' }); + await expect(editButton).toBeEnabled(); + await editButton.click(); }src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts (2)
9-11: Consider centralizing selectors in a separate constants fileWhile the selector definitions are clear and well-structured, consider moving them to a shared constants file to improve maintainability and reusability across test files.
+ // In src/test/playwright/support/constants/selectors.ts + export const TEXT_EXERCISE_SELECTORS = { + PROBLEM_STATEMENT: '#problemStatement', + EXAMPLE_SOLUTION: '#exampleSolution', + ASSESSMENT_INSTRUCTIONS: '#gradingInstructions', + }; - private readonly PROBLEM_STATEMENT_SELECTOR = '#problemStatement'; - private readonly EXAMPLE_SOLUTION_SELECTOR = '#exampleSolution'; - private readonly ASSESSMENT_INSTRUCTIONS_SELECTOR = '#gradingInstructions'; + private readonly PROBLEM_STATEMENT_SELECTOR = TEXT_EXERCISE_SELECTORS.PROBLEM_STATEMENT; + private readonly EXAMPLE_SOLUTION_SELECTOR = TEXT_EXERCISE_SELECTORS.EXAMPLE_SOLUTION; + private readonly ASSESSMENT_INSTRUCTIONS_SELECTOR = TEXT_EXERCISE_SELECTORS.ASSESSMENT_INSTRUCTIONS;
44-46: Consider more robust text clearing approachWhile the current implementation works, keyboard shortcuts can be flaky in E2E tests. Consider using Monaco Editor's API or value setting for more reliable text clearing.
private async clearText(textEditor: Locator) { - await textEditor.click(); - await textEditor.press('Control+a'); - await textEditor.press('Backspace'); + // Option 1: Use Monaco Editor's API + await this.page.evaluate((selector) => { + const editor = monaco.editor.getModels().find(model => + model.uri.path.includes(selector)); + if (editor) editor.setValue(''); + }, textEditor); + + // Option 2: Set value directly if Monaco API isn't accessible + await textEditor.evaluate((element) => { + element.textContent = ''; + element.dispatchEvent(new Event('input', { bubbles: true })); + }); }Also applies to: 54-56, 64-66, 85-89
src/test/playwright/support/pageobjects/exam/ExamParticipationActions.ts (1)
28-29: Consider using more reliable selectorsThe current selector
.cardwith text matching might be fragile. Consider using a dedicated test ID for more reliable element selection.-const problemStatementCard = this.page.locator('.card', { hasText: 'Problem Statement' }); +const problemStatementCard = this.page.getByTestId('problem-statement-card');src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.html (2)
Line range hint
38-108: Consider refactoring repeated permission checks.The permission check
@if (course?.isAtLeastEditor)is duplicated for each exercise type button group. Consider refactoring this section to reduce duplication and improve maintainability.Example approach:
- @if (course?.isAtLeastEditor) { - <div class="btn-group-vertical me-1 mb-1" style="justify-content: end"> - <a [routerLink]="[exerciseGroup.id, 'file-upload-exercises', 'new']" class="btn btn-info btn-sm me-1 mb-1" style="max-height: 44%"> - <!-- ... --> - </a> - <!-- ... --> - </div> - } - @if (course?.isAtLeastEditor) { - <div class="btn-group-vertical me-1 mb-1" style="justify-content: end"> - <!-- ... --> - </div> - } + @if (course?.isAtLeastEditor) { + <div class="d-flex"> + <div class="btn-group-vertical me-1 mb-1" style="justify-content: end"> + <a [routerLink]="[exerciseGroup.id, 'file-upload-exercises', 'new']" class="btn btn-info btn-sm me-1 mb-1" style="max-height: 44%"> + <!-- ... --> + </a> + <!-- ... --> + </div> + <div class="btn-group-vertical me-1 mb-1" style="justify-content: end"> + <!-- ... --> + </div> + </div> + }
Line range hint
156-186: Enhance table accessibility.Consider improving the table's accessibility for screen readers:
- Add a table caption or aria-label
- Add scope attributes to table headers
Example improvements:
- <table class="table table-striped"> + <table class="table table-striped" aria-label="{{ 'artemisApp.examManagement.exerciseGroup.exerciseTable' | artemisTranslate }}"> <thead> <tr> - <th class="d-md-table-cell"> + <th scope="col" class="d-md-table-cell"> <span jhiTranslate="global.field.id"></span> </th> - <th> + <th scope="col"> <span jhiTranslate="artemisApp.examManagement.exerciseGroup.type"></span> </th> <!-- ... --> </tr> </thead>src/test/playwright/e2e/exam/ExamParticipation.spec.ts (2)
410-410: Use a more robust selector for the 'Highlight Differences' buttonThe selector
#highlightDiffButtonmight be susceptible to breakage if the DOM structure changes. Using a data attribute or a more descriptive selector can make the test more resilient to UI changes.For example, update the selector to use a data-testid:
-await studentPage.locator('#highlightDiffButton').click(); +await studentPage.locator('[data-testid="highlight-diff-button"]').click();Ensure that the HTML element includes the
data-testid="highlight-diff-button"attribute.
372-376: Ensure browser contexts are properly closed to prevent resource leaksThe browser contexts created for each student are not being closed after the test. This can lead to resource leaks, especially when running multiple tests in sequence.
Consider closing each
studentContextafter its associated test actions are completed:const studentContexts = []; for (const student of students) { const studentContext = await browser.newContext(); const studentPage = await studentContext.newPage(); studentPages.push(studentPage); + studentContexts.push(studentContext); await Commands.login(studentPage, student); // ...existing code... } +// After all test actions are completed +for (const context of studentContexts) { + await context.close(); +}This ensures that all resources are properly released after the test execution.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.html(1 hunks)src/test/playwright/e2e/exam/ExamParticipation.spec.ts(3 hunks)src/test/playwright/support/fixtures.ts(3 hunks)src/test/playwright/support/pageobjects/exam/EditExamPage.ts(1 hunks)src/test/playwright/support/pageobjects/exam/ExamExerciseGroupsPage.ts(1 hunks)src/test/playwright/support/pageobjects/exam/ExamParticipationActions.ts(2 hunks)src/test/playwright/support/pageobjects/exam/ModalDialogBox.ts(2 hunks)src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts(3 hunks)
🧰 Additional context used
🔇 Additional comments (13)
src/test/playwright/support/pageobjects/exam/EditExamPage.ts (1)
6-8: LGTM! Constructor follows TypeScript best practices
The constructor is well-implemented with proper parameter typing and field assignment.
src/test/playwright/support/pageobjects/exam/ModalDialogBox.ts (1)
27-29: LGTM! Clean implementation of dialog type verification.
The method follows the established patterns in the class and correctly uses Playwright's locator API.
src/test/playwright/support/pageobjects/exam/ExamExerciseGroupsPage.ts (1)
62-64: Verify selector consistency across test files.
Let's ensure the ID patterns (#group-${groupID} #exercise-${exerciseID}) are consistently used across the test suite.
src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts (3)
39-42: LGTM! Well-structured text input methods
The text input methods are well-implemented:
- Follow consistent patterns
- Reuse utility methods effectively
- Align with Page Object Model best practices
Also applies to: 49-52, 59-62
81-83: LGTM! Well-designed utility methods
The utility methods are well-implemented:
- Clear single responsibility
- Good encapsulation
- Reliable text input using pressSequentially
Also applies to: 91-93
Line range hint 1-93: Verify test coverage for exercise update scenarios
The implementation supports the PR objectives for testing exercise updates during exams. To ensure comprehensive coverage, verify that these methods are used to test:
- Initial problem statement setting
- Mid-exam updates
- Notification triggers
✅ Verification successful
Let me gather more specific information about the test coverage by examining the test files found.
Let me check for the complete test context around the exercise updates to better understand the coverage.
Based on the test content found, I can now provide the final response:
Test coverage for exercise updates is comprehensive and well-implemented
The implementation is properly tested with scenarios that cover:
- Initial problem statement setting and verification
- Mid-exam problem statement updates with change tracking
- Update notifications to students via modal dialogs
- Text difference highlighting functionality
- Student navigation to updated exercises
The test Instructor changes problem statement and all participants are informed specifically validates the update notification flow with:
- Problem statement modification during exam
- Update notification delivery to all participants
- Proper display of text differences (additions/deletions)
- Navigation to the updated exercise
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using these methods for exercise updates
rg -l "typeProblemStatement|clearProblemStatement" --type ts
# Check test scenarios covering exercise updates
ast-grep --pattern 'test($_, async $_) {
$$$
typeProblemStatement($$$)
$$$
}'
Length of output: 657
Script:
#!/bin/bash
# Check test content in ExamParticipation.spec.ts for exercise update scenarios
rg -A 10 "typeProblemStatement" "src/test/playwright/e2e/exam/ExamParticipation.spec.ts"
# Check test content in TextExerciseManagement.spec.ts
rg -A 10 "typeProblemStatement" "src/test/playwright/e2e/exercise/text/TextExerciseManagement.spec.ts"
# Look for test descriptions related to updates
rg "test\(.*update.*exercise" "src/test/playwright/e2e/exam/ExamParticipation.spec.ts"
Length of output: 1876
Script:
#!/bin/bash
# Get more context around the exercise update test
rg -B 10 -A 20 "exercise.*was updated" "src/test/playwright/e2e/exam/ExamParticipation.spec.ts"
# Look for test titles/descriptions
rg "test\(.*'" "src/test/playwright/e2e/exam/ExamParticipation.spec.ts"
# Check for any notification or update related constants
rg "UPDATE|NOTIFICATION" "src/test/playwright/constants.ts"
Length of output: 3683
src/test/playwright/support/pageobjects/exam/ExamParticipationActions.ts (2)
118-129: LGTM! Clean and well-structured type definitions
The TextDifferenceSlice class and TextDifferenceType enum are well-designed and follow TypeScript best practices. The types are properly exported and provide a clear contract for the difference checking functionality.
27-50: Verify test coverage for all update scenarios
The implementation looks good for testing exercise problem statement updates. To ensure comprehensive coverage, verify that all possible update scenarios are tested.
✅ Verification successful
Let me check the test scenarios by examining the test file content.
Test coverage is comprehensive for exercise problem statement updates
The implementation has good test coverage with scenarios for:
- Complex updates showing both additions and deletions simultaneously
- Simple updates showing the final state without differences
- Toggle between difference view and normal view
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for exercise update scenarios
# Look for related test files and their scenarios
echo "Searching for related test files..."
rg -l "ExamParticipationActions" "src/test/playwright"
# Check for different test scenarios using the new method
echo "Checking test scenarios..."
rg "checkExerciseProblemStatementDifference" "src/test/playwright"
Length of output: 1032
Script:
#!/bin/bash
# Get the test scenarios from ExamParticipation.spec.ts
rg -A 10 "checkExerciseProblemStatementDifference" "src/test/playwright/e2e/exam/ExamParticipation.spec.ts"
Length of output: 1012
src/test/playwright/support/fixtures.ts (3)
70-70: LGTM: Import statement follows established patterns
The import statement is well-placed and follows the existing conventions for page object imports.
100-100: LGTM: Type definition maintains consistency
The editExam property is well-integrated into the ArtemisPageObjects type, following the established naming patterns and maintaining alphabetical ordering.
224-226: LGTM with verification: Fixture implementation follows patterns
The fixture implementation follows the established patterns for page objects. However, let's verify the referenced class implementation.
✅ Verification successful
LGTM: EditExamPage class exists and is properly implemented
The verification confirms:
- EditExamPage class exists in
src/test/playwright/support/pageobjects/exam/EditExamPage.ts - Class follows the page object pattern with proper constructor and page handling
- The fixture is actively used in test
ExamParticipation.spec.tswhich calls theopenExerciseGroupsmethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that EditExamPage class exists and is implemented correctly
# Check if the EditExamPage class file exists and contains the expected class definition
ast-grep --pattern 'export class EditExamPage {
$$$
}'
# Check for any references to this page object in test files
rg "editExam\." "src/test/playwright"
Length of output: 1262
src/main/webapp/app/exam/manage/exercise-groups/exercise-groups.component.html (2)
245-245: LGTM! Good addition of exercise row identifiers.
Adding unique IDs to exercise rows improves testability and accessibility. This change aligns well with the PR objectives for implementing E2E tests for exercise updates.
Line range hint 1-400: LGTM! Excellent adherence to Angular control flow syntax.
The template consistently uses the new Angular control flow syntax (@if and @for) as required by the coding guidelines.
src/test/playwright/support/pageobjects/exam/ExamParticipationActions.ts
Show resolved
Hide resolved
src/test/playwright/support/pageobjects/exam/ExamParticipationActions.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/test/playwright/e2e/exam/ExamParticipation.spec.ts (1)
391-406: Consider adding more assertions for robustnessWhile the test covers the basic flow, it could benefit from additional assertions:
- Verify the modal dialog timestamp
- Ensure the exercise title in the navigation matches
- Validate that other exercises (if any) remain unchanged
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/playwright/e2e/exam/ExamParticipation.spec.ts(3 hunks)
🔇 Additional comments (2)
src/test/playwright/e2e/exam/ExamParticipation.spec.ts (2)
14-15: LGTM: Import statements are well-organized
The new imports for TextDifferenceType and ExamNavigationBar are appropriately placed and necessary for the new test functionality.
267-267: LGTM: Exercise variable is properly scoped and initialized
The exercise variable is correctly declared at the describe block level and properly initialized in the beforeEach hook.
Also applies to: 272-272
krusche
changed the title
Development: Exercise update announcement e2e testsDevelopment: Add exercise update announcement e2e tests
Checklist
General
Client
Motivation and Context
We want to test the scenario where instructor changes the exercise problem statement during an exam run and students receive information about it.
Description
Adds E2E tests in Playwright about instructor changing exercise problem statement, all participants receiving pop-up notification about it, and correctly displaying the differences in problem statement.
Steps for Testing
Steps for running the tests:
npm install && npm run playwright:setupnpx playwright test e2e/exam/ExamParticipation.spec.ts --grep "Instructor changes problem statement and all participants are informed"npm run playwright:opento open the Playwright UI and search for the "Instructor changes problem statement and all participants are informed" test to run and observe its executionTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores