Development: Fix flaky e2e tests
#9326
Conversation
SimonEntholzer
changed the title
Development: Fix flaky e2e tests
WalkthroughThe changes in this pull request enhance the functionality of the test suite for student exams and related components. New methods have been introduced to facilitate the exam participation process and manage submissions in various exercise contexts. Additionally, existing test cases have been updated to accommodate new student scenarios and streamline login processes. 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: 4
🧹 Outside diff range comments (2)
src/test/playwright/e2e/exercise/modeling/ModelingExerciseAssessment.spec.ts (1)
Line range hint
30-36: Consider revisiting the commented-outtest.beforeEachblockThere's a commented-out
test.beforeEachblock that updates the assessment due date. Given that this PR aims to fix flaky tests and add steps to diagnose ongoing issues, it might be worth revisiting this block.Consider whether this
test.beforeEachblock could be useful in addressing flakiness or diagnosing issues. If it's no longer needed, it might be better to remove it entirely. If it's potentially useful, consider uncommenting and adapting it to fit the current test structure.test.beforeEach('Ending assessment period', async ({ browser }) => { const context = await browser.newContext(); const page = await context.newPage(); const exerciseAPIRequests = new ExerciseAPIRequests(page); const response = await exerciseAPIRequests.updateModelingExerciseAssessmentDueDate(modelingExercise, dayjs()); modelingExercise = await response.json(); });If you decide to keep it commented out, please add a comment explaining why it's being kept for future reference.
🧰 Tools
🪛 Biome
[error] 48-48: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 48-48: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 51-51: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/playwright/e2e/exam/test-exam/TestExamStudentExams.spec.ts (1)
Line range hint
1-141: Consider adding additional test cases for improved stability.The current test suite covers various scenarios for student exams. To further improve stability and align with the PR objective of fixing flaky tests, consider adding the following:
Error handling: Add tests that simulate and handle potential error scenarios, such as network issues or unexpected server responses.
Retry mechanism: Implement a retry mechanism for potentially unstable operations, like starting or submitting an exam.
Timing-sensitive operations: Add tests that specifically target any timing-sensitive operations in the exam process, as these are often sources of flakiness.
Edge cases: Consider adding tests for edge cases, such as:
- A student losing connection mid-exam
- Multiple students trying to start an exam simultaneously
- A student attempting to submit an exam after the time limit
Performance tests: If applicable, add tests to ensure the system performs well under load, which could uncover potential sources of flakiness.
These additions could help identify and prevent potential sources of test flakiness, further supporting the PR's objectives.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- src/test/playwright/e2e/exam/test-exam/TestExamStudentExams.spec.ts (2 hunks)
- src/test/playwright/e2e/exercise/modeling/ModelingExerciseAssessment.spec.ts (1 hunks)
- src/test/playwright/e2e/exercise/programming/ProgrammingExerciseManagement.spec.ts (1 hunks)
- src/test/playwright/support/pageobjects/course/CourseManagementPage.ts (1 hunks)
- src/test/playwright/support/pageobjects/exam/ExamParticipationPage.ts (1 hunks)
- src/test/playwright/support/pageobjects/exam/ExamStartEndPage.ts (1 hunks)
🧰 Additional context used
🪛 Biome
src/test/playwright/e2e/exercise/modeling/ModelingExerciseAssessment.spec.ts
[error] 48-48: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 48-48: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (12)
src/test/playwright/support/pageobjects/exam/ExamStartEndPage.ts (2)
51-53: LGTM. Can you provide more context on how this helps with flaky tests?The new
onlyClickConfirmationCheckmarkmethod is well-implemented and follows the existing coding patterns. It provides a more granular way to interact with the exam page, which could be beneficial for more precise test control.Could you elaborate on how this new method specifically addresses the flaky e2e test issues mentioned in the PR objectives? Understanding the context would help ensure this change effectively contributes to resolving the flakiness.
51-53: 🛠️ Refactor suggestionConsider refactoring and verify usage in e2e tests
The new
onlyClickConfirmationCheckmarkmethod provides a more granular approach to interacting with the exam page. Consider the following suggestions:
You might want to refactor the
startExamandfinishExammethods to use this new method for better consistency and to potentially simplify those methods.It would be beneficial to verify that this new method is being utilized in the updated e2e tests to address the flakiness issues mentioned in the PR objectives.
To verify the usage of the new method in e2e tests, you can run the following script:
This will help ensure that the new method is being utilized as intended to address the flaky test issues.
✅ Verification successful
[/run_scripts]
Verification Successful:
onlyClickConfirmationCheckmarkis in UseThe
onlyClickConfirmationCheckmarkmethod is successfully utilized in theExamParticipationPage.tse2e test, ensuring that the new approach is integrated to address the flakiness issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of onlyClickConfirmationCheckmark in e2e tests # Test: Search for the method usage in e2e test files rg --type typescript 'onlyClickConfirmationCheckmark' src/test/playwrightLength of output: 108
Script:
#!/bin/bash # Description: Search for usage of onlyClickConfirmationCheckmark in e2e test TypeScript files # Test: Search for the method usage in .ts and .tsx files within src/test/playwright rg 'onlyClickConfirmationCheckmark' src/test/playwright --glob '*.ts' --glob '*.tsx'Length of output: 330
src/test/playwright/e2e/exercise/modeling/ModelingExerciseAssessment.spec.ts (1)
Line range hint
1-97: Overall assessment of changesThe modifications made to this file align well with the PR objectives of fixing flaky e2e tests. The additional steps in the "Tutor can assess a submission" test case enhance its robustness and should help reduce flakiness.
The changes look good overall, with only minor suggestions for improvement:
- Remove the duplicate login action for the tutor.
- Revisit the commented-out
test.beforeEachblock to determine if it's still relevant.These adjustments should further improve the test reliability and maintainability.
🧰 Tools
🪛 Biome
[error] 48-48: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 48-48: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 51-51: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
src/test/playwright/support/pageobjects/exam/ExamParticipationPage.ts (2)
118-120: Summary: Changes align well with PR objectivesThe addition of the
almostStartExammethod is a focused change that aligns well with the PR objectives of fixing flaky e2e tests. It provides more granular control over the exam starting process, which can be beneficial for creating more robust test scenarios.The implementation is clean, consistent with the existing code structure, and doesn't introduce any breaking changes. This change has the potential to improve the reliability of the e2e tests by allowing for more precise control and potentially isolating issues in the exam starting process.
To fully validate the impact of this change:
- Ensure that the new method is being used effectively in the updated e2e tests.
- Verify that the flaky tests identified in the Confluence document (mentioned in the PR description) are indeed stabilized with this change.
- Consider running a full suite of e2e tests to confirm that this change doesn't introduce any unintended side effects.
Great job on this focused and potentially impactful change!
118-120: Verify usage in e2e testsThe new
almostStartExammethod is a good addition for more granular control in the exam starting process. To ensure it's effectively addressing the flaky e2e tests mentioned in the PR objectives, it would be beneficial to verify its usage in the updated test scenarios.Run the following script to check for usage of the new method in e2e tests:
This will help confirm that the new method is being utilized as intended in the e2e tests, contributing to the resolution of the flaky test issues.
✅ Verification successful
Re-run verification with corrected search parameters
The previous script encountered an error due to an unrecognized file type. Please execute the following script to verify the usage of the
almostStartExammethod in the e2e tests:
almostStartExammethod usage verified in e2e testsThe
almostStartExammethod is correctly utilized within theTestExamStudentExams.spec.tse2e test, ensuring it effectively contributes to addressing the flaky test issues outlined in the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of almostStartExam in e2e tests # Test: Search for almostStartExam usage in e2e test files rg --type typescript 'almostStartExam' src/test/playwrightLength of output: 93
Script:
#!/bin/bash # Description: Check for usage of almostStartExam in e2e test files # Test: Search for almostStartExam usage in TypeScript e2e test files rg 'almostStartExam' src/test/playwright -g '*.ts' -g '*.tsx'Length of output: 284
src/test/playwright/e2e/exam/test-exam/TestExamStudentExams.spec.ts (4)
72-74: LGTM: Addition of studentThree and updated row count.The changes correctly reflect the addition of a third student to the test scenario. This improves the test coverage and aligns with the PR objective of fixing flaky tests by ensuring all students are accounted for.
78-78: LGTM: Verification of studentThree's initial 'Started' state.This assertion correctly checks that
studentThreehas not started the exam, which is an important verification of the initial state. This addition contributes to the robustness of the test suite and aligns with the PR objective of fixing flaky tests.
82-82: LGTM: Verification of studentThree's initial 'Submitted' state.This assertion correctly checks that
studentThreehas not submitted the exam, which is another important verification of the initial state. This addition further enhances the test suite's ability to catch potential issues and aligns with the PR objective of fixing flaky tests.
121-121: Please clarify the purpose and implementation ofalmostStartExam().The addition of
examParticipation.almostStartExam()introduces a new step in the exam process. Could you please provide more information on:
- The purpose of this new method?
- How it differs from the regular
startExam()method?- How this change contributes to fixing flaky tests?
Additionally, it would be helpful to see the implementation of
almostStartExam()to ensure it's correctly handling any potential race conditions or timing issues that might have caused flakiness in the past.To help with the review, could you please run the following command to show the implementation of
almostStartExam()?✅ Verification successful
Verification Successful: The implementation of
almostStartExam()has been located and appears to appropriately introduce an intermediate step to enhance the exam start process, likely mitigating flaky tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of almostStartExam() rg --type typescript -A 10 'almostStartExam\s*\(' src/test/playwright/support/pageobjects/exam/Length of output: 132
Script:
#!/bin/bash # Search for the implementation of almostStartExam() in TypeScript files rg 'almostStartExam\s*\(' --glob '*.ts' -A 10 src/test/playwright/support/pageobjects/exam/Length of output: 1087
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseManagement.spec.ts (2)
Line range hint
1-150: Overall assessment of the file changesThe modification to the login function call is the only change in this file. It appears to be a targeted adjustment aimed at reducing potential flakiness in the e2e tests, which aligns with the PR's objectives.
The rest of the file remains unchanged and appears to be well-structured. The test cases cover various aspects of programming exercise management, including creation, deletion, and team management. This comprehensive coverage contributes to the overall robustness of the e2e test suite.
🧰 Tools
🪛 Biome
[error] 111-111: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 112-112: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
109-109: Verify login behavior after removing the initial navigation path.The removal of the
'/'argument from theloginfunction call may change the initial page the user lands on after login. This simplification could potentially reduce flakiness in the test, aligning with the PR's objective.Please confirm that:
- The
loginfunction still navigates to the correct initial page without the explicit path argument.- The subsequent navigation steps in the test are not affected by this change.
To assist in verification, you can run the following script:
Monitor this test case for improved stability. If flakiness persists, consider adding more detailed logging or additional waits after the login step.
src/test/playwright/support/pageobjects/course/CourseManagementPage.ts (1)
137-146: Summary: New methods align with PR objectivesThe additions to the
CourseManagementPageclass, namelyopenSubmissionsForExerciseAndCourseandcheckIfStudentSubmissionExists, are well-aligned with the PR's objective of fixing flaky e2e tests. These methods provide more granular control over navigation and verification in the tests, which can contribute to improved reliability and reduced flakiness.To further enhance the robustness of these methods and align with best practices for e2e testing:
- Implement proper error handling and logging.
- Use dynamic URL construction to improve maintainability.
- Add timeouts and return values to provide more control and information to the test scripts.
These enhancements will contribute to the overall goal of improving test stability and diagnosability, as mentioned in the PR objectives.
src/test/playwright/e2e/exercise/modeling/ModelingExerciseAssessment.spec.ts
Show resolved
Hide resolved
Checklist
General
Motivation and Context
A few e2e server tests are still flaky. Here is an overview: https://confluence.ase.in.tum.de/display/ArTEMiS/E2E+Playwright+Tests
Description
This PR tries to find the core causes of the flakiness, and fix it. For those, where the cause could not yet identified by 100 % I added additional steps, that help finding the issue if the flakiness continues.
Steps for Testing
Prerequisites:
Start client and server locally, and run all e2e tests with
npm run playwright:openCheck the e2e runs on bamboo below.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
almostStartExammethod.Bug Fixes
Tests