Development: Improve exercise import e2e tests
#10305
Conversation
…ementation is too specific on each inheritor
WalkthroughThe changes enhance Playwright tests and page objects for exercise import and management. In the test suites, explicit waits using Changes
Suggested labels
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
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: 1
🧹 Nitpick comments (6)
src/test/playwright/support/pageobjects/exercises/AbstractExerciseCreationPage.ts (1)
30-34: Efficient text clearing method.Using “Control+a” could pose a cross-platform nuance (e.g., macOS might need “Command+a”). Consider verifying or adding a fallback if needed.
src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts (1)
20-23: Consider extracting common text editor interaction logic.The methods
typeProblemStatement,typeExampleSolution, andtypeAssessmentInstructionsshare similar text editor interaction patterns. Consider extracting this common logic into a protected method in the parent class.+ protected async typeInEditor(selector: string, text: string) { + const textEditor = this.getTextEditorLocator(selector); + await this.typeText(textEditor, text); + } - async typeProblemStatement(statement: string) { - const textEditor = this.getTextEditorLocator(this.PROBLEM_STATEMENT_SELECTOR); - await this.typeText(textEditor, statement); - } + async typeProblemStatement(statement: string) { + await this.typeInEditor(this.PROBLEM_STATEMENT_SELECTOR, statement); + } // Apply similar changes to typeExampleSolution and typeAssessmentInstructionsAlso applies to: 30-33, 40-43
src/test/playwright/support/pageobjects/exercises/quiz/QuizExerciseCreationPage.ts (1)
65-69: Consider using Playwright's built-in drag and drop API.The current implementation uses low-level mouse events which could be fragile. Playwright provides a more reliable built-in drag and drop API.
- await this.page.mouse.move(boundingBox!.x + 800, boundingBox!.y + 10); - await this.page.mouse.down(); - await this.page.mouse.move(boundingBox!.x + 1000, boundingBox!.y + 150); - await this.page.mouse.up(); + await element.dragTo(element, { + targetPosition: { x: 1000, y: 150 }, + sourcePosition: { x: 800, y: 10 } + });src/test/playwright/support/pageobjects/exercises/programming/ProgrammingExerciseCreationPage.ts (2)
65-69: Consider using Playwright's waitForLoadState instead of fixed timeout.The current implementation uses a fixed timeout which could be flaky. Consider using Playwright's built-in load state detection.
- await this.page.locator('.owl-dt-control-content.owl-dt-control-button-content', { hasText: 'Set' }).dispatchEvent('click'); - await this.page.waitForTimeout(500); + await this.page.locator('.owl-dt-control-content.owl-dt-control-button-content', { hasText: 'Set' }).click(); + await this.page.waitForLoadState('networkidle');
77-94: Improve scroll detection with IntersectionObserver.The current scroll detection implementation could be improved using IntersectionObserver for more reliable results.
async waitForPageToFinishScrolling(maxTimeout: number = 5000) { const elementOnPageAffectedByScroll = this.page.locator('h2'); - let isScrolling = true; - const startTime = Date.now(); - - while (isScrolling) { - const initialPosition = await elementOnPageAffectedByScroll.boundingBox(); - await this.page.waitForTimeout(100); // give the page a short time to scroll - const newPosition = await elementOnPageAffectedByScroll.boundingBox(); - - isScrolling = initialPosition?.y !== newPosition?.y; - - const isWaitingForScrollExceedingTimeout = Date.now() - startTime > maxTimeout; - if (isWaitingForScrollExceedingTimeout) { - throw new Error(`Aborting waiting for scroll end - page is still scrolling after ${maxTimeout}ms`); - } - } + await this.page.evaluate( + ([timeout]) => { + return new Promise((resolve, reject) => { + let lastY = window.scrollY; + const timer = setTimeout(() => reject(new Error(`Scroll timeout after ${timeout}ms`)), timeout); + const observer = new IntersectionObserver(() => { + if (window.scrollY === lastY) { + clearTimeout(timer); + observer.disconnect(); + resolve(true); + } + lastY = window.scrollY; + }); + observer.observe(document.querySelector('h2')!); + }); + }, + [maxTimeout] + ); }src/test/playwright/e2e/exercise/ExerciseImport.spec.ts (1)
41-71: Consider adding negative test cases.The test coverage could be enhanced by adding test cases for:
- Import failures due to network issues
- Form validation errors
- Concurrent imports
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/test/playwright/e2e/exercise/ExerciseImport.spec.ts(4 hunks)src/test/playwright/e2e/exercise/file-upload/FileUploadExerciseManagement.spec.ts(1 hunks)src/test/playwright/support/fixtures.ts(3 hunks)src/test/playwright/support/pageobjects/exercises/AbstractExerciseCreationPage.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/file-upload/FileUploadExerciseCreationPage.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/modeling/ModelingExerciseCreationPage.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/programming/ProgrammingExerciseCreationPage.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/quiz/QuizExerciseCreationPage.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (17)
src/test/playwright/support/pageobjects/exercises/file-upload/FileUploadExerciseCreationPage.ts (2)
2-2: Good addition of the abstract page import.Bringing in
AbstractExerciseCreationPagepromotes shared functionality and reduces duplication across exercise creation pages.
4-4: Extending the abstract page for better maintainability.By inheriting from
AbstractExerciseCreationPage, this class benefits from centralized methods (e.g., form handling), simplifying future code updates.src/test/playwright/support/pageobjects/exercises/modeling/ModelingExerciseCreationPage.ts (2)
2-2: Importing the abstract page for standardization.This ensures consistency with other exercise creation pages that also rely on shared logic from
AbstractExerciseCreationPage.
4-4: Refactoring to extend the abstract page.Extending
AbstractExerciseCreationPagecentralizes common form interactions and fosters code reuse across various exercise types.src/test/playwright/support/pageobjects/exercises/AbstractExerciseCreationPage.ts (8)
1-3: Imports appear appropriate and concise.Using
@playwright/test,dayjs, and a local utility (enterDate) for date handling aligns well with the project’s E2E testing strategy.
5-7: Well-structured class declaration and dependency setup.Declaring
pageas a protected property neatly encapsulates it while allowing subclasses to leverage shared functionality.
8-11: Constructor ensures a consistent page reference.Accepting a
Pageobject in the constructor and storing it for subclass use is a clean approach to page object design.
12-16: Robust title setter.Clearing and then filling the title field avoids stale data, reducing risk of flaky tests. The flow is readable and straightforward.
18-20: Release date handling is properly encapsulated.Relying on
enterDatecentralizes date-typing logic. Good approach to maintain consistency across multiple exercise creation flows.
22-24: Due date handling mirrors release date setting.This parallel structure reduces confusion and makes test code easier to follow.
26-28: Assessment due date handling remains consistent.Keeping the same approach for all date fields eliminates discrepancies in test logic.
36-38: Reliable form-load wait strategy.Explicitly waiting for the edit form to become visible improves test stability, especially in slower or multi-node environments.
src/test/playwright/e2e/exercise/file-upload/FileUploadExerciseManagement.spec.ts (1)
27-27: LGTM!The change from
typeTitletosetTitlealigns with the standardized method naming across exercise creation pages.src/test/playwright/support/pageobjects/exercises/quiz/QuizExerciseCreationPage.ts (1)
7-7: LGTM!The inheritance from
AbstractExerciseCreationPageappropriately centralizes common exercise creation functionality.src/test/playwright/support/pageobjects/exercises/programming/ProgrammingExerciseCreationPage.ts (1)
8-8: LGTM!The inheritance from
AbstractExerciseCreationPageappropriately centralizes common exercise creation functionality.src/test/playwright/e2e/exercise/ExerciseImport.spec.ts (1)
47-47: LGTM! Improved test reliability with form load synchronization.The addition of
waitForFormToLoad()calls before form validation is a great improvement. This ensures that the tests wait for the exercise information to be fully loaded before proceeding with assertions, addressing the flaky test issues in multi-node configurations.Also applies to: 78-78, 105-105, 136-136
src/test/playwright/support/fixtures.ts (1)
40-40: LGTM! Consistent naming convention.The renaming of
CreateModelingExercisePagetoModelingExerciseCreationPagealigns with the naming convention used for other exercise creation pages in the codebase.Also applies to: 118-118, 294-296
src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts (2)
10-12: Consider using the inheritedsetPointsmethod.For consistency with other exercise creation pages, consider using the inherited
setPointsmethod instead oftypeMaxPoints.- async typeMaxPoints(maxPoints: number) { - await this.page.locator('#field_points').fill(maxPoints.toString()); - }
60-63: Consider using Playwright'sfillmethod for text input.Instead of using
pressSequentially, consider using Playwright'sfillmethod which is more reliable for text input.- private async typeText(textEditor: Locator, text: string) { - await textEditor.click(); - await textEditor.pressSequentially(text); - } + private async typeText(textEditor: Locator, text: string) { + await textEditor.fill(text); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/playwright/e2e/exam/ExamManagement.spec.ts(1 hunks)src/test/playwright/e2e/exam/test-exam/TestExamManagement.spec.ts(1 hunks)src/test/playwright/e2e/exercise/text/TextExerciseManagement.spec.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyse
🔇 Additional comments (4)
src/test/playwright/support/pageobjects/exercises/text/TextExerciseCreationPage.ts (1)
1-5: LGTM! Good refactoring to use inheritance.The class now extends
AbstractExerciseCreationPage, which helps centralize common functionality and reduce code duplication.src/test/playwright/e2e/exercise/text/TextExerciseManagement.spec.ts (1)
35-35: LGTM! Method name standardization.The change from
typeTitletosetTitlealigns with the standardized method naming across exercise creation pages.src/test/playwright/e2e/exam/test-exam/TestExamManagement.spec.ts (1)
54-54: LGTM! Consistent method naming.The changes from
typeTitletosetTitlemaintain consistency with the standardized method naming convention.Also applies to: 80-81
src/test/playwright/e2e/exam/ExamManagement.spec.ts (1)
42-42: LGTM! Consistent method naming.The change from
typeTitletosetTitlemaintains consistency with the standardized method naming convention.
Checklist
General
Client
Motivation and Context
While importing an exercise, loading exercise information and displaying it on exercise create/modify form may sometimes take longer. This was causing relevant E2E tests to be flaky, especially on multi-node configuration.
Description
This PR improves the waiting mechanism during execution of E2E tests. It waits for the exercise information to be loaded before making any assertions. It also removes some code repetition by small refactoring.
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Code Review
Summary by CodeRabbit