Development: Playwright exam checklist e2e tests
#10050
Conversation
…and dates set checks
github-actions
bot
added
tests
client
… exam date differences
# Conflicts: # src/test/playwright/support/fixtures.ts
WalkthroughThis pull request introduces enhancements to the exam management and testing infrastructure in the Artemis platform. The changes primarily focus on improving testability by adding Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (19)
src/test/playwright/support/pageobjects/exam/ExamExerciseGroupCreationPage.ts (2)
30-36: Ensure proper naming or return value for the “isMandatoryBoxShouldBeChecked” usage
Although this code is correct in terms of toggling the checkbox, the method name "isMandatoryBoxShouldBeChecked" does not return the boolean promise, nor is it used as an assertion. You might consider renaming it to "checkMandatoryBoxState" and returning the boolean result from "isChecked()" if needed.
82-89: Check for consistent handling of undefined parameters
Similar to the above comment, "isMandatory" and "exerciseTemplate" are optional. Confirm that "isMandatory" defaults to a suitable fallback (e.g., false) and that "exerciseTemplate" is only used where relevant.src/test/playwright/support/utils.ts (2)
198-258: Potentially broad function scope
The new “prepareExam” function incorporates several tasks (creating an exam, adding an exercise, registering students, generating exams, preparing for start, making a submission). While it works, consider whether the scope is too broad and if you can break it down into smaller, more focused functions for easier maintenance and test clarity.
260-275: Ensure robust error-handling for exam submission steps
The new “makeExamSubmission” method introduces a sequence of asynchronous steps. If any of these steps (e.g., “startParticipation”, “makeSubmission”, “handInEarly”) fails, a partial submission might occur. If possible, design a fallback or error-handling mechanism to handle partial exam submissions or issues during the submission process.src/test/playwright/e2e/exam/ExamAssessment.spec.ts (2)
73-73: Prevent unclear time increments
Using “dayjs().add(45, 'seconds')” might be too short for all test steps in certain CI environments. Evaluate whether more generous time buffers are needed or if a dynamic wait is more consistent.
148-148: Add coverage for quiz after exam start
After the exam is prepared for “ExerciseType.QUIZ”, it’s beneficial to explicitly test that the student can see the questions, confirm the right quiz format, then proceed with submission.src/test/playwright/e2e/exam/ExamChecklists.spec.ts (4)
30-43: Ensure consistent naming conventions
Consider aligning the “ExamChecklistItem.LEAST_ONE_EXERCISE_GROUP” naming with your existing naming patterns. Consistency in naming across specs and page objects makes it easier to track.
78-91: Refactor repetitive steps
The logic for adding empty vs. filled exercise groups might be extracted into a helper. This would reduce code duplication and make test steps more obvious.
193-213: Consider separate tests for date publishing vs. date review
You’re combining multiple date checks in a single test. Splitting them into smaller, more targeted tests could make it easier to isolate potential failures with publish date or review date logic.
277-304: Check for potential concurrency issues with “isMandatory”
You are toggling “isMandatory” in “addExamExerciseGroup”. If multiple instructors or parallel processes are updating exam groups, ensure that concurrency is handled gracefully.src/main/webapp/app/shared/components/checklist-check/checklist-check.component.html (1)
2-2: Consider adding aria-label for better accessibility.While the data-testid attributes are great for testing, consider adding aria-label attributes to improve accessibility for screen readers.
- <fa-icon [icon]="faCheckCircle" [size]="size" [style]="{ color: iconColor ?? 'var(--green)' }" data-testid="check-icon-checked" /> + <fa-icon [icon]="faCheckCircle" [size]="size" [style]="{ color: iconColor ?? 'var(--green)' }" data-testid="check-icon-checked" aria-label="Item checked" /> - <fa-icon [icon]="faTimes" [size]="size" class="fs-5" [style]="{ color: iconColor ?? 'var(--markdown-preview-red)' }" data-testid="check-icon-unchecked" /> + <fa-icon [icon]="faTimes" [size]="size" class="fs-5" [style]="{ color: iconColor ?? 'var(--markdown-preview-red)' }" data-testid="check-icon-unchecked" aria-label="Item unchecked" />Also applies to: 4-4
src/test/playwright/support/pageobjects/exam/StudentExamManagementPage.ts (1)
24-26: Consider adding response waiting pattern for consistency.Other methods in this class (e.g., clickGenerateStudentExams) wait for API responses. Consider applying the same pattern here for consistency and reliability.
async clickPrepareExerciseStart() { + const responsePromise = this.page.waitForResponse(`${COURSE_BASE}/*/exams/*/prepare-exercise-start`); await this.page.click('#startExercisesButton'); + await responsePromise; }src/test/playwright/support/pageobjects/exam/ExamDetailsPage.ts (2)
13-15: Consider using consistent selector strategy.Some methods use testid selectors while others use id selectors. For consistency and maintainability, consider using data-testid throughout.
- await this.page.locator(`#exercises-button-groups`).click(); + await this.page.getByTestId('exercises-button-groups').click(); - await this.page.locator('#editButton_publish').click(); + await this.page.getByTestId('edit-button-publish').click(); - await this.page.locator('#editButton_review').click(); + await this.page.getByTestId('edit-button-review').click(); - await this.page.locator('#evaluateQuizExercisesButton').click(); + await this.page.getByTestId('evaluate-quiz-exercises-button').click(); - await this.page.locator('#assessUnsubmittedExamModelingAndTextParticipationsButton').click(); + await this.page.getByTestId('assess-unsubmitted-participations-button').click();Also applies to: 47-49, 51-53, 55-57, 59-61
17-22: Consider adding type for error messages.The error messages could benefit from being defined as constants or in a separate type to ensure consistency and reusability.
type ChecklistErrorMessages = { [K in ExamChecklistItem]: string; }; const CHECKLIST_ERROR_MESSAGES: ChecklistErrorMessages = { [ExamChecklistItem.LEAST_ONE_EXERCISE_GROUP]: 'Checklist item for "least one exercise group" is not checked or not found', // ... other messages };Also applies to: 24-29
src/test/playwright/e2e/exam/ExamCreationDeletion.spec.ts (1)
141-144: Consider using date objects for comparisons.Using string comparison for dates can be fragile due to timezone and formatting differences. Consider comparing the actual date objects:
-expect(trimDate(editedExam.visibleDate)).toBe(trimDate(dayjsToString(editedExamData.visibleDate))); -expect(trimDate(editedExam.startDate)).toBe(trimDate(dayjsToString(editedExamData.startDate))); -expect(trimDate(editedExam.endDate)).toBe(trimDate(dayjsToString(editedExamData.endDate))); +expect(dayjs(editedExam.visibleDate).isSame(editedExamData.visibleDate)).toBeTruthy(); +expect(dayjs(editedExam.startDate).isSame(editedExamData.startDate)).toBeTruthy(); +expect(dayjs(editedExam.endDate).isSame(editedExamData.endDate)).toBeTruthy();src/test/playwright/support/requests/ExamAPIRequests.ts (2)
121-126: Enhance method documentation.Consider adding more details to the JSDoc comment, such as the purpose of bulk registration and any prerequisites or side effects.
/** * Register all course students for the exam + * @param exam The exam object for which to register all course students + * @description This method performs bulk registration of all students enrolled in the course + * for the specified exam. It's more efficient than registering students individually. */
218-229: LGTM! Well-implemented exam completion logic.The method properly handles the exam completion by calculating and adjusting the working time. The defensive check for positive time is a good practice.
Consider clarifying the safety margin comment:
-// Determine the time left until the exam ends and add extra minute -// to make sure the exam is finished after subtracting it from the working time +// Add a 60-second safety margin to ensure the exam is marked as finished +// when subtracting the remaining time from the working timesrc/test/playwright/support/requests/ExerciseAPIRequests.ts (1)
220-227: LGTM! Consider adding type safety for exerciseTemplate.The new parameter addition is well documented and implemented correctly. However, consider replacing
anywith a proper type definition for better type safety.-exerciseTemplate: any = textExerciseTemplate, +type TextExerciseTemplate = typeof textExerciseTemplate; +exerciseTemplate: TextExerciseTemplate = textExerciseTemplate,src/main/webapp/app/exam/manage/exams/exam-checklist-component/exam-checklist.component.html (1)
Line range hint
1-671: Consider adding data-testid to remaining interactive elements.For comprehensive E2E testing coverage, consider adding data-testid attributes to other interactive elements like:
- Edit buttons (e.g.,
editButton_table,editButton_publish,editButton_review)- Assessment dashboard buttons
- Score view button
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/main/webapp/app/exam/manage/exams/exam-checklist-component/exam-checklist.component.html(12 hunks)src/main/webapp/app/shared/components/checklist-check/checklist-check.component.html(1 hunks)src/test/playwright/e2e/exam/ExamAssessment.spec.ts(5 hunks)src/test/playwright/e2e/exam/ExamChecklists.spec.ts(1 hunks)src/test/playwright/e2e/exam/ExamCreationDeletion.spec.ts(3 hunks)src/test/playwright/e2e/exam/ExamParticipation.spec.ts(4 hunks)src/test/playwright/support/fixtures.ts(0 hunks)src/test/playwright/support/pageobjects/exam/EditExamPage.ts(0 hunks)src/test/playwright/support/pageobjects/exam/ExamCreationPage.ts(1 hunks)src/test/playwright/support/pageobjects/exam/ExamDetailsPage.ts(2 hunks)src/test/playwright/support/pageobjects/exam/ExamExerciseGroupCreationPage.ts(3 hunks)src/test/playwright/support/pageobjects/exam/StudentExamManagementPage.ts(1 hunks)src/test/playwright/support/requests/ExamAPIRequests.ts(2 hunks)src/test/playwright/support/requests/ExerciseAPIRequests.ts(2 hunks)src/test/playwright/support/utils.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/test/playwright/support/pageobjects/exam/EditExamPage.ts
- src/test/playwright/support/fixtures.ts
🧰 Additional context used
🔇 Additional comments (22)
src/test/playwright/e2e/exam/ExamParticipation.spec.ts (4)
263-263: LGTM: Improved test execution granularity
Moving the @slow tag from the suite level to individual tests provides better control over test execution and allows faster tests to run independently.
281-318: LGTM: Well-structured test with appropriate tagging
The test is well-organized with clear phases and proper resource management. The @slow tag is appropriate given the multiple browser contexts and page interactions.
320-320: LGTM: Appropriate test categorization
The addition of the @slow tag is appropriate for this test given its complexity and multiple browser contexts.
Line range hint 361-382: LGTM: Improved naming with examDetails
The replacement of editExam with examDetails provides better clarity about the page object's purpose.
Let's verify that this rename is consistent across the codebase:
✅ Verification successful
The variable rename is consistent across the codebase
The verification shows that:
- There are no remaining occurrences of
editExamin the Playwright test files - The new
examDetailsvariable is consistently used across multiple test files including:ExamParticipation.spec.tsExamCreationDeletion.spec.tsExamChecklists.spec.tsTestExamCreationDeletion.spec.ts
- The variable is properly defined in
fixtures.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining usage of editExam in test files
rg "editExam" "src/test/playwright"
# Check for the new examDetails usage pattern
rg "examDetails" "src/test/playwright"
Length of output: 9421
src/test/playwright/support/pageobjects/exam/ExamExerciseGroupCreationPage.ts (1)
59-65: Validate optional parameters usage
You introduced optional parameters (isMandatory, exerciseTemplate). Ensure that downstream flows cover the case when they are undefined. For instance, confirm that any logic that depends on 'isMandatory' or 'exerciseTemplate' gracefully handles a non-passed or undefined value.
src/test/playwright/support/utils.ts (1)
277-297:
Verify concurrency scenarios
The “startAssessing” function toggles second assessments and manipulates dashboards. Check if concurrency might be an issue here when multiple tutors initiate the assessment concurrently. Consider whether concurrency or lock states arise in your system.
Run the following script to search for concurrency or lock handling in the codebase:
src/test/playwright/e2e/exam/ExamAssessment.spec.ts (3)
16-16: Validate “startAssessing” usage
The import “startAssessing” is heavily utilized throughout this test file. Confirm that all invocation sites match updated signature changes (if any) and handle the second correction round toggling properly.
[approve]
49-49: Avoid silent test failures
When calling “prepareExam” here, consider adding more robust checks or asserts on the returned exam object to ensure the exam was correctly created before it’s used in subsequent steps. This will help avoid silent test failures if “prepareExam” ever fails.
111-111: Double-check “prepareExam” with the 2-round feature
When passing “2” as the numberOfCorrectionRounds, confirm that the exam is indeed configured for multiple assessment rounds. This might require verifying the relevant database or UI fields if these additional rounds are incorrectly set.
src/test/playwright/e2e/exam/ExamChecklists.spec.ts (5)
1-16: Great addition of comprehensive E2E coverage
The newly introduced “ExamChecklists.spec.ts” significantly improves coverage. It clearly tests each checklist item’s status and transitions. The step-by-step approach is well-structured.
52-69: Watch the loop iteration on exam.numberOfExercisesInExam
You loop from index 0 to exam.numberOfExercisesInExam! for adding exercise groups, then add additional groups. Confirm your logic around “NUMBER_OF_EXERCISE_GROUPS” to ensure you don’t create an off-by-one scenario or inadvertently skip the last item.
142-150: Validate student registration flow
After registering “at least one student,” confirm that the UI properly updates. If there are multiple students, you might need to confirm all are updated or only partial.
[approve]
167-187: Await the exam creation tasks before checking checklist
Sometimes the server might take a bit to generate individual exams. If your environment is slow, consider verifying the success of these actions (like ensuring the DB was updated) before re-checking the checklist.
215-229: Confirm race conditions between finishing exam and finalizing assessments
Using “startAssessing” after “finishExam” is valid, but check real usage. If finishing an exam triggers background processes, you may need an additional wait or check to avoid race conditions.
src/main/webapp/app/shared/components/checklist-check/checklist-check.component.html (1)
1-1: LGTM! Proper usage of new Angular control flow syntax.
The @if/@else syntax follows the new Angular guidelines correctly, replacing the older *ngIf directive.
Also applies to: 3-3
src/test/playwright/support/pageobjects/exam/ExamDetailsPage.ts (1)
77-92: LGTM! Well-structured enum for checklist items.
The ExamChecklistItem enum provides good type safety and clear documentation of all possible checklist items.
src/test/playwright/support/pageobjects/exam/ExamCreationPage.ts (1)
54-73: LGTM! Well-structured date-setting methods.
The new methods maintain consistency with existing patterns and are properly documented.
src/test/playwright/e2e/exam/ExamCreationDeletion.spec.ts (1)
23-34: LGTM! Improved test isolation.
Moving exam data definitions into test scopes and using local exam variables improves test isolation and maintainability.
Also applies to: 69-70, 74-76, 89-90, 94-97
src/test/playwright/support/requests/ExerciseAPIRequests.ts (1)
281-281: LGTM! The new parameter enhances flexibility.
The addition of createNewSubmission parameter provides good control over submission behavior.
src/main/webapp/app/exam/manage/exams/exam-checklist-component/exam-checklist.component.html (3)
Line range hint 53-94: LGTM! Exercise group validation checks are well identified.
The data-testid attributes for exercise group checks follow a consistent naming pattern and cover all critical validations.
146-146: LGTM! Student management elements are properly identified.
The data-testid attributes for student registration check and related navigation buttons are well placed and consistently named.
Also applies to: 157-208
Line range hint 471-605: LGTM! Assessment validation checks are well identified.
The data-testid attributes for assessment-related checks follow a consistent pattern and cover all critical validations.
|
@SimonEntholzer the skipped tests currently fail because of a bug. I created GitHub issues for them and added comments to the skipped tests to make the intention clear. |
muradium
dismissed stale reviews from SimonEntholzer and coderabbitai[bot]
via
6ac351a
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/playwright/e2e/exam/ExamChecklists.spec.ts (4)
25-139: Consider extracting common verification patterns.The exercise group tests are well-structured but contain repeated patterns of navigation and verification. Consider:
- Creating a helper function for the common pattern of "navigate and verify checklist item state"
- Adding descriptive messages to assertions for better debugging
Example helper function:
async function verifyChecklistItem( page: Page, course: Course, exam: Exam, examDetails: ExamDetailsPage, item: ExamChecklistItem, expectedState: 'checked' | 'unchecked', message?: string ) { await navigateToExamDetailsPage(page, course, exam); if (expectedState === 'checked') { await examDetails.checkItemChecked(item, message); } else { await examDetails.checkItemUnchecked(item, message); } }This would simplify the tests and make them more maintainable.
141-150: Consider adding negative test cases for student registration.While the happy path is covered, consider adding test cases for:
- Attempting to register a student who is already registered
- Attempting to register a student who is not in the course
- Edge cases around registration deadlines
190-213: Consider additional date-related test cases.The current test covers the happy path, but consider adding:
- Validation for invalid date combinations (e.g., review start date before exam end)
- Tests for timezone edge cases
- Tests for date modifications after exam creation
281-308: Add JSDoc documentation to helper functions.The helper functions are well-structured but would benefit from JSDoc documentation to improve maintainability and IDE support.
Example documentation:
/** * Creates an exam with the specified configuration * @param course - The course to create the exam in * @param page - The Playwright page instance * @param customConfig - Optional custom configuration to override defaults * @returns The created exam instance */ async function createExam(course: Course, page: Page, customConfig?: any) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/playwright/e2e/exam/ExamChecklists.spec.ts(1 hunks)
🔇 Additional comments (2)
src/test/playwright/e2e/exam/ExamChecklists.spec.ts (2)
16-24: LGTM! Well-structured test setup.
The test suite follows best practices by using API calls for setup, properly managing test resources, and following the Arrange-Act-Assert pattern.
232-237: Monitor skipped tests and their associated issues.
Two tests are currently skipped due to known issues:
- Issue Exam check for unassessed quizzes not working for instructors #10074: Quiz submission assessment
- Issue Exam check for unsubmitted exercises not working for instructors #10076: Late exam submission handling
Consider setting up automated monitoring of these issues to re-enable the tests once fixed.
Also applies to: 248-274
✅ Verification successful
Both referenced issues are still open - continue skipping the tests
The verification confirms that GitHub issues #10074 and #10076 are still open, justifying the current test skip directives. Continue monitoring these issues:
- Exam check for unassessed quizzes not working for instructors #10074: Quiz submission assessment
- Exam check for unsubmitted exercises not working for instructors #10076: Late exam submission handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the issues are still open
gh issue view 10074 --json state
gh issue view 10076 --json state
Length of output: 103
Checklist
General
Client
Motivation and Context
Functionality of the exam checklist is not being tested from the user perspective.
Description
Adds E2E tests that assert the states of all checks on exam checklist during various scenarios.
Steps for Testing
Steps for running the tests:
npm install && npm run playwright:setupnpm run playwright:opento open the Playwright UI, search for the "Exam Checklists" test suite and run all tests of itReview Progress
Code Review
Manual Tests
Summary by CodeRabbit
New Features
data-testidattributes in various components.Bug Fixes
Documentation
Chores