test(j-s): Regression E2E for court's handling of indictment cases #17457
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request focuses on enhancing the indictment tests in the judicial system e2e test suite. The changes include updating test helper functions, modifying the test flow for indictment cases, and adding more comprehensive UI interaction checks. Key modifications involve introducing new helper functions like Changes
Possibly related PRs
Suggested labels
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
|
unakb
changed the title
![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 (6)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts (4)
105-105: *Typo in label text 'LÖKE málsnúmer Veldu málsnú'The label text
'LÖKE málsnúmer *Veldu málsnú'appears to be incomplete or contain a typo in the word'málsnú'. Please verify and correct the label text to ensure it displays accurately.
126-126: Typo in label text 'Krefjast sviptingarKrefjast'The label text
'Krefjast sviptingarKrefjast'seems to have a duplicated word'Krefjast'. Please check the text and correct it to avoid repetition.
234-234: Incomplete label text 'Ákærði óskar ekki eftir að sé'The label text
'Ákærði óskar ekki eftir að sé'looks incomplete and may be missing some words at the end. Please verify and update the label text for clarity and completeness.
253-257: Use more specific selectors instead of.nth()Using
.nth()to select elements can make tests brittle if the order of elements changes. Consider using more specific selectors or test IDs to target the upload buttons for documents to enhance test reliability.Also applies to: 263-267
apps/system-e2e/src/tests/judicial-system/utils/helpers.ts (2)
8-12: Ensure consistent formatting for court case numbersThe function
randomCourtCaseNumbermay generate numbers like'R-0/2025', which might not be desirable. To maintain consistency, consider padding the random number with leading zeros to ensure a fixed length, such as three digits.You can modify the function as follows:
export const randomCourtCaseNumber = (prefix?: string) => { - return `${prefix ?? 'R'}-${Math.floor( - Math.random() * 1000, - )}/${new Date().getFullYear()}` + const randomNumber = Math.floor(Math.random() * 1000) + const formattedNumber = randomNumber.toString().padStart(3, '000') + return `${prefix ?? 'R'}-${formattedNumber}/${new Date().getFullYear()}` }
Line range hint
44-47: Avoid using fixed timeouts; wait for specific events insteadIn the
chooseDocumentfunction,await page.waitForTimeout(1000)introduces an unnecessary delay and may lead to flaky tests. It's better to wait for specific events or conditions to ensure proper synchronization without arbitrary timeouts.You can remove the timeout and rely on the file chooser event:
export const chooseDocument = async ( page: Page, clickButton: () => Promise<void>, fileName: string, ) => { const fileChooserPromise = page.waitForEvent('filechooser') await clickButton() const fileChooser = await fileChooserPromise - await page.waitForTimeout(1000) await fileChooser.setFiles(createFakePdf(fileName)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts(5 hunks)apps/system-e2e/src/tests/judicial-system/utils/helpers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/system-e2e/src/tests/judicial-system/utils/helpers.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts (1)
217-217: Verify date format for 'courtDate' inputEnsure that the date string returned by
getDaysFromNow(7)matches the expected format for thecourtDateinput field. The date picker might require a specific format such as'dd.MM.yyyy'. Adjust the date format if necessary to prevent potential issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts (2)
77-132: Consider enhancing assertions in the processing and indictment sections.While the test flow is good, consider adding these improvements:
- Add assertions for error states in form submissions
- Verify the state of UI elements after each transition
- Add data validation checks for the indictment form
Example assertion for form validation:
await expect(page.getByTestId('form-errors')).toHaveCount(0, 'Form should be valid');
Line range hint
1-287: Consider implementing a more robust test architecture.While the implementation is good, consider these architectural improvements:
- Create shared test fixtures for common setup
- Implement custom test assertions for domain-specific validations
- Add test data factories for generating consistent test data
Example fixture:
export const indictmentFixture = base.extend({ setupIndictment: async ({ prosecutorPage }, use) => { // Common setup code await use(/* setup result */); }, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (3)
apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts (3)
9-10: LGTM! Helper functions imported for court case handling.The new imports align well with the PR objectives for testing the court's handling of indictment cases.
25-26: LGTM! Good practice moving case number generation to setup.Moving the police case number generation to the beginning improves test maintainability and ensures consistency across the test suite.
249-268: Enhance document upload validation and error handling.While the document upload implementation works, consider adding:
- File type validation
- File size limits
- Upload failure handling
Run this script to check existing file validation:
| test('judge should receive indictment case', async ({ judgePage }) => { | ||
| const page = judgePage | ||
| const nextWeek = getDaysFromNow(7) | ||
|
|
||
| // Case list | ||
| await page.goto('/krofur') | ||
| await page.getByText(accusedName).click() | ||
|
|
||
| // Indictment Overview | ||
| await expect(page).toHaveURL(`domur/akaera/yfirlit/${caseId}`) | ||
|
|
||
| await Promise.all([ | ||
| page.getByTestId('continueButton').click(), | ||
| verifyRequestCompletion(page, '/api/graphql', 'Case'), | ||
| ]) | ||
|
|
||
| // Reception and assignment | ||
| await expect(page).toHaveURL(`domur/akaera/mottaka/${caseId}`) | ||
|
|
||
| await page | ||
| .getByTestId('courtCaseNumber') | ||
| .fill(randomIndictmentCourtCaseNumber()) | ||
| await page.keyboard.press('Tab') | ||
|
|
||
| await page.getByText('Veldu dómara/aðstoðarmann').click() | ||
| await page | ||
| .getByTestId('select-judge') | ||
| .getByText('Test Dómari') | ||
| .last() | ||
| .click() | ||
|
|
||
| await Promise.all([ | ||
| page.getByTestId('continueButton').click(), | ||
| verifyRequestCompletion(page, '/api/graphql', 'Case'), | ||
| ]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case coverage for judge's workflow.
The happy path is well tested, but consider adding tests for:
- Invalid court case number format
- Missing judge assignment
- Failed API responses
Example test case:
test('should handle invalid court case number', async ({ judgePage }) => {
await judgePage.getByTestId('courtCaseNumber').fill('invalid');
await expect(judgePage.getByTestId('error-message')).toBeVisible();
});
There was a problem hiding this comment.
Nice 👏 The rabbit is also proposing to test some failure cases instead of only the valid flow (or happy path) for some fields, which could be useful for fields such as the court case number etc. But I will leave it up to you to decide, as I don't think this is a blocker ✅
E2E test fyrir ákærur - dómari móttekur, klárar og sendir til Ríksak
What
Added a regression test for the judges step of an indictment. It handles the receiving of an indictment case, assigns it to a judge and sends the subpoena. Then proceeds to complete a ruling in the case and sends it to the public prosecutor.
Why
So we can have an automated test for this step of an indictment case. We now have the following steps covered:
Screenshots / Gifs
E2E.court.mov
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests