feat(tests): Use new e2e testing lib for imports + move some system-e2e tests to apps #16387
Conversation
WalkthroughThe changes in this pull request involve a comprehensive restructuring of the exports in the 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: 32
🧹 Outside diff range and nitpick comments (24)
libs/testing/e2e/src/lib/utils/pageHelpers.ts (3)
1-1: Approve with suggestion: Consider input sanitizationThe
getInputByNamefunction is well-defined, typed correctly, and follows the coding guidelines for reusability and tree-shaking. However, directly interpolating thenameparameter into the string could potentially lead to XSS vulnerabilities if the input is not properly sanitized.Consider adding input sanitization to prevent potential XSS attacks. Here's a suggested improvement:
import { escapeSelector } from 'css.escape'; export const getInputByName = (name: string) => `input[name="${escapeSelector(name)}"]`;This change ensures that any special characters in the
nameparameter are properly escaped, preventing potential CSS injection attacks.
2-2: Approve with suggestion: Consider input sanitizationThe
getTextareaByNamefunction is well-defined, typed correctly, and adheres to the coding guidelines for reusability and tree-shaking. However, as with thegetInputByNamefunction, there's a potential security risk in directly interpolating thenameparameter into the string without sanitization.To mitigate potential XSS vulnerabilities, consider implementing input sanitization. Here's a suggested improvement:
import { escapeSelector } from 'css.escape'; export const getTextareaByName = (name: string) => `textarea[name="${escapeSelector(name)}"]`;This change ensures that any special characters in the
nameparameter are properly escaped, preventing potential CSS injection attacks.
1-2: Overall assessment: Well-structured utility functions with room for security enhancementThe
pageHelpers.tsfile introduces two utility functions that are well-defined, typed correctly, and adhere to the coding guidelines for thelibsdirectory. They are reusable across different NextJS apps, make proper use of TypeScript, and are exported in a way that supports effective tree-shaking.However, both functions could benefit from input sanitization to prevent potential XSS vulnerabilities. Consider implementing the suggested improvements to enhance the security of these helper functions.
To further improve the modularity and reusability of these functions, consider creating a separate utility file for input sanitization. This would allow you to use the same sanitization logic across multiple helper functions and potentially in other parts of your application.
libs/testing/e2e/src/lib/support/i18n.ts (1)
1-13: LGTM with a suggestion for improvementThe internationalization setup looks good and adheres to the coding guidelines. It's reusable, uses TypeScript, and follows good practices for tree-shaking. The error handling approach is sensible.
However, consider extracting the locale ('is') into a constant or configuration file. This would improve maintainability and allow for easier locale changes in the future.
You could refactor it like this:
import { LOCALE } from './config' // ... rest of the code ... const intl = createIntl( { locale: LOCALE, // ... rest of the options ... }, cache, )libs/testing/e2e/src/lib/support/locator-helpers.ts (2)
4-10: LGTM:locatorByRolefunction is well-implemented.The function is reusable, type-safe, and adheres to the coding guidelines for the
libsdirectory. It handles different input types effectively and returns a properly formatted locator string.Consider using a template literal for better readability:
export const locatorByRole = ( role: Roles | string, name: string | { name: string }, ): string => `role=${role}[name="${typeof name === 'string' ? name : name.name}"]`This suggestion combines the conditional logic into a single template literal, potentially making the code more concise and easier to read.
11-23: LGTM:helpersfunction provides useful utility methods.The
helpersfunction is well-structured, reusable, and type-safe. It adheres to the coding guidelines for thelibsdirectory and provides valuable utility methods for Playwright tests.For consistency, consider using arrow function syntax for all methods in the returned object:
export const helpers = (page: Page) => ({ findByRole: (role: Roles | string, name: string | { name: string }): Locator => page.locator(locatorByRole(role, name)), findByTestId: (name: string): Locator => page.locator(`[data-testid="${name}"]`), proceed: async (): Promise<void> => await page.locator('[data-testid="proceed"]').click(), })This suggestion makes the syntax consistent across all methods and explicitly defines the return type for
proceed.libs/testing/e2e/src/lib/support/utils.ts (3)
1-5: LGTM with a minor suggestion.The imports and the
sleepfunction look good. Thesleepfunction is a useful utility for introducing delays in tests and is correctly typed.Consider adding a JSDoc comment to the
sleepfunction to improve documentation:/** * Introduces a delay in the execution for the specified number of milliseconds. * @param ms The number of milliseconds to sleep. * @returns A Promise that resolves after the specified delay. */ export const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms));
27-30: LGTM with a minor documentation suggestion.The
debugfunction is well-implemented, using Node.js'sdebuglogfor conditional debugging output. It's properly typed and allows for flexible usage with multiple arguments.Consider adding a JSDoc comment to improve documentation:
/** * Outputs debug messages when NODE_DEBUG=system-e2e is set in the environment. * @param msg The main debug message. * @param args Additional arguments to be logged. */ export function debug(msg: string, ...args: unknown[]) { debuglog('system-e2e')(msg, ...args) }
1-30: Overall good implementation with room for improvements.The utility functions in this file are well-structured and follow the coding guidelines for
libs/**files. They use TypeScript for type definitions and export the necessary functions, promoting reusability across different NextJS apps.To further improve the file:
- Consider using
fs/promisesconsistently across all file operations for better async handling.- Implement proper error handling in all functions, ensuring errors are logged and propagated correctly.
- Use async/await for asynchronous operations to improve readability and error handling.
- Add comprehensive JSDoc comments to all exported functions for better documentation.
- Consider bundling these utilities into a class or namespace if they grow in number, to prevent polluting the global namespace.
These improvements will enhance the maintainability, reliability, and usability of these utility functions across your e2e testing suite.
libs/testing/e2e/src/lib/support/application.ts (1)
14-29: LGTM: Function implementation is correct and follows guidelines. Consider adding error handling.The
createApplicationfunction is well-implemented, adhering to TypeScript usage and reusability guidelines for thelibsdirectory. It correctly interacts with the GraphQL API and manages application creation based on the current state.Consider enhancing the function with error handling for API requests and page interactions. This would improve robustness and make debugging easier. For example:
export const createApplication = async (page: Page) => { try { const responsePromise = await page.waitForResponse( '**/api/graphql?op=ApplicationApplications', ) const response = await responsePromise const responseData = await response.json() const numberOfApplications = responseData.data.applicationApplications.length || 0 if (numberOfApplications > 0) { await page.getByTestId('create-new-application').click() } return numberOfApplications } catch (error) { console.error('Error in createApplication:', error) throw error // or handle it as appropriate for your use case } }This change would catch and log any errors that occur during the API request or page interaction, making it easier to diagnose issues in tests.
libs/testing/e2e/src/lib/utils/playwright-config.ts (2)
3-8: ExportGlobalConfigParamsinterface for reusability.Consider exporting the
GlobalConfigParamsinterface so it can be reused in other modules, enhancing type safety across the codebase.Apply this change:
-interface GlobalConfigParams { +export interface GlobalConfigParams {
10-15: Add JSDoc comments for better documentation.Including JSDoc comments for the
createGlobalConfigfunction and theGlobalConfigParamsinterface will improve code readability and help other developers understand their usage.libs/testing/e2e/src/lib/support/login.ts (1)
100-101: Avoid using hardcoded text for selectorsThe selector uses hardcoded text
'Skipta um notanda', which may cause issues if the application's language changes or if localization is introduced.Consider using a
data-testidattribute or another stable selector that is not affected by language changes.Apply this diff to use a data-testid attribute:
-await page.getByRole('button', { name: 'Skipta um notanda' }).click() +await page.locator('data-testid=switch-user-button').click()Ensure that the corresponding element in the application has the
data-testid="switch-user-button"attribute.libs/testing/e2e/src/lib/support/urls.ts (1)
44-44: Consider Defaultingprocess.env.PORTProperlyIn the
localUrlconstant:const localUrl = `http://${BaseAuthority.local}:${process.env.PORT ?? 4200}`Ensure that the default port
4200aligns with the actual port used in your local development environment. If the standard port differs, update the default accordingly.libs/testing/e2e/src/lib/support/disablers.ts (7)
22-22: Typographical error in documentation commentThere's a typo in the comment: "
eroginal" should be "original".Apply this diff to fix the typo:
- * Return a copy of the `eroginal` object with any sub-objects mocked as `mockData` + * Return a copy of the `original` object with any sub-objects mocked as `mockData`
37-37: Use strict equality checksIt's recommended to use strict equality checks (
!==instead of!=) to prevent unexpected type coercion.Apply this diff:
- if (typeof original != 'object') { + if (typeof original !== 'object') {
41-41: Use strict equality checksAgain, use strict equality checks (
===instead of==) for improved type safety.Apply this diff:
- if (typeof mockKey == 'string') + if (typeof mockKey === 'string')
45-45: Remove hardcoded debug statementThe debug statement
if (key.match('currenLic')) debug('Mocking currentLic', original)appears to be specific to a particular key and may not be intended for inclusion in the final code. Consider removing it or generalizing it for broader use.Apply this diff:
- if (key.match('currenLic')) debug('Mocking currentLic', original)
112-115: Provide more context in error messageThe error message in the
if (!mockKey)block could be more informative by including additional context or guidance on how to resolve the issue.Consider updating the error message to help developers troubleshoot:
throw Error( - `Invalid key for mock (mockKey=${mockKey}, responseKey=${responseKey}, op=${op})!\nYou probably need to change the 'op' or add 'responseKey'`, + `Invalid mock key detected. Details:\n mockKey: ${mockKey}\n responseKey: ${responseKey}\n op: ${op}\nPlease verify that 'op' is correct or provide a valid 'responseKey'.`, )
164-176: Remove commented-out codeThere's a block of commented-out code that may no longer be needed. Removing unused code can improve readability and maintainability.
Apply this diff:
-/* - await mockQGL( - page, - 'UpdateApplication', - { - externalData: { - existingApplication: { data: [] }, - syslumennOnEntry: { data: {} }, - }, - }, - { patchResponse: true }, - ) -*/
179-183: Ensure consistent return statementsIn the
disableI18nfunction, consider adding a return statement for consistency, even though it's an asynchronous function.Apply this diff:
export async function disableI18n(page: Page) { - return await mockQGL(page, 'GetTranslations', { + await mockQGL(page, 'GetTranslations', { 'mock.translation': 'YES-mocked', }) + return }Or simply remove the redundant
return.libs/testing/e2e/src/lib/support/email-account.ts (3)
54-56: Avoid bracket notation when accessing object propertiesAccessing properties with string literals using bracket notation can be less readable and may bypass TypeScript's type checking.
Consider changing:
-emailVerifiedStatus.VerificationAttributes[emailAccount.email]['VerificationStatus'] !== 'Success' +emailVerifiedStatus.VerificationAttributes[emailAccount.email].VerificationStatus !== 'Success'This change improves readability and leverages TypeScript's strong typing.
73-169: Ensure explicit typing for variables and return typesTo adhere to TypeScript best practices, provide explicit types for variables and function return types where possible. This enhances code clarity and type safety.
For example, in the
makeEmailAccountfunction:-const emailConfig = { +const emailConfig: { imap: ImapOptions } = {And specify the return type for asynchronous functions:
-const makeEmailAccount = async (name: string): Promise<EmailAccount> => { +const makeEmailAccount = async (name: string): Promise<EmailAccount> => {
1-171: Optimize imports for effective tree-shakingTo ensure effective tree-shaking and bundling, import only the necessary modules from libraries instead of entire packages.
For example, when importing from
aws-sdk, import specific clients:-import { - GetIdentityVerificationAttributesCommand, - SESClient, - VerifyEmailAddressCommand, -} from '@aws-sdk/client-ses' +import { SESClient } from '@aws-sdk/client-ses' +import { GetIdentityVerificationAttributesCommand } from '@aws-sdk/client-ses/commands/GetIdentityVerificationAttributesCommand' +import { VerifyEmailAddressCommand } from '@aws-sdk/client-ses/commands/VerifyEmailAddressCommand'This can help reduce the bundle size by including only the necessary parts of the AWS SDK.
🧰 Tools
🪛 Biome
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
- libs/testing/e2e/src/index.ts (1 hunks)
- libs/testing/e2e/src/lib/support/api-tools.ts (1 hunks)
- libs/testing/e2e/src/lib/support/application.ts (1 hunks)
- libs/testing/e2e/src/lib/support/disablers.ts (1 hunks)
- libs/testing/e2e/src/lib/support/email-account.ts (1 hunks)
- libs/testing/e2e/src/lib/support/i18n.ts (1 hunks)
- libs/testing/e2e/src/lib/support/locator-helpers.ts (1 hunks)
- libs/testing/e2e/src/lib/support/login.ts (1 hunks)
- libs/testing/e2e/src/lib/support/session.ts (1 hunks)
- libs/testing/e2e/src/lib/support/urls.ts (1 hunks)
- libs/testing/e2e/src/lib/support/utils.ts (1 hunks)
- libs/testing/e2e/src/lib/support/wire-mocks.ts (1 hunks)
- libs/testing/e2e/src/lib/utils/pageHelpers.ts (1 hunks)
- libs/testing/e2e/src/lib/utils/playwright-config.ts (1 hunks)
- package.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
libs/testing/e2e/src/index.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/api-tools.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/application.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/disablers.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/email-account.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/i18n.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/locator-helpers.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/login.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/session.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/urls.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/utils.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/wire-mocks.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/utils/pageHelpers.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/utils/playwright-config.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome
libs/testing/e2e/src/lib/support/email-account.ts
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/testing/e2e/src/lib/support/wire-mocks.ts
[error] 8-8: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (16)
libs/testing/e2e/src/lib/support/i18n.ts (1)
15-15: LGTM! Well-implemented exported functionThe
labelfunction is well-implemented and adheres to the coding guidelines. It's exported for reusability, uses TypeScript for type definition, and is a named export which is good for tree-shaking. The function is concise and effectively utilizes theintlobject for message formatting.libs/testing/e2e/src/index.ts (1)
1-14: Significant changes to module's public API - update documentation and consumersThis change substantially alters the public API of the module. While it provides more granular access to various functionalities, it might break existing code that relied on the previous single export.
Please ensure that:
- All consumers of this module are updated to use the new export structure.
- The module's documentation is updated to reflect these changes, including any new functionalities now exposed.
- A migration guide is provided for users of the previous version of this module.
To verify the impact, you can run the following script to find potential usages of the old import style:
This will help identify files that might need updates due to these changes.
libs/testing/e2e/src/lib/support/locator-helpers.ts (2)
1-3: LGTM: Imports and type definition look good.The imports from '@playwright/test' are appropriate for the Playwright testing environment. The
Rolestype definition enhances type safety and code readability. This adheres to the TypeScript usage guideline for thelibsdirectory.
1-23: Great job: File adheres to coding guidelines and provides valuable utilities.This file (
locator-helpers.ts) is well-structured and adheres to all specified coding guidelines for thelibsdirectory:
- It provides reusable components (functions) that can be used across different NextJS apps.
- It uses TypeScript effectively for defining types and function signatures.
- The pure functions and modular structure support effective tree-shaking and bundling practices.
The utility functions enhance the functionality of element selection in Playwright tests, making it easier to write and maintain e2e tests.
libs/testing/e2e/src/lib/support/application.ts (2)
1-1: LGTM: Import statement is correct and follows guidelines.The import statement correctly imports the
Pagetype from Playwright, adhering to the TypeScript usage guideline for thelibsdirectory.
3-13: LGTM: Function documentation is comprehensive and follows best practices.The JSDoc comment block provides clear and detailed information about the
createApplicationfunction, including its purpose, parameters, return value, and behavior. This documentation supports code reusability and maintainability, aligning with the guidelines for thelibsdirectory.package.json (1)
358-358: Approved: Playwright test framework updated to latest version.The update of
@playwright/testfrom version 1.29 to 1.45.0 is a significant upgrade that likely includes numerous improvements and new features. This change is approved, but please consider the following recommendations:
- Verify that all existing Playwright tests still pass with the new version.
- Review the Playwright changelog for any breaking changes that might affect your test suite.
- Consider leveraging new features or improvements introduced in this version to enhance your test coverage or performance.
To ensure compatibility, run the following commands:
libs/testing/e2e/src/lib/support/login.ts (2)
82-85: Ensure robust handling of delegation identifiersWhen matching and formatting the
delegationidentifier, ensure that it correctly handles all expected formats and edge cases. Currently, it assumes a specific format for national IDs.Consider adding validation or comments to clarify the expected formats. Also, ensure that the regular expressions correctly handle all cases.
103-105: Handle navigation after switching usersAfter clicking on the user to switch to, ensure that the page has navigated to the expected URL. Currently, the code waits for the URL using
waitForURL, but it might be safer to also check that the page has fully loaded.Optionally, you can wait until a specific element on the
homeUrlpage is visible to ensure that the navigation was successful.libs/testing/e2e/src/lib/support/urls.ts (4)
93-97: VerifyjudicialSystemBaseUrlfor Production EnvironmentIn the
prodenvironment configuration,judicialSystemBaseUrlis set togetEnvironmentBaseUrl(BaseAuthority.prod), which resolves tohttps://island.is. This makesjudicialSystemBaseUrlthe same asislandisBaseUrl.Please verify if the judicial system's production base URL should be the main
island.isdomain or if it requires a specific subdomain.To confirm, you might need to check the correct production URL for the judicial system. If it should be a subdomain (e.g.,
https://judicial-system.island.is), update theBaseAuthorityenum and thejudicialSystemBaseUrlaccordingly.
36-43: Ensure Correct Handling ofBASE_URL_PREFIXThe
getEnvironmentBaseUrlfunction constructs the base URL considering theBASE_URL_PREFIXenvironment variable. Ensure that the logic correctly handles cases whereBASE_URL_PREFIXcould be'main'or an empty string.Current logic:
const prefix = (baseurlPrefix?.length ?? 0) > 0 && baseurlPrefix !== 'main' ? `${baseurlPrefix}-` : '' return `https://${prefix}${authority}`Please verify that this meets the intended URL structure for all environments.
99-103: Consistency in Local Environment URLsIn the
localconfiguration withinenvs, all base URLs uselocalUrl, which includes the port. Verify that services hosted locally are all reachable via the same port, or adjust the ports as necessary.
1-110: Code Adheres to Coding GuidelinesThe code follows the project coding guidelines:
- Reusability: The utility functions and constants are exportable and can be reused across different NextJS apps.
- TypeScript Usage: Types and enums are properly defined, enhancing type safety.
- Tree-Shaking and Bundling: Exported members are clearly defined, which aids in effective tree-shaking during bundling.
libs/testing/e2e/src/lib/support/wire-mocks.ts (1)
85-91: Ensure mocks are properly reset before testsThe
resetMocksfunction resets all mocks by recreating imposters. Confirm that this function is called appropriately before each test to prevent cross-test contamination.If not already in place, consider adding a setup step to your test suite to call
resetMocks()before tests run.libs/testing/e2e/src/lib/support/disablers.ts (2)
150-154: Clarify wildcard operation indisableObjectKeyfunctionThe
mockQGLfunction is called with'**'as theopparameter, which acts as a wildcard. Ensure that this behavior is intentional and won't inadvertently affect unintended operations.Please confirm that using
'**'is safe in this context and won't have side effects on other GraphQL operations.
161-162: Ensure compatibility across NextJS appsAs per the coding guidelines, confirm that the
disablePreviousApplicationsfunction is reusable across different NextJS apps. Verify that any app-specific logic is abstracted appropriately.Please review the function to ensure it adheres to reusability practices.
libs/testing/e2e/src/index.ts
Outdated
| export * from './lib/support/api-tools' | ||
| export * from './lib/support/application' | ||
| export * from './lib/support/disablers' | ||
| export * from './lib/support/email-account' | ||
| export * from './lib/support/i18n' | ||
| export * from './lib/support/locator-helpers' | ||
| export * from './lib/support/login' | ||
| export * from './lib/support/session' | ||
| export * from './lib/support/urls' | ||
| export * from './lib/support/utils' | ||
| export * from './lib/support/wire-mocks' | ||
| export * from './lib/utils/pageHelpers' | ||
| export * from './lib/utils/playwright-config' | ||
| export * from '@playwright/test' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-organized exports, but consider more specific export statements
The file demonstrates a clear organization of exports, logically grouped into support tools, utilities, and external library exports. This structure promotes good reusability across different NextJS apps.
However, the use of wildcard exports (export *) for all modules might impact tree-shaking and bundling efficiency. While it provides convenience, it can lead to larger bundle sizes if not all exported items are used in the importing modules.
Consider replacing wildcard exports with more specific export statements. This approach would:
- Improve tree-shaking by explicitly declaring what's being exported.
- Prevent potential naming conflicts between different modules.
- Provide better control over the public API of this module.
Example of a more specific export:
export { apiTool1, apiTool2 } from './lib/support/api-tools';
export { ApplicationSupport } from './lib/support/application';
// ... other specific exportsThis change would require more maintenance but could lead to better performance and more predictable behavior in the long run.
| export function deleteMockPdf() { | ||
| fs.unlink('./mockPdf.pdf', (err) => { | ||
| if (err) throw err | ||
| debug('Successfully deleted mockPdf file.') | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to use async/await for better error handling and consistency.
While the function works, it can be improved for better error handling and to align with modern JavaScript practices.
Consider refactoring the function to use async/await:
import fs from 'fs/promises'
export async function deleteMockPdf(): Promise<void> {
try {
await fs.unlink('./mockPdf.pdf');
debug('Successfully deleted mockPdf file.');
} catch (error) {
debug('Error deleting mockPdf file:', error);
throw error;
}
}This refactored version:
- Uses
fs/promisesfor Promise-based file operations. - Properly handles errors and logs them before re-throwing.
- Is consistent with the async nature of file operations.
| export function createMockPdf() { | ||
| fs.writeFile('./mockPdf.pdf', 'test', (e) => { | ||
| throw e | ||
| }) | ||
| /* | ||
| const doc = new PDFDocument() | ||
| doc.text('test') | ||
| doc.save() | ||
| doc.pipe(fs.createWriteStream('./mockPdf.pdf')) | ||
| doc.end() | ||
| */ | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
PDFKit Dependency Missing
pdfkit is listed as a dependency in package.json but is not installed in node_modules. Please install pdfkit to enable proper PDF creation and address the issues mentioned in the review comment.
🔗 Analysis chain
Improve error handling and consider using PDFKit for valid PDF creation.
The current implementation has a few issues:
- It doesn't create a valid PDF file, just a text file with a .pdf extension.
- Error handling is incorrect; throwing an error in the callback won't be caught.
- The function is synchronous, which might not be ideal for file operations.
Consider the following improvements:
- Use the commented-out PDFKit code to create a valid PDF file.
- Implement proper error handling.
- Make the function asynchronous.
Here's a suggested implementation:
import PDFDocument from 'pdfkit'
import fs from 'fs/promises'
export async function createMockPdf(): Promise<void> {
const doc = new PDFDocument();
doc.text('test');
try {
await fs.writeFile('./mockPdf.pdf', doc.toString());
debug('Successfully created mockPdf file.');
} catch (error) {
debug('Error creating mockPdf file:', error);
throw error;
}
}Don't forget to uncomment and install the PDFKit import at the top of the file.
Further Verification Needed:
The previous check indicated that npm is not available, preventing verification of PDFKit's installation. To determine if pdfkit is listed as a dependency, please run the following script:
Verification of PDFKit Installation:
To confirm whether pdfkit is installed, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if PDFKit is installed
npm list pdfkit || echo "PDFKit is not installed. Please install it using: npm install pdfkit"
Length of output: 235
Script:
#!/bin/bash
# Check if 'pdfkit' is listed in package.json dependencies
grep -E '"pdfkit"\s*:' package.json && echo "'pdfkit' is listed as a dependency." || echo "'pdfkit' is not listed as a dependency in package.json."
Length of output: 148
Script:
#!/bin/bash
# Check if 'pdfkit' is installed in node_modules
if [ -d "node_modules/pdfkit" ]; then
echo "'pdfkit' is installed in node_modules."
else
echo "'pdfkit' is not installed in node_modules."
fi
Length of output: 133
| (fieldExtractor: (op: { request: any; response: any }) => string) => | ||
| () => { | ||
| const op = data[0] | ||
| return op ? fieldExtractor(op) : '' | ||
| }, | ||
| data: (fieldExtractor: (op: { request: any; response: any }) => string) => { | ||
| const op = data[0] | ||
| return op ? fieldExtractor(op) : '' | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety by defining specific interfaces instead of 'any'
The data array is currently typed with { request: any; response: any }[], which uses the any type. This can lead to potential type safety issues and diminishes the benefits of using TypeScript. Consider defining specific interfaces for the request and response objects to leverage TypeScript's type-checking capabilities.
For example, define interfaces:
interface GraphQLRequest {
operationName: string;
variables: any;
query: string;
}
interface GraphQLResponse {
data: any;
errors?: any;
}
const data: { request: GraphQLRequest; response: GraphQLResponse }[] = [];This ensures that the types are well-defined and can improve code maintainability and readability.
| export async function verifyRequestCompletion( | ||
| page: Page, | ||
| url: string, | ||
| op: string, | ||
| ) { | ||
| const response = await page.waitForResponse( | ||
| (resp) => | ||
| resp.url().includes(url) && | ||
| resp.request().postDataJSON().operationName === op, | ||
| ) | ||
|
|
||
| return await response.json() | ||
| } |
There was a problem hiding this comment.
Add null checks to prevent potential runtime errors
In the verifyRequestCompletion function, accessing resp.request().postDataJSON().operationName assumes that postDataJSON() returns a valid object. If the request does not have JSON data or postDataJSON() returns undefined, it could lead to a runtime error.
Consider adding a null check to ensure safety:
- resp.request().postDataJSON().operationName === op,
+ resp.request().postDataJSON()?.operationName === op,Or, check that the content type is JSON before proceeding:
resp.request().headers()['content-type'] === 'application/json' &&
resp.request().postDataJSON()?.operationName === opThis will prevent errors if postDataJSON() is undefined.
| async function ensureIDSsession( | ||
| idsLoginOn: { nextAuth?: { nextAuthRoot: string } } | boolean, | ||
| page: Page, | ||
| context: BrowserContext, | ||
| homeUrl: string, | ||
| phoneNumber: string, | ||
| authUrlPrefix: string, | ||
| delegation?: string, | ||
| authTrigger: ((page: Page) => Promise<string>) | string = homeUrl, | ||
| ) { | ||
| if (typeof idsLoginOn === 'object' && idsLoginOn.nextAuth) { | ||
| const idsSessionValidation = await page.request.get( | ||
| `${idsLoginOn.nextAuth.nextAuthRoot}/api/auth/session`, | ||
| ) | ||
| const sessionObject = await idsSessionValidation.json() | ||
| if (!sessionObject.expires) { | ||
| const idsPage = await context.newPage() | ||
| if (typeof authTrigger === 'string') await idsPage.goto(authTrigger) | ||
| else authTrigger = await authTrigger(idsPage) | ||
| await idsLogin(idsPage, phoneNumber, authTrigger, delegation) | ||
| await idsPage.close() | ||
| } else { | ||
| debug(`IDS(next-auth) session exists`) | ||
| } | ||
| } else { | ||
| const idsSessionValidation = await page.request.get( | ||
| `${authUrlPrefix}/connect/sessioninfo`, | ||
| ) | ||
| const sessionHTML = await idsSessionValidation.text() | ||
| const sessionMatch = sessionHTML.match(/({.*?})/) | ||
| if (!sessionMatch || sessionMatch.length !== 2) { | ||
| throw new Error( | ||
| `IDS session html code has changed. Please review regex for extracting the session info.`, | ||
| ) | ||
| } | ||
| const sessionObject = JSON.parse(sessionMatch[1]) | ||
| if (sessionObject.status === 'No Session' || sessionObject.isExpired) { | ||
| const idsPage = await context.newPage() | ||
| if (typeof authTrigger === 'string') await idsPage.goto(authTrigger) | ||
| else authTrigger = await authTrigger(idsPage) | ||
| await idsLogin(idsPage, phoneNumber, authTrigger, delegation) | ||
| await idsPage.close() | ||
| } else { | ||
| debug(`IDS session exists`) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor ensureIDSsession for better maintainability
The ensureIDSsession function is lengthy and handles multiple scenarios, which can make it harder to read and maintain. Consider refactoring it into smaller helper functions to enhance readability and facilitate future updates.
| async function ensureCognitoSessionIfNeeded( | ||
| page: Page, | ||
| homeUrl = '/', | ||
| authUrlPrefix = '', | ||
| ) { |
There was a problem hiding this comment.
Add explicit TypeScript types to function parameters
The parameters homeUrl and authUrlPrefix in ensureCognitoSessionIfNeeded are missing explicit type annotations. Adding types enhances code clarity and type safety.
Apply this diff to include the missing types:
async function ensureCognitoSessionIfNeeded(
page: Page,
- homeUrl = '/',
- authUrlPrefix = '',
+ homeUrl: string = '/',
+ authUrlPrefix: string = '',
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function ensureCognitoSessionIfNeeded( | |
| page: Page, | |
| homeUrl = '/', | |
| authUrlPrefix = '', | |
| ) { | |
| async function ensureCognitoSessionIfNeeded( | |
| page: Page, | |
| homeUrl: string = '/', | |
| authUrlPrefix: string = '', | |
| ) { |
| const sessionObject = JSON.parse(sessionMatch[1]) | ||
| if (sessionObject.status === 'No Session' || sessionObject.isExpired) { |
There was a problem hiding this comment.
Handle potential JSON parsing errors
Parsing sessionMatch[1] with JSON.parse may throw an error if the content is not valid JSON. Consider wrapping the parsing in a try-catch block to handle exceptions gracefully.
Apply this diff to add error handling:
- const sessionObject = JSON.parse(sessionMatch[1])
+ let sessionObject
+ try {
+ sessionObject = JSON.parse(sessionMatch[1])
+ } catch (error) {
+ throw new Error(`Failed to parse session info JSON: ${error.message}`)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sessionObject = JSON.parse(sessionMatch[1]) | |
| if (sessionObject.status === 'No Session' || sessionObject.isExpired) { | |
| let sessionObject | |
| try { | |
| sessionObject = JSON.parse(sessionMatch[1]) | |
| } catch (error) { | |
| throw new Error(`Failed to parse session info JSON: ${error.message}`) | |
| } | |
| if (sessionObject.status === 'No Session' || sessionObject.isExpired) { |
| const sessionValidation = await sessionValidationPage.goto(homeUrl, { | ||
| waitUntil: 'domcontentloaded', | ||
| }) | ||
| await expect(sessionValidation?.url()).toMatch(homeUrl) |
There was a problem hiding this comment.
Handle possible null response from navigation
When navigating to homeUrl, the sessionValidation response may be null if the navigation fails. To prevent runtime errors, ensure that you handle this case before accessing sessionValidation.url().
Apply this diff to handle a potential null response:
- await expect(sessionValidation?.url()).toMatch(homeUrl)
+ if (sessionValidation) {
+ await expect(sessionValidation.url()).toMatch(homeUrl)
+ } else {
+ throw new Error(`Navigation to ${homeUrl} failed.`)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sessionValidation = await sessionValidationPage.goto(homeUrl, { | |
| waitUntil: 'domcontentloaded', | |
| }) | |
| await expect(sessionValidation?.url()).toMatch(homeUrl) | |
| const sessionValidation = await sessionValidationPage.goto(homeUrl, { | |
| waitUntil: 'domcontentloaded', | |
| }) | |
| if (sessionValidation) { | |
| await expect(sessionValidation.url()).toMatch(homeUrl) | |
| } else { | |
| throw new Error(`Navigation to ${homeUrl} failed.`) | |
| } |
| export async function session({ | ||
| browser, | ||
| homeUrl = '/', | ||
| phoneNumber = '', | ||
| authUrl = urls.authUrl, | ||
| idsLoginOn = true, | ||
| delegation = '', | ||
| storageState = `playwright-sessions-${homeUrl}-${phoneNumber}`, | ||
| authTrigger = homeUrl, | ||
| }: { | ||
| browser: Browser | ||
| homeUrl?: string | ||
| phoneNumber?: string | ||
| authUrl?: string | ||
| idsLoginOn?: | ||
| | boolean | ||
| | { | ||
| nextAuth?: { | ||
| nextAuthRoot: string | ||
| } | ||
| } | ||
| delegation?: string | ||
| storageState?: string | ||
| authTrigger?: string | ((page: Page) => Promise<string>) | ||
| }) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define and export interfaces for function parameters
To improve type safety and reuse across different NextJS apps, consider defining interfaces for the parameter objects of the exported session function. This aligns with best practices for TypeScript usage.
Example implementation:
+ export interface SessionOptions {
+ browser: Browser
+ homeUrl?: string
+ phoneNumber?: string
+ authUrl?: string
+ idsLoginOn?:
+ | boolean
+ | {
+ nextAuth?: {
+ nextAuthRoot: string
+ }
+ }
+ delegation?: string
+ storageState?: string
+ authTrigger?: string | ((page: Page) => Promise<string>)
+ }
export async function session({
- browser,
- homeUrl = '/',
- phoneNumber = '',
- authUrl = urls.authUrl,
- idsLoginOn = true,
- delegation = '',
- storageState = `playwright-sessions-${homeUrl}-${phoneNumber}`,
- authTrigger = homeUrl,
- }: {
- browser: Browser
- homeUrl?: string
- phoneNumber?: string
- authUrl?: string
- idsLoginOn?:
- | boolean
- | {
- nextAuth?: {
- nextAuthRoot: string
- }
- }
- delegation?: string
- storageState?: string
- authTrigger?: string | ((page: Page) => Promise<string>)
- }) {
+ }: SessionOptions) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function session({ | |
| browser, | |
| homeUrl = '/', | |
| phoneNumber = '', | |
| authUrl = urls.authUrl, | |
| idsLoginOn = true, | |
| delegation = '', | |
| storageState = `playwright-sessions-${homeUrl}-${phoneNumber}`, | |
| authTrigger = homeUrl, | |
| }: { | |
| browser: Browser | |
| homeUrl?: string | |
| phoneNumber?: string | |
| authUrl?: string | |
| idsLoginOn?: | |
| | boolean | |
| | { | |
| nextAuth?: { | |
| nextAuthRoot: string | |
| } | |
| } | |
| delegation?: string | |
| storageState?: string | |
| authTrigger?: string | ((page: Page) => Promise<string>) | |
| }) { | |
| export interface SessionOptions { | |
| browser: Browser | |
| homeUrl?: string | |
| phoneNumber?: string | |
| authUrl?: string | |
| idsLoginOn?: | |
| | boolean | |
| | { | |
| nextAuth?: { | |
| nextAuthRoot: string | |
| } | |
| } | |
| delegation?: string | |
| storageState?: string | |
| authTrigger?: string | ((page: Page) => Promise<string>) | |
| } | |
| export async function session({ | |
| browser, | |
| homeUrl = '/', | |
| phoneNumber = '', | |
| authUrl = urls.authUrl, | |
| idsLoginOn = true, | |
| delegation = '', | |
| storageState = `playwright-sessions-${homeUrl}-${phoneNumber}`, | |
| authTrigger = homeUrl, | |
| }: SessionOptions) { |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16387 +/- ##
==========================================
- Coverage 36.64% 36.37% -0.28%
==========================================
Files 6872 6855 -17
Lines 143068 144460 +1392
Branches 40770 41434 +664
==========================================
+ Hits 52431 52544 +113
- Misses 90637 91916 +1279 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 273 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 101 Total Test Services: 0 Failed, 98 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (2)
9-173: Consider standardizing folder names and structureThe folders section is well-organized, representing different service categories. However, to improve consistency and maintainability, consider the following suggestions:
- Standardize folder naming conventions (e.g., use either PascalCase or kebab-case consistently).
- Group related services together (e.g., "Health Insurance" and "Social Insurance" could be under a parent "Insurance" folder).
- Add descriptions to folders to provide more context about the contained services.
These changes would enhance the overall structure and make it easier for developers to navigate and understand the mock services.
1-1176: Overall, a solid mock configuration with room for improvementThis
service_portal_mocks.jsonfile provides a comprehensive mock configuration for various services, which is valuable for testing and development. However, to maximize its effectiveness and maintainability, consider the following general recommendations:
- Consistency: Standardize naming conventions across folders, routes, and data fields.
- Documentation: Add descriptions to folders and routes to provide context and improve usability.
- Data Realism: Enhance mock data to more closely resemble real-world scenarios.
- Security: Implement the suggested security improvements in CORS and header configurations.
- Modularity: Consider breaking this large file into smaller, service-specific files for easier management.
- Version Control: Implement a versioning system for the mock configuration to track changes over time.
Implementing these suggestions will result in a more robust, maintainable, and developer-friendly mock service configuration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
- .gitignore (1 hunks)
- libs/testing/e2e/src/index.ts (1 hunks)
- libs/testing/e2e/src/lib/libs/testing/e2e.spec.ts (0 hunks)
- libs/testing/e2e/src/lib/libs/testing/e2e.ts (0 hunks)
- libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (1 hunks)
- libs/testing/e2e/src/lib/support/email-account.ts (1 hunks)
- libs/testing/e2e/src/lib/utils/playwright-config.ts (1 hunks)
- package.json (1 hunks)
💤 Files with no reviewable changes (2)
- libs/testing/e2e/src/lib/libs/testing/e2e.spec.ts
- libs/testing/e2e/src/lib/libs/testing/e2e.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (4)
libs/testing/e2e/src/index.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/email-account.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/utils/playwright-config.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome
libs/testing/e2e/src/lib/support/email-account.ts
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
.gitignore (1)
101-104: LGTM: Appropriate additions for E2E testing outputs.The new entries for ignoring E2E testing outputs (
test-results/,playwright-report/, andtmp-sessions/) are appropriate and align with the PR objective of implementing a new e2e testing library. These directories typically contain temporary files and reports generated during E2E tests, which should not be tracked in version control.To ensure these directories are actually used by the new E2E testing setup, let's verify their presence in the codebase:
✅ Verification successful
LGTM: Verified E2E testing directories are in use.
The.gitignoreadditions fortest-results/,playwright-report/, andtmp-sessions/are appropriate as these directories are actively used by the E2E testing setup.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new E2E testing directories in the codebase. # Test: Search for references to the new directories in test configuration files or scripts rg -i 'test-results|playwright-report|tmp-sessions' --type yaml --type json --type js --type tsLength of output: 376
libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (1)
1-8: Main structure and metadata look goodThe configuration file has a well-defined structure with essential metadata such as UUID, name, endpoint prefix, latency, port, and hostname. This provides a solid foundation for the mock service.
libs/testing/e2e/src/lib/utils/playwright-config.ts (1)
49-49: Verify the correctness of the current working directoryThe
cwdparameter is set to'../../../'. Please ensure that this relative path correctly points to the intended directory when the configuration is used. Misconfiguring the current working directory could lead to failures in starting the web server.libs/testing/e2e/src/lib/support/email-account.ts (3)
22-29: Consider using theEmailAccounttype for consistencyUsing the
EmailAccounttype for theemailAccountparameter in theregisterEmailAddressWithSESfunction enhances consistency and maintainability.
35-35: Simplify conditional with optional chainingThe condition
if (verifyMsg && verifyMsg.text)can be simplified using optional chaining for better readability.🧰 Tools
🪛 Biome
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
171-171: Export theEmailAccounttype for reusabilityExporting the
EmailAccounttype improves reusability across different NextJS apps.
libs/testing/e2e/src/index.ts
Outdated
| export * from './lib/support/api-tools' | ||
| export * from './lib/support/application' | ||
| export * from './lib/support/disablers' | ||
| export * from './lib/support/email-account' | ||
| export * from './lib/support/i18n' | ||
| export * from './lib/support/locator-helpers' | ||
| export * from './lib/support/login' | ||
| export * from './lib/support/session' | ||
| export * from './lib/support/urls' | ||
| export * from './lib/support/utils' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using more specific export statements for better tree-shaking
The exports are well-organized and logically grouped, which promotes good reusability across different NextJS apps. However, the use of wildcard exports (export *) for all modules might impact tree-shaking and bundling efficiency.
Consider replacing wildcard exports with more specific export statements. This approach would:
- Improve tree-shaking by explicitly declaring what's being exported.
- Provide better control over the public API of this module.
Example:
export { graphqlSpy, mockApi, verifyRequestCompletion } from './lib/support/api-tools';
export { createApplication } from './lib/support/application';
// ... other specific exportsThis change would require more maintenance but could lead to better performance and more predictable behavior in the long run.
libs/testing/e2e/src/index.ts
Outdated
| export * from './lib/utils/pageHelpers' | ||
| export * from './lib/utils/playwright-config' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve utility exports for better module control
The separation of utility exports from support modules demonstrates good organization. However, the use of wildcard exports for utilities may still impact tree-shaking and bundling efficiency.
Consider using named exports for the utility modules to provide better control over the exposed API and improve tree-shaking capabilities. For example:
export { selectFormElement, fillFormElement } from './lib/utils/pageHelpers';
export { createPlaywrightConfig } from './lib/utils/playwright-config';This approach allows for more precise control over what is exposed from these utility modules and can potentially reduce bundle sizes in consuming applications.
libs/testing/e2e/src/index.ts
Outdated
| export * from './lib/support/utils' | ||
| export * from './lib/utils/pageHelpers' | ||
| export * from './lib/utils/playwright-config' | ||
| export * from '@playwright/test' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider re-exporting the entire Playwright test library
Re-exporting the entire '@playwright/test' module can lead to potential issues:
- It may cause version conflicts if the consuming project also imports Playwright directly.
- It potentially exposes more of the Playwright API than necessary, which could lead to misuse.
Consider one of these alternatives:
- Export only the specific entities from Playwright that are necessary for your testing utilities:
export { test, expect } from '@playwright/test';
- Create a custom namespace for Playwright exports to avoid polluting the global namespace:
import * as playwright from '@playwright/test'; export { playwright };
This approach would provide more control over the exposed API and reduce potential conflicts.
| "databucketID": "", | ||
| "sendFileAsBody": false, | ||
| "rules": [], | ||
| "rulesOperator": "OR", | ||
| "disableTemplating": false, | ||
| "fallbackTo404": false, | ||
| "default": true, | ||
| "crudKey": "id", | ||
| "callbacks": [] | ||
| } | ||
| ], | ||
| "responseMode": null | ||
| }, | ||
| { | ||
| "uuid": "81579408-5556-4864-b1bb-a0f0cdd85aa4", | ||
| "type": "http", | ||
| "documentation": "", | ||
| "method": "get", | ||
| "endpoint": "r1/IS-DEV/GOV/10016/Syslumenn-Protected/IslandMinarSidur/RettindiFyrirIslandIs/RettindiFyrirIslandIs", | ||
| "responses": [ | ||
| { | ||
| "uuid": "92b86f1f-4d87-4009-bfb5-4dc6665ab280", | ||
| "body": "{\n \"leyfi\": [\n {\n \"audkenni\": \"1010101\",\n \"titill\": \"Heimagisting - Test\",\n \"utgafudagur\": \"2022-07-11T14:40:36\",\n \"utgefandi\": {\n \"audkenni\": \"41\",\n \"titill\": \"Sýslumaðurinn á höfuðborgarsvæðinu\"\n },\n \"stada\": {\n \"titill\": \"Útrunnið\",\n \"kodi\": \"EXPIRED\"\n }\n },\n {\n \"audkenni\": \"123\",\n \"titill\": \"Verðbréfaréttindi\",\n \"utgafudagur\": \"2023-03-29T22:54:26\",\n \"utgefandi\": {\n \"audkenni\": \"41\",\n \"titill\": \"Sýslumaðurinn á höfuðborgarsvæðinu\"\n },\n \"stada\": {\n \"titill\": \"Í gildi\",\n \"kodi\": \"VALID\"\n }\n },\n {\n \"audkenni\": \"789\",\n \"titill\": \"Heimagisting - Test2\",\n \"utgafudagur\": null,\n \"utgefandi\": {\n \"audkenni\": \"41\",\n \"titill\": \"Sýslumaðurinn á höfuðborgarsvæðinu\"\n },\n \"stada\": {\n \"titill\": \"Í vinnslu\",\n \"kodi\": \"INPROGRESS\"\n }\n }\n ],\n \"villur\": {\n \"kodi\": null,\n \"skilabod\": null\n }\n}", | ||
| "latency": 0, | ||
| "statusCode": 200, | ||
| "label": "", | ||
| "headers": [], | ||
| "bodyType": "INLINE", | ||
| "filePath": "", | ||
| "databucketID": "", | ||
| "sendFileAsBody": false, | ||
| "rules": [], | ||
| "rulesOperator": "OR", | ||
| "disableTemplating": false, | ||
| "fallbackTo404": false, | ||
| "default": true, | ||
| "crudKey": "id", | ||
| "callbacks": [] | ||
| } | ||
| ], | ||
| "responseMode": null | ||
| }, | ||
| { | ||
| "uuid": "9a9c4083-fe80-474a-b829-ef49c15ab611", | ||
| "type": "http", | ||
| "documentation": "", | ||
| "method": "get", | ||
| "endpoint": "r1/IS-DEV/GOV/10016/Syslumenn-Protected/IslandMinarSidur/RettindiFyrirIslandIs/RettindiFyrirIslandIs/123", | ||
| "responses": [ | ||
| { | ||
| "uuid": "c7ebea65-747e-4464-9f8a-68e4e5e1a9e3", | ||
| "body": "{\n \"nafn\": \"Bubbi verðbréfakall\",\n \"leyfi\": {\n \"audkenni\": \"123\",\n \"titill\": \"Verðbréfaréttindi\",\n \"utgafudagur\": \"1999-12-12T01:01:01\",\n \"utgefandi\": {\n \"audkenni\": \"999\",\n \"titill\": \"Sýslukall\"\n },\n \"stada\": {\n \"titill\": \"Í vinnslu\",\n \"kodi\": \"INPROGRESS\"\n }\n },\n \"svid\": [\n {\n \"heiti\": \"Gildir til\",\n \"gildi\": \"1.1.2999\",\n \"tegund\": \"date\"\n }\n ],\n \"textar\": {\n \"haus\": \"Verðbréfaréttindi abc\",\n \"fotur\": \"blablabla\"\n },\n \"adgerdir\": [\n {\n \"tegund\": \"postholf\",\n \"titill\": \"test pósthólf\",\n \"slod\": \"123xyz\"\n },\n {\n \"tegund\": \"link\",\n \"titill\": \"Test link\",\n \"slod\": \"https://example.org\"\n }\n ],\n \"skrar\": null,\n \"villur\": null\n}", | ||
| "latency": 0, | ||
| "statusCode": 200, | ||
| "label": "", | ||
| "headers": [], | ||
| "bodyType": "INLINE", | ||
| "filePath": "", | ||
| "databucketID": "", | ||
| "sendFileAsBody": false, | ||
| "rules": [], | ||
| "rulesOperator": "OR", | ||
| "disableTemplating": false, | ||
| "fallbackTo404": false, | ||
| "default": true, | ||
| "crudKey": "id", | ||
| "callbacks": [] | ||
| } | ||
| ], | ||
| "responseMode": null | ||
| }, | ||
| { | ||
| "uuid": "c3bdad0b-b319-49f1-8c86-f19b0aae1064", | ||
| "type": "http", | ||
| "documentation": "", | ||
| "method": "get", | ||
| "endpoint": "r1/IS-DEV/GOV/10008/TR-Protected/external-v1/api/protected/v1/PaymentPlan/legitimatepayments", | ||
| "responses": [ | ||
| { | ||
| "uuid": "701a4736-8613-4abc-856d-830cd29fa8f5", | ||
| "body": "{\n \"nextPayment\": 1587,\n \"previousPayment\": 98671498\n}", | ||
| "latency": 0, | ||
| "statusCode": 200, | ||
| "label": "", | ||
| "headers": [], | ||
| "bodyType": "INLINE", | ||
| "filePath": "", | ||
| "databucketID": "", | ||
| "sendFileAsBody": false, | ||
| "rules": [], | ||
| "rulesOperator": "OR", | ||
| "disableTemplating": false, | ||
| "fallbackTo404": false, | ||
| "default": true, | ||
| "crudKey": "id", | ||
| "callbacks": [] | ||
| } | ||
| ], | ||
| "responseMode": null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance routes section for better maintainability and realism
The routes section is comprehensive, but consider the following improvements:
-
Consistent HTTP methods: Some endpoints only use GET or POST. Consider adding other methods (PUT, DELETE, PATCH) where appropriate for a more realistic API simulation.
-
Improve documentation: Many routes have empty documentation strings. Adding brief descriptions for each endpoint would greatly improve usability for developers.
-
Enhance mock data variety: Some responses use placeholder data (e.g., "bing", "beng", "bung"). Replace these with more realistic mock data to improve the testing experience.
-
Consider adding pagination: For endpoints that might return large datasets, implement pagination in the mock responses to better simulate real-world API behavior.
These enhancements will make the mock service more robust and useful for testing scenarios.
| "proxyMode": false, | ||
| "proxyHost": "", | ||
| "proxyRemovePrefix": false, | ||
| "tlsOptions": { | ||
| "enabled": false, | ||
| "type": "CERT", | ||
| "pfxPath": "", | ||
| "certPath": "", | ||
| "keyPath": "", | ||
| "caPath": "", | ||
| "passphrase": "" | ||
| }, | ||
| "cors": true, | ||
| "headers": [ | ||
| { | ||
| "key": "Content-Type", | ||
| "value": "application/json" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Origin", | ||
| "value": "*" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Methods", | ||
| "value": "GET,POST,PUT,PATCH,DELETE,HEAD,OPTIONS" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Headers", | ||
| "value": "Content-Type, Origin, Accept, Authorization, Content-Length, X-Requested-With" | ||
| } | ||
| ], | ||
| "proxyReqHeaders": [ | ||
| { | ||
| "key": "", | ||
| "value": "" | ||
| } | ||
| ], | ||
| "proxyResHeaders": [ | ||
| { | ||
| "key": "", | ||
| "value": "" | ||
| } | ||
| ], |
There was a problem hiding this comment.
Address security concerns in CORS and header configurations
The CORS and header configurations are generally well-set, but consider the following security improvements:
-
CORS: The current configuration allows all origins (
*). In a production environment, it's more secure to specify allowed origins explicitly. -
Content Security Policy: Consider adding a Content-Security-Policy header to mitigate risks of XSS attacks.
-
TLS: TLS options are defined but disabled. For production use, enable TLS to ensure secure communications.
-
X-Frame-Options: Add this header to prevent clickjacking attacks.
-
X-Content-Type-Options: Include this header to prevent MIME type sniffing.
Implementing these changes will significantly enhance the security posture of your mock service, making it more aligned with best practices for production-ready APIs.
| const mockoonCommand = `mockoon-cli start --data libs/testing/e2e/src/lib/mocks/${mockFile} --port 9388` | ||
| const serviceCommand = `cd infra && yarn cli run-local-env --services ${service} --dependencies ${dependencies.join( | ||
| ' ', | ||
| )}` |
There was a problem hiding this comment.
Validate inputs to prevent command injection vulnerabilities
The parameters mockFile, service, and dependencies are interpolated directly into shell commands in mockoonCommand and serviceCommand. If these inputs come from untrusted sources, this could lead to command injection vulnerabilities. Please ensure that these inputs are sanitized or validated before being used in command strings.
Consider applying input validation or sanitization to these parameters before usage.
|
|
||
| webServer: { | ||
| stdout: 'pipe', | ||
| command: `${mockoonCommand} & ${serviceCommand}`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve command execution for reliable web server startup
The command combines mockoonCommand and serviceCommand using &, which runs both commands concurrently. This may cause issues if one command depends on the other or if there are race conditions. Consider using && to ensure that mockoonCommand completes before serviceCommand starts, or use a more robust process management approach to handle parallel processes.
Apply this diff to modify the command:
-command: `${mockoonCommand} & ${serviceCommand}`,
+command: `${mockoonCommand} && ${serviceCommand}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| command: `${mockoonCommand} & ${serviceCommand}`, | |
| command: `${mockoonCommand} && ${serviceCommand}`, |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (1)
1-8: Consider enhancing the root-level configuration for better clarity and realism.The root-level configuration provides a good foundation, but consider the following improvements:
- Add a comment or documentation for the
lastMigrationfield to explain its purpose and usage.- Consider setting a non-zero default
latencyto simulate more realistic network conditions. You could use a small value like 50ms as a default.- If the empty
hostnameis intentional, consider adding a comment to explain why it's left blank or provide guidance on how it should be set in different environments.These changes will improve the configuration's clarity and make the mock service more realistic for testing scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
- libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/testing/e2e/src/lib/mocks/service_portal_mocks.json (2)
174-1103: Skipping comment generation as the previous review already covered these points.
1131-1173: Skipping comment generation as the previous review already covered these points.
| { | ||
| "type": "route", | ||
| "uuid": "ad26276e-7807-47b6-b4d6-8c5884837741" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "uuid": "f5bc18a5-1732-4fa4-875e-60e12099fe92", | ||
| "name": "Licenses", | ||
| "children": [ | ||
| { | ||
| "type": "route", | ||
| "uuid": "fd94adfd-ea59-4b8b-9619-7e54a9211874" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "40e7ffbc-e626-42db-917f-bcd8435a01cb" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "dc4f81a9-c735-4a0f-801c-365f3cc61cfa" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "02bbc9d5-a819-499a-bb61-92a7807e83cf" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "24a0a001-6830-49f3-833a-63ca2b04aa9b" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "842c6e50-2bc4-4d2f-908e-478a5ce1d560" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "7f67c189-feb5-4b70-b8f4-ac2975a720a8" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "65769b5f-6faa-4064-8c39-98a3158753cf" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "49f61c48-a63f-4786-9e51-52f2a576ed07" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "be1a7fe9-eed7-4b8d-8e7d-c12600a52b80" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "d854d346-a3fb-45d5-8ff7-da628a0af2f7" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "ddb97ac8-5fc8-41f9-9d53-cbc6997c407d" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "f47b6ce2-24d4-401e-8568-5ae2e7ee6077" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "uuid": "02ff10c6-ee39-4153-aff1-f66081984b82", | ||
| "name": "Occupational Licenses", | ||
| "children": [ | ||
| { | ||
| "type": "route", | ||
| "uuid": "3b8b9615-8428-4986-ba58-e3d1fcddec61" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "261f5b58-5aea-49db-8606-1a9d4a5427bb" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "81579408-5556-4864-b1bb-a0f0cdd85aa4" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "9a9c4083-fe80-474a-b829-ef49c15ab611" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "uuid": "f1748812-2277-4c8e-a33d-1fcf3b59f608", | ||
| "name": "Social Insurance", | ||
| "children": [ | ||
| { | ||
| "type": "route", | ||
| "uuid": "9ff539a0-96bd-4e55-a6c3-391f868761c2" | ||
| }, | ||
| { | ||
| "type": "route", | ||
| "uuid": "c3bdad0b-b319-49f1-8c86-f19b0aae1064" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance folder structure for consistency and flexibility.
The current folder structure provides a good organization for different service categories. However, consider the following improvements:
-
Standardize folder names for consistency. For example, use either singular or plural forms consistently (e.g., "Health Insurance" vs "Licenses").
-
Consider adding metadata to folders, such as a description field, to provide more context about each service category.
-
Allow for nested folders to support more complex service hierarchies if needed in the future.
-
Consider adding a
sortOrderfield to folders to control their display order in the UI.
These changes will improve the overall structure and make it more maintainable and extensible.
| "type": "http", | ||
| "documentation": "", | ||
| "method": "get", | ||
| "endpoint": "r1/IS-DEV/GOV/10008/TR-Protected/external-v1/api/protected/v1/PaymentPlan/legitimatepayments", | ||
| "responses": [ | ||
| { | ||
| "uuid": "701a4736-8613-4abc-856d-830cd29fa8f5", | ||
| "body": "{\n \"nextPayment\": 1587,\n \"previousPayment\": 98671498\n}", | ||
| "latency": 0, | ||
| "statusCode": 200, | ||
| "label": "", | ||
| "headers": [], | ||
| "bodyType": "INLINE", | ||
| "filePath": "", | ||
| "databucketID": "", | ||
| "sendFileAsBody": false, | ||
| "rules": [], | ||
| "rulesOperator": "OR", | ||
| "disableTemplating": false, | ||
| "fallbackTo404": false, | ||
| "default": true, | ||
| "crudKey": "id", | ||
| "callbacks": [] | ||
| } | ||
| ], | ||
| "responseMode": null | ||
| } | ||
| ], | ||
| "rootChildren": [ | ||
| { | ||
| "type": "folder", | ||
| "uuid": "c3275fb8-3e44-4a63-a1b4-a1bd5ef54fed" | ||
| }, | ||
| { | ||
| "type": "folder", | ||
| "uuid": "4e78eb48-9b40-4adf-8a3a-b6b32f93f5fa" | ||
| }, | ||
| { | ||
| "type": "folder", | ||
| "uuid": "5224e13b-be8e-4665-a7f9-2f5f53f8e0a5" | ||
| }, | ||
| { | ||
| "type": "folder", | ||
| "uuid": "f5bc18a5-1732-4fa4-875e-60e12099fe92" | ||
| }, | ||
| { | ||
| "type": "folder", | ||
| "uuid": "02ff10c6-ee39-4153-aff1-f66081984b82" | ||
| }, | ||
| { | ||
| "type": "folder", | ||
| "uuid": "f1748812-2277-4c8e-a33d-1fcf3b59f608" | ||
| } | ||
| ], | ||
| "proxyMode": false, | ||
| "proxyHost": "", | ||
| "proxyRemovePrefix": false, | ||
| "tlsOptions": { | ||
| "enabled": false, | ||
| "type": "CERT", | ||
| "pfxPath": "", | ||
| "certPath": "", | ||
| "keyPath": "", | ||
| "caPath": "", | ||
| "passphrase": "" | ||
| }, | ||
| "cors": true, | ||
| "headers": [ | ||
| { | ||
| "key": "Content-Type", | ||
| "value": "application/json" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Origin", | ||
| "value": "*" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Methods", | ||
| "value": "GET,POST,PUT,PATCH,DELETE,HEAD,OPTIONS" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Headers", | ||
| "value": "Content-Type, Origin, Accept, Authorization, Content-Length, X-Requested-With" | ||
| } | ||
| ], | ||
| "proxyReqHeaders": [ | ||
| { | ||
| "key": "", | ||
| "value": "" | ||
| } | ||
| ], | ||
| "proxyResHeaders": [ | ||
| { | ||
| "key": "", | ||
| "value": "" | ||
| } | ||
| ], | ||
| "data": [], | ||
| "callbacks": [] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider strategies to improve maintainability and consistency of the mock configuration.
While the mock configuration is comprehensive and well-structured, consider the following improvements to enhance its maintainability and consistency:
-
Split the configuration into smaller, more manageable files. For example, create separate files for each major service category or group of related endpoints. This will make it easier to update and maintain specific parts of the mock service.
-
Implement a versioning strategy for the mock configuration. This could include a version number in the root of the JSON structure and a changelog to track major changes.
-
Create and use shared response templates for common data structures to ensure consistency across different endpoints. This will make it easier to maintain and update mock data.
-
Consider using a schema validation tool to ensure the mock configuration adheres to a predefined structure. This can help catch errors and inconsistencies early in the development process.
-
Implement a naming convention for UUIDs to make it easier to identify and manage different parts of the configuration.
These improvements will enhance the overall quality, maintainability, and consistency of the mock service configuration.
svanaeinars
force-pushed
the
feat/use-new-e2e-test-lib-imports
branch
from
9bd76a9 to
360d052
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (180)
apps/consultation-portal/playwright.config.ts (1)
4-10: 🛠️ Refactor suggestion
Consider improving command string maintainability
While the configuration is functional, the command string could benefit from better maintainability.
Consider breaking down the command string into separate constants:
+const GET_SECRETS_COMMANDS = 'yarn get-secrets consultation-portal && yarn get-secrets api' +const LOCAL_ENV_COMMAND = 'cd infra && yarn cli run-local-env consultation-portal --dependencies api --print' + const consultationPortalConfig = createGlobalConfig({ webServerUrl: 'http://localhost:4200', port: 4200, - command: - 'yarn get-secrets consultation-portal && yarn get-secrets api && cd infra && yarn cli run-local-env consultation-portal --dependencies api --print', + command: `${GET_SECRETS_COMMANDS} && ${LOCAL_ENV_COMMAND}`, cwd: '../../', })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const GET_SECRETS_COMMANDS = 'yarn get-secrets consultation-portal && yarn get-secrets api' const LOCAL_ENV_COMMAND = 'cd infra && yarn cli run-local-env consultation-portal --dependencies api --print' const consultationPortalConfig = createGlobalConfig({ webServerUrl: 'http://localhost:4200', port: 4200, command: `${GET_SECRETS_COMMANDS} && ${LOCAL_ENV_COMMAND}`, cwd: '../../', })apps/portals/my-pages/playwright.config.ts (1)
7-9: 🛠️ Refactor suggestion
Consider splitting the command string for better maintainability
The command string combines multiple operations (mockoon-cli, changing directory, running local env) in a single line, which might be hard to maintain or debug.
Consider refactoring to improve readability:
- command: - '(mockoon-cli start --data ./apps/portals/my-pages/e2e/mocks/service_portal_mocks.json & cd infra && yarn cli run-local-env service-portal --dependencies api service-portal-api services-documents --print)', + command: [ + 'mockoon-cli start --data ./apps/portals/my-pages/e2e/mocks/service_portal_mocks.json &', + 'cd infra &&', + 'yarn cli run-local-env service-portal', + '--dependencies api service-portal-api services-documents', + '--print' + ].join(' '),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.command: [ 'mockoon-cli start --data ./apps/portals/my-pages/e2e/mocks/service_portal_mocks.json &', 'cd infra &&', 'yarn cli run-local-env service-portal', '--dependencies api service-portal-api services-documents', '--print' ].join(' '), })apps/portals/admin/playwright.config.ts (1)
4-9: 🛠️ Refactor suggestion
Consider separating the command string for better maintainability
The command string is quite long and combines multiple operations. Consider extracting it to a separate constant or configuration object for better maintainability.
+const MOCK_SERVER_COMMAND = 'mockoon-cli start --data ./apps/portals/my-pages/e2e/mocks/service_portal_mocks.json --port 9388' +const LOCAL_ENV_COMMAND = 'cd infra && yarn cli run-local-env service-portal --dependencies api service-portal-api services-documents --print' + const adminPortalConfig = createGlobalConfig({ webServerUrl: 'http://localhost:4200', port: 4200, - command: - '(mockoon-cli start --data ./apps/portals/my-pages/e2e/mocks/service_portal_mocks.json --port 9388 & cd infra && yarn cli run-local-env service-portal --dependencies api service-portal-api services-documents --print)', + command: `(${MOCK_SERVER_COMMAND} & ${LOCAL_ENV_COMMAND})`, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const MOCK_SERVER_COMMAND = 'mockoon-cli start --data ./apps/portals/my-pages/e2e/mocks/service_portal_mocks.json --port 9388' const LOCAL_ENV_COMMAND = 'cd infra && yarn cli run-local-env service-portal --dependencies api service-portal-api services-documents --print' const adminPortalConfig = createGlobalConfig({ webServerUrl: 'http://localhost:4200', port: 4200, command: `(${MOCK_SERVER_COMMAND} & ${LOCAL_ENV_COMMAND})`, })apps/application-system/form/playwright.config.ts (1)
4-9: 🛠️ Refactor suggestion
Consider refactoring the complex startup command
The current command string is complex and might be difficult to maintain. Consider breaking it down into separate configuration options or utility functions.
Here's a suggested refactoring approach:
-const applicationPortalApiConfig = createGlobalConfig({ - webServerUrl: 'http://localhost:4242', - port: 4242, - command: - '(mockoon-cli start --data ./apps/application-system/form/e2e/mocks/application_portal_mocks.json & cd infra && yarn cli run-local-env application-system-form --dependencies service-portal-api services-documents endorsement-system-api skilavottord-ws user-notification application-system-api icelandic-names-registry-backend services-sessions services-university-gateway --print)', -}) +const PORT = 4242; +const MOCK_DATA_PATH = './apps/application-system/form/e2e/mocks/application_portal_mocks.json'; + +const REQUIRED_SERVICES = [ + 'service-portal-api', + 'services-documents', + 'endorsement-system-api', + 'skilavottord-ws', + 'user-notification', + 'application-system-api', + 'icelandic-names-registry-backend', + 'services-sessions', + 'services-university-gateway' +]; + +const getMockServerCommand = () => + `mockoon-cli start --data ${MOCK_DATA_PATH}`; + +const getLocalEnvCommand = () => + `cd infra && yarn cli run-local-env application-system-form --dependencies ${REQUIRED_SERVICES.join(' ')} --print`; + +const applicationPortalApiConfig = createGlobalConfig({ + webServerUrl: `http://localhost:${PORT}`, + port: PORT, + command: `(${getMockServerCommand()} & ${getLocalEnvCommand()})`, +});This refactoring:
- Makes the configuration more maintainable
- Extracts magic numbers and strings into constants
- Makes service dependencies explicit and easier to modify
- Improves readability of the command construction
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const PORT = 4242; const MOCK_DATA_PATH = './apps/application-system/form/e2e/mocks/application_portal_mocks.json'; const REQUIRED_SERVICES = [ 'service-portal-api', 'services-documents', 'endorsement-system-api', 'skilavottord-ws', 'user-notification', 'application-system-api', 'icelandic-names-registry-backend', 'services-sessions', 'services-university-gateway' ]; const getMockServerCommand = () => `mockoon-cli start --data ${MOCK_DATA_PATH}`; const getLocalEnvCommand = () => `cd infra && yarn cli run-local-env application-system-form --dependencies ${REQUIRED_SERVICES.join(' ')} --print`; const applicationPortalApiConfig = createGlobalConfig({ webServerUrl: `http://localhost:${PORT}`, port: PORT, command: `(${getMockServerCommand()} & ${getLocalEnvCommand()})`, });apps/portals/admin/e2e/smoke/admin-portal.spec.ts (2)
12-24: 🛠️ Refactor suggestion
Consider extracting test configuration to constants.
The test setup is well-structured, but the hardcoded phone number could make tests brittle. Consider moving configuration values to a separate constants file or environment variables.
+import { TEST_USERS } from '../constants' + test.describe('Admin portal', () => { let context: BrowserContext test.beforeAll(async ({ browser }) => { context = await session({ browser, homeUrl: `/stjornbord`, - phoneNumber: '0102399', + phoneNumber: TEST_USERS.ADMIN.phoneNumber, idsLoginOn: true, }) })Committable suggestion skipped: line range outside the PR's diff.
25-30: 🛠️ Refactor suggestion
Enhance test robustness and coverage.
While the basic smoke test is a good start, consider these improvements:
- Add error handling for navigation failures
- Include more assertions to verify the admin portal state
- Add timeout handling for element visibility
test('should see welcome title', async () => { const page = await context.newPage() const { findByTestId } = helpers(page) - await page.goto('/stjornbord') - await expect(findByTestId('active-module-name')).toBeVisible() + try { + await page.goto('/stjornbord', { waitUntil: 'networkidle' }) + await expect(findByTestId('active-module-name')).toBeVisible({ timeout: 5000 }) + // Additional assertions + await expect(page).toHaveURL('/stjornbord') + await expect(page).toHaveTitle(/Admin Portal/) + } catch (error) { + throw new Error(`Failed to verify admin portal: ${error.message}`) + } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test('should see welcome title', async () => { const page = await context.newPage() const { findByTestId } = helpers(page) try { await page.goto('/stjornbord', { waitUntil: 'networkidle' }) await expect(findByTestId('active-module-name')).toBeVisible({ timeout: 5000 }) // Additional assertions await expect(page).toHaveURL('/stjornbord') await expect(page).toHaveTitle(/Admin Portal/) } catch (error) { throw new Error(`Failed to verify admin portal: ${error.message}`) } })apps/portals/my-pages/e2e/smoke/switchDelegation.ts (3)
11-20:
⚠️ Potential issueAdd error handling for delegation not found scenario.
The current implementation assumes the delegation will be found. Consider adding explicit error handling for better test debugging.
const firstDelegation = page .locator(`role=button[name*="${delegationType}"]`) .first() - await expect(firstDelegation).toBeVisible() + try { + await expect(firstDelegation).toBeVisible({ timeout: 5000 }) + } catch (error) { + throw new Error(`No ${delegationType} delegation found: ${error.message}`) + } const delegationName = await firstDelegation .locator('.identity-card--name') .textContent() - expect(delegationName).toBeTruthy() + if (!delegationName) { + throw new Error(`Delegation name not found for ${delegationType}`) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const firstDelegation = page .locator(`role=button[name*="${delegationType}"]`) .first() try { await expect(firstDelegation).toBeVisible({ timeout: 5000 }) } catch (error) { throw new Error(`No ${delegationType} delegation found: ${error.message}`) } const delegationName = await firstDelegation .locator('.identity-card--name') .textContent() if (!delegationName) { throw new Error(`Delegation name not found for ${delegationType}`) }
4-7: 🛠️ Refactor suggestion
Consider strengthening type safety with a union type constant.
While the union type for
delegationTypeis correct, consider extracting it to a constant to promote reusability and maintainability.+export const DelegationType = { + PROKURA: 'Prókúra' as const, + FORSJA: 'Forsjá' as const, +} as const +export type DelegationType = typeof DelegationType[keyof typeof DelegationType] export const switchDelegation = async ( page: Page, - delegationType: 'Prókúra' | 'Forsjá', + delegationType: DelegationType, ) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const DelegationType = { PROKURA: 'Prókúra' as const, FORSJA: 'Forsjá' as const, } as const export type DelegationType = typeof DelegationType[keyof typeof DelegationType] export const switchDelegation = async ( page: Page, delegationType: DelegationType, ) => {
8-9: 🛠️ Refactor suggestion
Add timeout and retry strategy for better test stability.
The current selector might be flaky if the user menu takes time to appear. Consider adding timeout and retry logic.
- await page.locator('data-testid=user-menu >> visible=true').click() + await page.locator('data-testid=user-menu >> visible=true').click({ + timeout: 5000, + trial: true + })Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/acceptance/social-insurance.spec.ts (3)
1-12: 🛠️ Refactor suggestion
Consider extracting configuration values to a separate constants file.
The test configuration looks good, but hardcoded values like URLs could be better managed in a dedicated constants file for better maintainability.
Create a new file
test-constants.ts:export const TEST_CONFIG = { HOME_URL: '/minarsidur', TEST_PHONE: '0102399', STORAGE_STATE: 'service-portal-faereyjar.json' }
14-29: 🛠️ Refactor suggestion
Add error handling for context setup and teardown.
While the test setup is good, it could benefit from proper error handling to ensure resources are cleaned up even if setup fails.
Consider this improved implementation:
test.beforeAll(async ({ browser }) => { + try { context = await session({ browser: browser, storageState: 'service-portal-faereyjar.json', homeUrl, phoneNumber: '0102399', idsLoginOn: true, }) + } catch (error) { + console.error('Failed to setup test context:', error) + throw error + } }) test.afterAll(async () => { + try { await context?.close() + } catch (error) { + console.error('Failed to close test context:', error) + } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('MS - Social Insurance', () => { let context: BrowserContext test.beforeAll(async ({ browser }) => { try { context = await session({ browser: browser, storageState: 'service-portal-faereyjar.json', homeUrl, phoneNumber: '0102399', idsLoginOn: true, }) } catch (error) { console.error('Failed to setup test context:', error) throw error } }) test.afterAll(async () => { try { await context?.close() } catch (error) { console.error('Failed to close test context:', error) } })
31-51: 🛠️ Refactor suggestion
Enhance test reliability and coverage.
While the test case is well-structured, it could benefit from more robust assertions and waiting strategies.
Consider these improvements:
test('payment plan', async () => { const page = await context.newPage() await disableI18n(page) await test.step('should display data when switching years', async () => { - // Arrange await page.goto( icelandicAndNoPopupUrl('minarsidur/framfaersla/greidsluaetlun'), ) + // Wait for network idle to ensure data is loaded + await page.waitForLoadState('networkidle') const title = page.getByRole('heading', { name: 'Greiðsluáætlun', }) - await expect(title).toBeVisible() + await expect(title).toBeVisible({ timeout: 5000 }) - await expect( - page.getByText('Skattskyldar greiðslutegundir'), - ).toBeVisible() + // Add more specific assertions + const paymentTypeSection = page.getByText('Skattskyldar greiðslutegundir') + await expect(paymentTypeSection).toBeVisible({ timeout: 5000 }) + + // Verify the page is interactive + await expect(page.getByRole('button')).toBeEnabled() }) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test('payment plan', async () => { const page = await context.newPage() await disableI18n(page) await test.step('should display data when switching years', async () => { await page.goto( icelandicAndNoPopupUrl('minarsidur/framfaersla/greidsluaetlun'), ) // Wait for network idle to ensure data is loaded await page.waitForLoadState('networkidle') const title = page.getByRole('heading', { name: 'Greiðsluáætlun', }) await expect(title).toBeVisible({ timeout: 5000 }) // Add more specific assertions const paymentTypeSection = page.getByText('Skattskyldar greiðslutegundir') await expect(paymentTypeSection).toBeVisible({ timeout: 5000 }) // Verify the page is interactive await expect(page.getByRole('button')).toBeEnabled() }) })apps/application-system/form/e2e/utils/addons.ts (2)
4-24: 🛠️ Refactor suggestion
Enhance robustness and type safety of the count matcher
While the implementation works, there are several improvements that could make it more robust:
Consider applying these enhancements:
async toHaveCountGreaterThan( received: Locator, value: number, - options: { timeout: number; sleepTime: number } = { + options: { timeout?: number; sleepTime?: number } = { timeout: 10000, sleepTime: 100, }, ) { + const { timeout, sleepTime } = { + timeout: Math.max(0, options.timeout ?? 10000), + sleepTime: Math.max(10, options.sleepTime ?? 100), + } const initialTime = Date.now() let count = -1 while (count <= value) { count = await received.count() - if (Date.now() > initialTime + options.timeout) - return { message: () => 'Timeout', pass: false } + if (Date.now() > initialTime + timeout) + return { + message: () => + `Timeout after ${timeout}ms. Last count was ${count}, expected > ${value}`, + pass: false + } + if (count > value + 100) break; // Early exit if count is significantly higher - await sleep(options.sleepTime) + await sleep(sleepTime) } return { - message: () => `Found ${count} elements`, + message: () => `Found ${count} elements (expected > ${value})`, pass: true, } }These changes:
- Add type safety for options with proper defaults
- Prevent negative timeout/sleep values
- Add early exit for efficiency
- Improve error messages with more context
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async toHaveCountGreaterThan( received: Locator, value: number, options: { timeout?: number; sleepTime?: number } = { timeout: 10000, sleepTime: 100, }, ) { const { timeout, sleepTime } = { timeout: Math.max(0, options.timeout ?? 10000), sleepTime: Math.max(10, options.sleepTime ?? 100), } const initialTime = Date.now() let count = -1 while (count <= value) { count = await received.count() if (Date.now() > initialTime + timeout) return { message: () => `Timeout after ${timeout}ms. Last count was ${count}, expected > ${value}`, pass: false } if (count > value + 100) break; // Early exit if count is significantly higher await sleep(sleepTime) } return { message: () => `Found ${count} elements (expected > ${value})`, pass: true, } },
25-39: 🛠️ Refactor suggestion
Strengthen URL validation and type safety
The URL validation could be more robust to prevent potential security issues:
Consider these improvements:
-async toBeApplication(received: string | Page, applicationType = '\\w+') { +async toBeApplication( + received: string | Page, + applicationType: string = '[a-zA-Z0-9-]+', +) { const url: string = typeof received == 'string' ? received : received.url() + // Sanitize URL to prevent injection + const sanitizedUrl = decodeURIComponent(url).trim() const protocol = 'https?://' const host = '[^/]+' - const applicationId = '(/(\\w|-)*)?' + const applicationId = '(/[a-zA-Z0-9-]+)?' const applicationRegExp = new RegExp( `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, + 'i' // case insensitive ) - const pass = applicationRegExp.test(url) + const pass = applicationRegExp.test(sanitizedUrl) const message = () => `Current page is ${pass ? '' : '*not* '}an application Pattern ${applicationRegExp} - URL is ${url}` + URL is ${sanitizedUrl}` return { message, pass } }These changes:
- Use stricter regex patterns for better security
- Add URL sanitization
- Make the match case-insensitive
- Improve type safety with more specific patterns
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async toBeApplication( received: string | Page, applicationType: string = '[a-zA-Z0-9-]+', ) { const url: string = typeof received == 'string' ? received : received.url() // Sanitize URL to prevent injection const sanitizedUrl = decodeURIComponent(url).trim() const protocol = 'https?://' const host = '[^/]+' const applicationId = '(/[a-zA-Z0-9-]+)?' const applicationRegExp = new RegExp( `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, 'i' // case insensitive ) const pass = applicationRegExp.test(sanitizedUrl) const message = () => `Current page is ${pass ? '' : '*not* '}an application Pattern ${applicationRegExp} URL is ${sanitizedUrl}` return { message, pass } },apps/consultation-portal/e2e/utils/addons.ts (2)
4-24:
⚠️ Potential issueImprove robustness of toHaveCountGreaterThan matcher
The matcher has several potential issues:
- The while loop condition
count <= valuecould lead to an infinite loop when count equals value- The options parameter lacks TypeScript interface
- No validation for negative timeout/sleep values
- Error messages could be more descriptive
Consider applying these improvements:
+interface PollingOptions { + timeout: number; + sleepTime: number; +} async toHaveCountGreaterThan( received: Locator, value: number, - options: { timeout: number; sleepTime: number } = { + options: PollingOptions = { timeout: 10000, sleepTime: 100, }, ) { + if (options.timeout < 0 || options.sleepTime < 0) { + throw new Error('Timeout and sleep time must be positive numbers'); + } const initialTime = Date.now() let count = -1 - while (count <= value) { + while (count < value) { count = await received.count() - if (Date.now() > initialTime + options.timeout) - return { message: () => 'Timeout', pass: false } + if (Date.now() > initialTime + options.timeout) { + return { + message: () => + `Timeout reached after ${options.timeout}ms. Found ${count} elements, expected more than ${value}`, + pass: false + } + } await sleep(options.sleepTime) } return { - message: () => `Found ${count} elements`, + message: () => `Found ${count} elements (expected more than ${value})`, pass: true, } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.interface PollingOptions { timeout: number; sleepTime: number; } async toHaveCountGreaterThan( received: Locator, value: number, options: PollingOptions = { timeout: 10000, sleepTime: 100, }, ) { if (options.timeout < 0 || options.sleepTime < 0) { throw new Error('Timeout and sleep time must be positive numbers'); } const initialTime = Date.now() let count = -1 while (count < value) { count = await received.count() if (Date.now() > initialTime + options.timeout) { return { message: () => `Timeout reached after ${options.timeout}ms. Found ${count} elements, expected more than ${value}`, pass: false } } await sleep(options.sleepTime) } return { message: () => `Found ${count} elements (expected more than ${value})`, pass: true, } }
25-39: 🛠️ Refactor suggestion
Enhance maintainability and type safety of toBeApplication matcher
The matcher could benefit from:
- More maintainable RegExp construction
- Stricter URL validation
- Better type safety
Consider these improvements:
+type ApplicationInput = string | Page; + +const URL_PARTS = { + protocol: 'https?://', + host: '[^/]+', + applicationId: '(/(\\w|-)*)?' +} as const; + -async toBeApplication(received: string | Page, applicationType = '\\w+') { +async toBeApplication(received: ApplicationInput, applicationType = '\\w+') { const url: string = typeof received == 'string' ? received : received.url() - const protocol = 'https?://' - const host = '[^/]+' - const applicationId = '(/(\\w|-)*)?' + if (!url.match(/^https?:\/\//)) { + return { + message: () => `Invalid URL protocol: ${url}`, + pass: false + } + } + const applicationRegExp = new RegExp( - `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, + [ + '^', + URL_PARTS.protocol, + URL_PARTS.host, + '/umsoknir/', + applicationType, + URL_PARTS.applicationId, + '$' + ].join('') ) const pass = applicationRegExp.test(url) const message = () => - `Current page is ${pass ? '' : '*not* '}an application - Pattern ${applicationRegExp} - URL is ${url}` + [ + `Current page is ${pass ? '' : 'not '}an application`, + `Pattern: ${applicationRegExp}`, + `URL: ${url}` + ].join('\n') return { message, pass } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.type ApplicationInput = string | Page; const URL_PARTS = { protocol: 'https?://', host: '[^/]+', applicationId: '(/(\\w|-)*)?' } as const; async toBeApplication(received: ApplicationInput, applicationType = '\\w+') { const url: string = typeof received == 'string' ? received : received.url() if (!url.match(/^https?:\/\//)) { return { message: () => `Invalid URL protocol: ${url}`, pass: false } } const applicationRegExp = new RegExp( [ '^', URL_PARTS.protocol, URL_PARTS.host, '/umsoknir/', applicationType, URL_PARTS.applicationId, '$' ].join('') ) const pass = applicationRegExp.test(url) const message = () => [ `Current page is ${pass ? '' : 'not '}an application`, `Pattern: ${applicationRegExp}`, `URL: ${url}` ].join('\n') return { message, pass } }apps/portals/admin/e2e/utils/addons.ts (2)
25-39: 🛠️ Refactor suggestion
Enhance URL validation and error reporting
The application URL matcher could be improved in several areas:
- The regex pattern could be more strict to prevent invalid URLs
- The error message formatting could be clearer
- Missing validation for applicationType parameter
Here's the suggested implementation:
async toBeApplication(received: string | Page, applicationType = '\\w+') { + if (!/^[\w-]+$/.test(applicationType)) { + throw new Error('applicationType must contain only word characters and hyphens'); + } const url: string = typeof received == 'string' ? received : received.url() - const protocol = 'https?://' + const protocol = 'https://' const host = '[^/]+' const applicationId = '(/(\\w|-)*)?' const applicationRegExp = new RegExp( `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, ) const pass = applicationRegExp.test(url) const message = () => - `Current page is ${pass ? '' : '*not* '}an application - Pattern ${applicationRegExp} - URL is ${url}` + [ + `Current page is ${pass ? '' : 'not '}an application`, + `Expected pattern: ${applicationRegExp}`, + `Actual URL: ${url}`, + ].join('\n') return { message, pass } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async toBeApplication(received: string | Page, applicationType = '\\w+') { if (!/^[\w-]+$/.test(applicationType)) { throw new Error('applicationType must contain only word characters and hyphens'); } const url: string = typeof received == 'string' ? received : received.url() const protocol = 'https://' const host = '[^/]+' const applicationId = '(/(\\w|-)*)?' const applicationRegExp = new RegExp( `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, ) const pass = applicationRegExp.test(url) const message = () => [ `Current page is ${pass ? '' : 'not '}an application`, `Expected pattern: ${applicationRegExp}`, `Actual URL: ${url}`, ].join('\n') return { message, pass } }
4-24:
⚠️ Potential issueImprove robustness and type safety of the count matcher
The matcher has several areas that could be improved:
- The while loop condition
count <= valuemeans it continues running when count equals value- Missing validation for timeout and sleepTime values
- Error message could be more informative
Here's the suggested implementation:
async toHaveCountGreaterThan( received: Locator, value: number, options: { timeout: number; sleepTime: number } = { timeout: 10000, sleepTime: 100, }, ) { + if (options.timeout < 0 || options.sleepTime < 0) { + throw new Error('Timeout and sleepTime must be positive numbers'); + } const initialTime = Date.now() let count = -1 - while (count <= value) { + while (count < value) { count = await received.count() if (Date.now() > initialTime + options.timeout) - return { message: () => 'Timeout', pass: false } + return { + message: () => + `Timeout waiting for count to exceed ${value}. Last count was ${count}`, + pass: false + } await sleep(options.sleepTime) } return { message: () => `Found ${count} elements`, pass: true, } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async toHaveCountGreaterThan( received: Locator, value: number, options: { timeout: number; sleepTime: number } = { timeout: 10000, sleepTime: 100, }, ) { if (options.timeout < 0 || options.sleepTime < 0) { throw new Error('Timeout and sleepTime must be positive numbers'); } const initialTime = Date.now() let count = -1 while (count < value) { count = await received.count() if (Date.now() > initialTime + options.timeout) return { message: () => `Timeout waiting for count to exceed ${value}. Last count was ${count}`, pass: false } await sleep(options.sleepTime) } return { message: () => `Found ${count} elements`, pass: true, } }apps/portals/my-pages/e2e/utils/addons.ts (2)
4-24: 🛠️ Refactor suggestion
Enhance robustness and documentation of the toHaveCountGreaterThan matcher.
Several improvements could make this matcher more robust and maintainable:
Consider applying these enhancements:
+/** + * Custom matcher that checks if the count of elements matching a locator exceeds a specified value. + * @param received - Playwright locator to check + * @param value - Minimum count threshold + * @param options - Configuration for timeout and polling interval + * @returns Promise<{message: () => string, pass: boolean}> + */ async toHaveCountGreaterThan( received: Locator, value: number, options: { timeout: number; sleepTime: number } = { timeout: 10000, sleepTime: 100, }, ) { + if (options.timeout < 0 || options.sleepTime < 0) { + throw new Error('Timeout and sleep time must be positive numbers'); + } const initialTime = Date.now() let count = -1 while (count <= value) { count = await received.count() - if (Date.now() > initialTime + options.timeout) - return { message: () => 'Timeout', pass: false } + if (Date.now() > initialTime + options.timeout) { + return { + message: () => + `Timeout reached after ${options.timeout}ms. Found ${count} elements, expected more than ${value}.`, + pass: false + } + } await sleep(options.sleepTime) } return { - message: () => `Found ${count} elements`, + message: () => `Found ${count} elements (expected more than ${value})`, pass: true, } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Custom matcher that checks if the count of elements matching a locator exceeds a specified value. * @param received - Playwright locator to check * @param value - Minimum count threshold * @param options - Configuration for timeout and polling interval * @returns Promise<{message: () => string, pass: boolean}> */ async toHaveCountGreaterThan( received: Locator, value: number, options: { timeout: number; sleepTime: number } = { timeout: 10000, sleepTime: 100, }, ) { if (options.timeout < 0 || options.sleepTime < 0) { throw new Error('Timeout and sleep time must be positive numbers'); } const initialTime = Date.now() let count = -1 while (count <= value) { count = await received.count() if (Date.now() > initialTime + options.timeout) { return { message: () => `Timeout reached after ${options.timeout}ms. Found ${count} elements, expected more than ${value}.`, pass: false } } await sleep(options.sleepTime) } return { message: () => `Found ${count} elements (expected more than ${value})`, pass: true, } }
25-39: 💡 Codebase verification
🛠️ Refactor suggestion
The search results show various application URL patterns in use, and I can now make an informed assessment of the review comment. Let me generate the final response:
The suggested URL pattern restrictions need adjustment based on actual usage patterns
The review comment's suggestions for URL pattern restrictions are too strict given the actual application URLs in the codebase. The URLs show:
- Application types using hyphens extensively (e.g., 'breytt-logheimili-barns', 'uppbot-a-lifeyri')
- Some application IDs containing special characters
- Various application type lengths
Recommended modifications to the original suggestion:
async toBeApplication(received: string | Page, applicationType = '\\w+') { - if (!/^[\w-]+$/.test(applicationType.replace('\\', ''))) { - throw new Error('applicationType must contain only word characters and hyphens'); + if (!/^[\\]?[\w-]+$/.test(applicationType)) { + throw new Error('Invalid applicationType pattern'); } const url: string = typeof received == 'string' ? received : received.url() const protocol = 'https?://' const host = '[^/]+' - const applicationId = '(/[\\w-]{1,50})?' + const applicationId = '(/[\\w-]+)?' const applicationRegExp = new RegExp( `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, )The JSDoc and message formatting improvements from the original review are still valid and beneficial.
🔗 Analysis chain
Enhance security and documentation of the toBeApplication matcher.
The matcher could benefit from improved input validation and documentation:
Consider applying these enhancements:
+/** + * Custom matcher that validates if a URL matches the application pattern. + * @param received - URL string or Playwright Page object + * @param applicationType - Type of application to match (defaults to any word characters) + * @returns Promise<{message: () => string, pass: boolean}> + */ async toBeApplication(received: string | Page, applicationType = '\\w+') { + if (!/^[\w-]+$/.test(applicationType.replace('\\', ''))) { + throw new Error('applicationType must contain only word characters and hyphens'); + } const url: string = typeof received == 'string' ? received : received.url() const protocol = 'https?://' const host = '[^/]+' - const applicationId = '(/(\\w|-)*)?' + const applicationId = '(/[\\w-]{1,50})?' const applicationRegExp = new RegExp( `^${protocol}${host}/umsoknir/${applicationType}${applicationId}$`, ) const pass = applicationRegExp.test(url) const message = () => - `Current page is ${pass ? '' : '*not* '}an application - Pattern ${applicationRegExp} - URL is ${url}` + [ + `Current page is ${pass ? '' : 'not '}an application of type '${applicationType}'`, + `Expected pattern: ${applicationRegExp}`, + `Actual URL: ${url}` + ].join('\n') return { message, pass } }The changes include:
- Added input validation for
applicationType- Limited the length and characters in
applicationId- Improved message formatting
- Added JSDoc documentation
Let's verify the URL patterns in use:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for application URL patterns in the codebase rg -g '*.{ts,js,tsx,jsx}' '/umsoknir/[\w-]+' --no-filenameLength of output: 9751
apps/application-system/form/e2e/acceptance/passport.spec.ts (1)
35-47: 🛠️ Refactor suggestion
Enhance test robustness and documentation
While the test implementation is functional, consider these improvements:
- Extract test data to constants:
+const TEST_DATA = { + EXPECTED_NAME: 'Gervimaður Færeyjar', + QUERY_PARAMS: { delegationChecked: 'true' } +} as const; applicationTest.describe('Passport', () => { applicationTest( 'Should be able to start application', async ({ applicationPage }) => { const page = applicationPage - page.goto(`${homeUrl}?delegationChecked=true`) + const url = new URL(homeUrl); + url.search = new URLSearchParams(TEST_DATA.QUERY_PARAMS).toString(); + page.goto(url.toString()) await page.getByTestId('agree-to-data-providers').check() await page.getByTestId('proceed').click() - await expect(page.getByText('Gervimaður Færeyjar')).toBeVisible() + await expect(page.getByText(TEST_DATA.EXPECTED_NAME)).toBeVisible() }, ) })
- Add intermediate state validations:
// Validate checkbox state after checking await expect(page.getByTestId('agree-to-data-providers')).toBeChecked() // Validate button state before clicking await expect(page.getByTestId('proceed')).toBeEnabled()apps/portals/my-pages/e2e/smoke/endorsements.spec.ts (2)
15-23:
⚠️ Potential issueMove sensitive data to environment variables.
The phone number is hardcoded in the session configuration. Consider moving it to an environment variable for better security and configurability.
context = await session({ browser: browser, storageState: 'service-portal-amerika.json', homeUrl: `${urls.islandisBaseUrl}/minarsidur`, - phoneNumber: '0102989', + phoneNumber: process.env.TEST_PHONE_NUMBER, idsLoginOn: true, })Committable suggestion skipped: line range outside the PR's diff.
29-51: 🛠️ Refactor suggestion
Enhance test robustness and maintainability.
Consider the following improvements:
- Move the timeout value to a named constant
- Add error state testing
- Extract URL paths to constants
- Add comments explaining the purpose of disabling i18n
+ const VISIBILITY_TIMEOUT = 10000 + const ENDORSEMENT_LIST_PATH = '/minarsidur/min-gogn/listar' test('should be able to access and see UI elements in minar-sidur for endorsements', async () => { const page = await context.newPage() + // Disable i18n to ensure consistent text matching across locales await disableI18n(page) - const timeout = 10000 - - await page.goto(icelandicAndNoPopupUrl('/minarsidur/min-gogn/listar')) + await page.goto(icelandicAndNoPopupUrl(ENDORSEMENT_LIST_PATH)) await Promise.all([ expect( page.getByRole('button', { name: 'Stofna nýjan lista' }), - ).toBeVisible({ timeout }), + ).toBeVisible({ timeout: VISIBILITY_TIMEOUT }), // ... similar changes for other expectations ]) + + // Test error states + await page.route(ENDORSEMENT_LIST_PATH, (route) => route.abort()) + await expect(page.getByText('Error loading endorsement lists')).toBeVisible() })Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/work-license.spec.ts (2)
34-54: 🛠️ Refactor suggestion
Consider extracting test data to constants.
The test data like URLs and expected text could be moved to constants at the top of the file for better maintainability.
+const TEST_DATA = { + PAGE_URL: '/minarsidur/starfsleyfi', + LICENSE_NAME: 'Kennari', +} as const; + test('Work license overview has a work-license', async () => { const page = await context.newPage() await disableI18n(page) await test.step('Renders the page', async () => { // Arrange - await page.goto(icelandicAndNoPopupUrl('/minarsidur/starfsleyfi')) + await page.goto(icelandicAndNoPopupUrl(TEST_DATA.PAGE_URL)) // ... // "Leyfisbréf - Kennari" comes from the api - not translateable - const licenseName = page.getByText('Kennari').first() + const licenseName = page.getByText(TEST_DATA.LICENSE_NAME).first()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const TEST_DATA = { PAGE_URL: '/minarsidur/starfsleyfi', LICENSE_NAME: 'Kennari', } as const; test('Work license overview has a work-license', async () => { const page = await context.newPage() await disableI18n(page) await test.step('Renders the page', async () => { // Arrange await page.goto(icelandicAndNoPopupUrl(TEST_DATA.PAGE_URL)) await page.waitForLoadState('networkidle') // Act const headline = page.getByRole('heading', { name: label(m.educationLicense), }) const licenseRenewLink = page .locator(`[data-testid="action-card-cta"]`) .getByRole('button') .first() // "Leyfisbréf - Kennari" comes from the api - not translateable const licenseName = page.getByText(TEST_DATA.LICENSE_NAME).first()
44-53: 💡 Codebase verification
Interactive elements need accessibility improvements
The test is using proper role-based selectors, but the implementation shows potential accessibility gaps:
- The button in
data-testid="action-card-cta"lacks an explicitaria-labelto describe its purpose- The text "Kennari" is used for visual display but may not be properly associated with the interactive elements for screen readers
To improve accessibility:
- Add descriptive
aria-labelto the license renewal button- Ensure the license name "Kennari" is properly associated with related interactive elements using
aria-labelledbyoraria-describedby🔗 Analysis chain
Verify accessibility of interactive elements.
The test uses role-based selectors which is good for accessibility. Let's verify that all interactive elements have proper ARIA attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for aria-label attributes in the codebase for work license related components rg -g '*.tsx' -g '*.ts' 'aria-label.*(?:license|leyfi)' apps/portals/my-pages/ # Search for role attributes in the codebase for work license related components rg -g '*.tsx' -g '*.ts' 'role=["'\''](?:button|link)["'\''].*(?:license|leyfi)' apps/portals/my-pages/Length of output: 185
Script:
#!/bin/bash # Let's try a broader search for accessibility attributes in the work license components rg -g '*.tsx' 'aria-|role=' apps/portals/my-pages/src/routes/work-license/ -A 2 -B 2 # Search for the ActionCard component definition since it's used in the test ast-grep --pattern 'const ActionCard = $_' # Also search for ActionCard usage with accessibility props rg -g '*.tsx' '<ActionCard.*>' -A 3 -B 3Length of output: 14301
Script:
#!/bin/bash # Let's check the actual implementation of the work license page component rg -g '*.tsx' -g '*.ts' 'data-testid="action-card-cta"' -A 5 -B 5 # Let's also check for Button component usage with accessibility attributes ast-grep --pattern 'Button $$$aria-label$$$' # And check the test file's directory for the component being tested fd work-license apps/portals/my-pages/Length of output: 4373
apps/application-system/form/e2e/smoke/p-sign.spec.ts (3)
15-28: 🛠️ Refactor suggestion
Consider using a URL constant for better maintainability.
The hardcoded path
/umsoknir/p-merkicould be moved to a constant or URL configuration object. This would make it easier to update paths across tests and maintain consistency.// Add to a shared constants file or URL configuration export const URLS = { pSign: { home: '/umsoknir/p-merki', }, } as const; // Then use in the test homeUrl: URLS.pSign.home,
43-51: 🛠️ Refactor suggestion
Simplify input field interactions and use test constants.
The current implementation has several areas for improvement:
- Input clearing could be simplified using
fill()- Hardcoded test values should be moved to constants
- Missing assertions after form submission
// Add to test constants file export const TEST_DATA = { pSign: { phone: '7654321', email: 'secret@island.is', }, } as const; // Simplified implementation const phoneLocator = page.locator('input[name="phone"]'); await phoneLocator.fill(TEST_DATA.pSign.phone); const emailLocator = page.locator('input[name="email"]'); await emailLocator.fill(TEST_DATA.pSign.email); await page.locator('button[type="submit"]').click(); // Add assertions await expect(page).toHaveURL(/\/success$/); await expect(page.locator('text=Application submitted')).toBeVisible();
29-32:
⚠️ Potential issueAdd error handling for page navigation.
The page navigation should include error handling and timeout configuration to make the test more robust.
- await page.goto('/umsoknir/p-merki?delegationChecked=true') + try { + await page.goto('/umsoknir/p-merki?delegationChecked=true', { + waitUntil: 'networkidle', + timeout: 30000, + }); + } catch (error) { + throw new Error(`Failed to navigate to P-sign page: ${error.message}`); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test('should be able to create application', async () => { const page = await context.newPage() try { await page.goto('/umsoknir/p-merki?delegationChecked=true', { waitUntil: 'networkidle', timeout: 30000, }); } catch (error) { throw new Error(`Failed to navigate to P-sign page: ${error.message}`); }apps/portals/my-pages/e2e/smoke/university.spec.ts (6)
13-13: 🛠️ Refactor suggestion
Consider moving homeUrl to a constants file.
For better reusability and maintainability, consider moving the
homeUrlconstant to a shared constants file within the e2e test utilities.-const homeUrl = `${urls.islandisBaseUrl}/minarsidur`Create a new constant in a shared location:
// e2e/constants.ts export const PORTAL_URLS = { HOME: (baseUrl: string) => `${baseUrl}/minarsidur`, // Add other portal URLs here }
19-27: 🛠️ Refactor suggestion
Enhance type safety for session configuration.
Consider creating a type-safe interface for the session configuration to prevent potential errors and improve maintainability.
interface SessionConfig { browser: Browser; storageState: string; homeUrl: string; phoneNumber: string; idsLoginOn: boolean; } // Then use it in the session function: const config: SessionConfig = { browser, storageState: 'service-portal-finnland.json', homeUrl, phoneNumber: '0102209', idsLoginOn: true, } await session(config)
41-43: 🛠️ Refactor suggestion
Extract URL path to a constant.
The URL path should be extracted to a constant to maintain consistency and make updates easier.
const GRADUATION_PATHS = { OVERVIEW: '/minarsidur/menntun/haskoli/brautskraning', DETAIL: (id: string) => `/minarsidur/menntun/haskoli/brautskraning/${id}`, } as const; await page.goto(icelandicAndNoPopupUrl(GRADUATION_PATHS.OVERVIEW));
46-48: 🛠️ Refactor suggestion
Create a Page Object for selectors.
Extract selectors into a Page Object Model for better maintainability and reusability.
class GraduationPage { constructor(private page: Page) {} viewDetailsButton = () => this.page .locator(`role=button[name="${label(m.view)}"]`) .last() title = () => this.page.getByRole('heading', { name: label(m.educationGraduation), }) schoolName = () => this.page.getByText('Háskóli Íslands') }
59-61: 🛠️ Refactor suggestion
Improve URL pattern assertion.
The URL pattern check could be more specific and type-safe.
const GRADUATION_DETAIL_URL_PATTERN = new RegExp( `^${GRADUATION_PATHS.OVERVIEW}/\\d+$` ); await expect(page).toHaveURL(GRADUATION_DETAIL_URL_PATTERN);
44-44: 🛠️ Refactor suggestion
Add timeout and error handling for network requests.
Consider adding a timeout and error handling for the network request wait.
-await page.waitForLoadState('networkidle') +try { + await page.waitForLoadState('networkidle', { timeout: 10000 }) +} catch (error) { + throw new Error(`Network did not stabilize: ${error.message}`) +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { await page.waitForLoadState('networkidle', { timeout: 10000 }) } catch (error) { throw new Error(`Network did not stabilize: ${error.message}`) }apps/portals/my-pages/e2e/acceptance/assets.spec.ts (3)
15-28: 🛠️ Refactor suggestion
Consider parameterizing test configuration
The session setup contains hardcoded values that could make tests brittle and difficult to maintain.
Consider extracting configuration to a separate file:
// test-config.ts export const TEST_CONFIG = { storageState: process.env.STORAGE_STATE || 'service-portal-othekkt.json', phoneNumber: process.env.TEST_PHONE || '0104359', // ... other config }Then update the session setup:
- context = await session({ - browser: browser, - storageState: 'service-portal-othekkt.json', - homeUrl: `${urls.islandisBaseUrl}/minarsidur`, - phoneNumber: '0104359', - idsLoginOn: true, - }) + context = await session({ + browser: browser, + storageState: TEST_CONFIG.storageState, + homeUrl: `${urls.islandisBaseUrl}/minarsidur`, + phoneNumber: TEST_CONFIG.phoneNumber, + idsLoginOn: true, + })
30-51: 🛠️ Refactor suggestion
Consider enhancing test robustness
The test follows good practices but could be improved in a few areas:
- Extract timeout to a constant:
+ const VISIBILITY_TIMEOUT = 10000; - await expect(headline).toBeVisible({ timeout: 10000 }) + await expect(headline).toBeVisible({ timeout: VISIBILITY_TIMEOUT })
- Add more assertions to verify the page state:
// Add these assertions await expect(page).toHaveURL(/.*\/fasteignir$/); await expect(page.getByTestId('action-card-cta')).toHaveCount(1);
53-72: 🛠️ Refactor suggestion
Consider improving test data management
The test uses hardcoded test data which could make maintenance difficult.
Consider creating a test data helper:
// test-data.ts export const TEST_PROPERTIES = { default: { id: 'F12345', displayNumber: 'RVK - F12345', url: (baseUrl: string) => `${baseUrl}/minarsidur/eignir/fasteignir/${TEST_PROPERTIES.default.id}`, } };Then update the test:
- await page.goto( - icelandicAndNoPopupUrl('/minarsidur/eignir/fasteignir/F12345'), - ) + await page.goto( + icelandicAndNoPopupUrl(TEST_PROPERTIES.default.url(urls.islandisBaseUrl)), + ) - const propertyNumber = page.getByText('RVK - F12345') + const propertyNumber = page.getByText(TEST_PROPERTIES.default.displayNumber)apps/portals/admin/e2e/smoke/tenants.spec.ts (3)
15-22: 🛠️ Refactor suggestion
Consider extracting configuration values
The storage state file name and phone number are hardcoded. Consider moving these to a configuration file for better maintainability.
+ // Add to a config file (e.g., e2e.config.ts) + export const E2E_CONFIG = { + storageStates: { + serviceFaereyjar: 'service-portal-faereyjar.json' + }, + testPhoneNumbers: { + default: '0102399' + } + } test.beforeAll(async ({ browser }) => { contextGranter = await session({ browser, - storageState: 'service-portal-faereyjar.json', + storageState: E2E_CONFIG.storageStates.serviceFaereyjar, homeUrl, - phoneNumber: '0102399', + phoneNumber: E2E_CONFIG.testPhoneNumbers.default, }) })Committable suggestion skipped: line range outside the PR's diff.
28-74: 🛠️ Refactor suggestion
Consider enhancing test robustness and accessibility
A few suggestions to improve the test:
- Add navigation error handling
- Include accessibility checks for the tenant management interface
test('can manage tenants', async () => { const page = await contextGranter.newPage() + // Add navigation error handling + await Promise.all([ + page.waitForNavigation({ waitUntil: 'networkidle' }), page.goto(homeUrl) + ]).catch(error => { + throw new Error(`Navigation failed: ${error.message}`) + }) + // Add accessibility checks + await test.step('Check accessibility', async () => { + const accessibilitySnapshot = await page.accessibility.snapshot() + expect(accessibilitySnapshot).toBeTruthy() + + // Check if search input is properly labeled + await expect(page.getByPlaceholder('Search')) + .toHaveAttribute('aria-label', 'Search tenants') + })Committable suggestion skipped: line range outside the PR's diff.
40-68: 🛠️ Refactor suggestion
Refactor duplicated test data
The filter value '@island.is' is duplicated across test steps. Consider extracting it to a constant.
+ const TEST_TENANT_ID = '@island.is' + const TEST_TENANT_NAME = 'Mínar síður Ísland.is' await test.step('Filter tenants list by name or id', async () => { await page .getByPlaceholder('Search') - .fill('@island.is') + .fill(TEST_TENANT_ID) await expect( page.getByRole('link', { - name: 'Mínar síður Ísland.is @island.is', + name: `${TEST_TENANT_NAME} ${TEST_TENANT_ID}`, }), ).toBeVisible() }) await test.step('To link to tenant applications page', async () => { await page .getByPlaceholder('Search') - .fill('@island.is') + .fill(TEST_TENANT_ID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const TEST_TENANT_ID = '@island.is' const TEST_TENANT_NAME = 'Mínar síður Ísland.is' await test.step('Filter tenants list by name or id', async () => { // Arrange await page .getByPlaceholder('Search') // filter by id .fill(TEST_TENANT_ID) // Assert await expect( page.getByRole('link', { name: `${TEST_TENANT_NAME} ${TEST_TENANT_ID}`, }), ).toBeVisible() await expect(page.getByTestId('tenant-list-item')).toBeVisible() await expect(page.getByTestId('tenant-list-item')).toHaveCount(1) }) // await test.step('To link to tenant applications page', async () => { // Arrange await page .getByPlaceholder('Search') // filter by id .fill(TEST_TENANT_ID) // Act await page.getByTestId('tenant-list-item').click()apps/application-system/form/e2e/acceptance/driving-assessment-approval.spec.ts (3)
41-67: 🛠️ Refactor suggestion
Improve test organization and interactions
A few suggestions to enhance the test:
- Consider breaking down the test steps into helper functions for better readability
- The double-click action on the error message seems unnecessary
- Consider adding assertions between steps to verify the state
Example refactor:
async function fillDataProviders(page: Page) { await page.getByTestId('agree-to-data-providers').click() await page.getByTestId('proceed').click() await expect(page).toHaveURL(/.*student-info/) } async function fillStudentInfo(page: Page) { await page.getByLabel('Kennitala nemanda').fill('0101307789') await page.getByLabel('Netfang').fill('email@domain.test') await expect(page.getByText('Gervimaður útlönd')).toBeVisible() await page.getByTestId('proceed').click() }Also, replace the double-click with a single click:
- .dblclick() + .click()
10-10: 🛠️ Refactor suggestion
Consider moving the URL to a constants file
Moving the hardcoded URL to a shared constants file would improve maintainability and reusability across test files.
-const homeUrl = '/umsoknir/akstursmat' +import { ROUTES } from '../constants' +const homeUrl = ROUTES.DRIVING_ASSESSMENTCommittable suggestion skipped: line range outside the PR's diff.
35-68: 💡 Codebase verification
Additional test cases needed for driving assessment approval
The current test suite only covers a single happy path scenario that ends with an error message for a student with a full license. Based on the implementation, additional test cases should be added to cover:
- Invalid student kennitala validation
- Missing or invalid email format
- Missing confirmation checkbox
- Different student license statuses
- Navigation between steps (back/forward)
- Form submission for eligible students
🔗 Analysis chain
Consider adding more test cases
The test suite currently only has one test case. Consider adding more test cases to cover:
- Validation errors
- Different student scenarios
- Edge cases
Let's check if there are similar test files we can reference:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find similar test files in the application system fd "spec.ts$" apps/application-system/form/e2e/acceptance/ --exec rg -l "applicationTest.describe"Length of output: 1319
Script:
#!/bin/bash # Let's check the test cases in similar files to understand the test coverage patterns rg "applicationTest\(" apps/application-system/form/e2e/acceptance/driving-licence.spec.ts -A 2 rg "applicationTest\(" apps/application-system/form/e2e/acceptance/driving-instructor-registrations.spec.ts -A 2Length of output: 466
Script:
#!/bin/bash # Let's examine the full content of driving-assessment-approval.spec.ts to understand the current test coverage cat apps/application-system/form/e2e/acceptance/driving-assessment-approval.spec.tsLength of output: 2098
apps/portals/my-pages/e2e/acceptance/occupational-licenses.spec.ts (5)
33-37: 🛠️ Refactor suggestion
Move test data to a fixture file.
Consider extracting the licenses array to a separate test fixture file to improve maintainability and reusability.
Create a new file
fixtures/occupational-licenses.ts:export const testLicenses = [ 'Kennari test', 'Bebeb - Starfsleyfi', 'Verðbréfaréttindi', ]
54-58: 🛠️ Refactor suggestion
Standardize test data structure.
The licenses array structure differs from the previous test step. Consider using a consistent data structure throughout the test file and moving it to a shared fixture.
In the suggested
fixtures/occupational-licenses.ts:export const testLicenseDetails = [ { title: 'Bebeb', ref: 'H13' }, { title: 'Kennari test', ref: '123' }, { title: 'Verðbréfaréttindi', ref: 'D123' }, ]
61-64: 🛠️ Refactor suggestion
Consider using more robust selectors.
Using partial href matching could be fragile if the URL structure changes. Consider using data-testid attributes for more stable element selection.
- const ref = `[href*="${license.ref}"]` - const element = page.locator(ref) + const element = page.getByTestId(`license-${license.ref}`)Add the corresponding attribute to your component:
<Link data-testid={`license-${licenseRef}`} ...>
17-25:
⚠️ Potential issueAvoid hardcoding test credentials.
The phone number should be moved to an environment variable or test configuration file to improve security and maintainability.
- phoneNumber: '0102399', + phoneNumber: process.env.TEST_PHONE_NUMBER,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.beforeAll(async ({ browser }) => { context = await session({ browser: browser, storageState: 'service-portal-faereyjar.json', homeUrl, phoneNumber: process.env.TEST_PHONE_NUMBER, idsLoginOn: true, }) })
48-48: 💡 Codebase verification
30-second timeout is inconsistent with other tests and should be reduced
Based on the codebase analysis:
- Most visibility checks use 10-20 second timeouts (e.g.,
assets.spec.tsuses 10000ms,auth.spec.tsuses 20000ms)- Default timeout in
addons.tsis set to 10000ms- The 30-second timeout is only used in
occupational-licenses.spec.tsandlicenses.spec.tsConsider reducing the timeout to 20 seconds to maintain consistency with other tests in the codebase. If elements consistently take longer than 20 seconds to appear, this might indicate a performance issue that should be investigated.
🔗 Analysis chain
Review the timeout duration.
A 30-second timeout for element visibility might indicate potential performance issues. Consider:
- Reducing the timeout to a more reasonable value (e.g., 5-10 seconds)
- Adding performance assertions to ensure the page loads within expected timeframes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other timeout values in e2e tests for consistency rg -A 1 "timeout:" "apps/**/e2e/**/*.ts"Length of output: 100
Script:
#!/bin/bash # Search for timeout values in e2e tests using a more flexible path pattern rg -A 1 "timeout:" apps/portals/my-pages/e2e/Length of output: 1213
Script:
#!/bin/bash # Search for waitFor patterns in e2e tests rg -A 1 "waitFor" apps/portals/my-pages/e2e/Length of output: 4060
Script:
#!/bin/bash # Find all e2e test files to ensure we're searching in the right place fd . apps/portals/my-pages/e2e/ -e tsLength of output: 1399
apps/portals/my-pages/e2e/smoke/service-portal.spec.ts (4)
51-62: 🛠️ Refactor suggestion
Make language change assertions more specific.
The current regex pattern
/(?:day|evening)/is quite loose. Consider making the assertions more specific:-await expect(greeting).toContainText(/(?:day|evening)/) +const expectedEnglishGreetings = ['Good morning', 'Good afternoon', 'Good evening'] +await expect(greeting).toContainText(new RegExp(expectedEnglishGreetings.join('|'))) + +// Verify URL reflects language change +await expect(page).toHaveURL(/\?lang=en/)Committable suggestion skipped: line range outside the PR's diff.
43-50: 🛠️ Refactor suggestion
Add URL verification after navigation.
Consider adding a URL check after clicking the Pósthólf link to ensure proper navigation:
await findByRole('link', 'Pósthólf').click() +await expect(page).toHaveURL(/.*\/minarsidur\/posthorlf/) await expect(page.getByText(label(m.pickDocument))).toBeVisible()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test('should have Pósthólf', async () => { const page = await context.newPage() await disableI18n(page) const { findByRole } = helpers(page) await page.goto(icelandicAndNoPopupUrl(servicePortalHome)) await findByRole('link', 'Pósthólf').click() await expect(page).toHaveURL(/.*\/minarsidur\/posthorlf/) await expect(page.getByText(label(m.pickDocument))).toBeVisible() })
37-42: 🛠️ Refactor suggestion
Consider adding more comprehensive login verification.
While the basic login check is good, consider adding assertions for other user-specific elements to ensure complete login verification:
await page.goto(icelandicAndNoPopupUrl(servicePortalHome)) await expect(findByRole('heading', 'Gervimaður Afríka')).toBeVisible() +// Verify user-specific elements +await expect(findByRole('button', { name: 'Útskrá' })).toBeVisible() +await expect(page).toHaveURL(/.*\/minarsidur/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test('should have user Gervimaður Afríka logged in', async () => { const page = await context.newPage() const { findByRole } = helpers(page) await page.goto(icelandicAndNoPopupUrl(servicePortalHome)) await expect(findByRole('heading', 'Gervimaður Afríka')).toBeVisible() // Verify user-specific elements await expect(findByRole('button', { name: 'Útskrá' })).toBeVisible() await expect(page).toHaveURL(/.*\/minarsidur/) })
16-30: 🛠️ Refactor suggestion
Consider using environment variables for test configuration.
While the context setup is well-structured, consider moving the configuration values to environment variables for better maintainability:
- Phone number: '0103019'
- Storage state file: 'service-portal-afrika.json'
context = await session({ browser: browser, - storageState: 'service-portal-afrika.json', + storageState: process.env.SERVICE_PORTAL_STORAGE_STATE || 'service-portal-afrika.json', homeUrl: `${urls.islandisBaseUrl}/minarsidur`, - phoneNumber: '0103019', + phoneNumber: process.env.TEST_PHONE_NUMBER || '0103019', idsLoginOn: true, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('Service portal', () => { let context: BrowserContext test.beforeAll(async ({ browser }) => { context = await session({ browser: browser, storageState: process.env.SERVICE_PORTAL_STORAGE_STATE || 'service-portal-afrika.json', homeUrl: `${urls.islandisBaseUrl}/minarsidur`, phoneNumber: process.env.TEST_PHONE_NUMBER || '0103019', idsLoginOn: true, }) }) test.afterAll(async () => { await context.close() }) const servicePortalHome = icelandicAndNoPopupUrl('/minarsidur')apps/application-system/form/e2e/acceptance/no-debt-certificate.spec.ts (3)
12-31: 🛠️ Refactor suggestion
Consider moving homeUrl to a shared constants file.
The
homeUrlconstant could be reused across different test files. Consider moving it to a shared constants file to maintain DRY principles and ensure consistency.Create a new file
apps/application-system/form/e2e/constants.ts:export const ROUTES = { NO_DEBT_CERTIFICATE: '/umsoknir/skuldleysisvottord' } as constThen update the import:
+import { ROUTES } from '../constants' -const homeUrl = '/umsoknir/skuldleysisvottord' +const homeUrl = ROUTES.NO_DEBT_CERTIFICATE
33-48: 🛠️ Refactor suggestion
Extract test data to a separate configuration file.
The test contains hardcoded values that should be moved to a test data configuration file for better maintainability and reusability.
Create a new file
apps/application-system/form/e2e/test-data.ts:export const TEST_DATA = { COMPANY: { NAME: 'Alla leið ehf' }, USER: { PHONE: '0103019' } } as constThen update the usage:
+import { TEST_DATA } from '../test-data' // In session configuration -phoneNumber: '0103019', +phoneNumber: TEST_DATA.USER.PHONE, // In test -await page.getByRole('button', { name: 'Alla leið ehf' }).click() +await page.getByRole('button', { name: TEST_DATA.COMPANY.NAME }).click()
53-60: 🛠️ Refactor suggestion
Add retry logic for application submission.
The application submission step could be flaky due to network conditions. Consider adding retry logic for better test stability.
await applicationTest.step( 'Proceed with the application and reach the submitted state', async () => { + const maxRetries = 3; + let attempt = 0; + while (attempt < maxRetries) { + try { await page.getByTestId('agree-to-data-providers').check() await page.getByTestId('proceed').click() await expect(page.getByText('Gervimaður Afríka')).toBeVisible() + break; + } catch (error) { + attempt++; + if (attempt === maxRetries) throw error; + await page.waitForTimeout(1000); // Wait before retry + } + } }, )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await applicationTest.step( 'Proceed with the application and reach the submitted state', async () => { const maxRetries = 3; let attempt = 0; while (attempt < maxRetries) { try { await page.getByTestId('agree-to-data-providers').check() await page.getByTestId('proceed').click() await expect(page.getByText('Gervimaður Afríka')).toBeVisible() break; } catch (error) { attempt++; if (attempt === maxRetries) throw error; await page.waitForTimeout(1000); // Wait before retry } } }, )apps/consultation-portal/infra/samradsgatt.ts (1)
31-31: 🛠️ Refactor suggestion
Consider adding development-only security headers.
For local development, consider adding security headers to prevent accidental exposure of development endpoints.
Add the following configuration to your local development server:
// Add these headers for local development const devHeaders = { 'X-Development-Server': 'true', 'X-Frame-Options': 'DENY', 'X-Content-Type-Options': 'nosniff' }apps/application-system/form/e2e/acceptance/endorsement-list.spec.ts (4)
36-37: 🛠️ Refactor suggestion
Consider moving test data to constants.
Test data like title and description should be moved to constants or test fixtures for better maintainability and reusability.
const TEST_DATA = { title: 'System E2E Playwright Test listi title', description: 'System E2E Playwright Test listi description', } as const;
40-40: 🛠️ Refactor suggestion
Add navigation timeout handling.
Consider adding timeout and navigation error handling for better test reliability.
-await page.goto(icelandicAndNoPopupUrl('/umsoknir/undirskriftalisti')) +await Promise.race([ + page.goto(icelandicAndNoPopupUrl('/umsoknir/undirskriftalisti')), + page.waitForTimeout(30000).then(() => { + throw new Error('Navigation timeout') + }) +])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await Promise.race([ page.goto(icelandicAndNoPopupUrl('/umsoknir/undirskriftalisti')), page.waitForTimeout(30000).then(() => { throw new Error('Navigation timeout') }) ])
34-35: 🛠️ Refactor suggestion
Improve date handling using date-fns utilities.
The manual date calculation could be replaced with date-fns utilities for better readability and maintenance.
-const today = new Date() -const tomorrow = new Date(new Date().getTime() + 24 * 60 * 60 * 1000) +import { addDays } from 'date-fns' +const today = new Date() +const tomorrow = addDays(today, 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { addDays } from 'date-fns' const today = new Date() const tomorrow = addDays(today, 1)
64-70:
⚠️ Potential issueImprove date input handling reliability.
The current implementation using keyboard.press('Enter') could be flaky. Consider using Playwright's built-in date input handling or waiting for the date picker to close.
-await page.getByLabel('Tímabil lista').first().fill(format(today, 'dd.MM.yyyy')) -await page.keyboard.press('Enter') -await page.getByLabel('Tímabil lista').last().fill(format(tomorrow, 'dd.MM.yyyy')) -await page.keyboard.press('Enter') +await page.getByLabel('Tímabil lista').first().fill(format(today, 'dd.MM.yyyy')) +await page.getByLabel('Tímabil lista').last().fill(format(tomorrow, 'dd.MM.yyyy')) +await expect(page.getByRole('dialog')).not.toBeVisible()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements..fill(format(today, 'dd.MM.yyyy')) await page .getByLabel('Tímabil lista') .last() .fill(format(tomorrow, 'dd.MM.yyyy')) await expect(page.getByRole('dialog')).not.toBeVisible()apps/portals/my-pages/e2e/smoke/work-machines.spec.ts (3)
20-28: 🛠️ Refactor suggestion
Add type safety for session configuration.
Consider creating an interface for the session configuration to ensure type safety and document the purpose of the storage state file.
interface SessionConfig { browser: Browser; storageState: string; homeUrl: string; phoneNumber: string; idsLoginOn: boolean; } // Add JSDoc comment explaining the purpose of amerika.json /** * Sets up a browser session with pre-authenticated state for testing. * @param config Session configuration * @remarks 'service-portal-amerika.json' contains auth tokens for test user */
65-80: 🛠️ Refactor suggestion
Improve URL assertion reliability in detail test.
The current URL assertion using regex could be fragile. Consider using a more specific assertion or extracting the pattern to a constant.
+ const DETAIL_URL_PATTERN = /minarsidur\/eignir\/vinnuvelar(\/[^/]+){2}/; await test.step('should display detail', async () => { // Arrange - await page.goto(icelandicAndNoPopupUrl('/minarsidur/eignir/vinnuvelar')) + try { + await page.goto(icelandicAndNoPopupUrl('/minarsidur/eignir/vinnuvelar')) + } catch (error) { + throw new Error(`Failed to navigate to work machines page: ${error}`) + } // Act const actionCardButton = page.getByTestId('action-card-cta').first() await actionCardButton.click() const basicInfoText = page .getByText(label(messages.baseInfoWorkMachineTitle)) .first() // Assert - await expect(page).toHaveURL(/minarsidur\/eignir\/vinnuvelar(\/[^/]+){2}/) + await expect(page).toHaveURL(DETAIL_URL_PATTERN) await expect(basicInfoText).toBeVisible() })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const DETAIL_URL_PATTERN = /minarsidur\/eignir\/vinnuvelar(\/[^/]+){2}/; await test.step('should display detail', async () => { // Arrange try { await page.goto(icelandicAndNoPopupUrl('/minarsidur/eignir/vinnuvelar')) } catch (error) { throw new Error(`Failed to navigate to work machines page: ${error}`) } // Act const actionCardButton = page.getByTestId('action-card-cta').first() await actionCardButton.click() const basicInfoText = page .getByText(label(messages.baseInfoWorkMachineTitle)) .first() // Assert await expect(page).toHaveURL(DETAIL_URL_PATTERN) await expect(basicInfoText).toBeVisible() })
34-63: 🛠️ Refactor suggestion
Improve test reliability and maintainability in overview test.
Consider the following improvements:
- Extract the type delay (200ms) to a named constant
- Add error handling for navigation
- Consider using test data constants instead of hardcoded values
+ const TYPE_DELAY_MS = 200; + const TEST_SEARCH_TERM = 'hys'; test('work machines', async () => { const page = await context.newPage() await disableI18n(page) await test.step('should display data and filter overview', async () => { // Arrange - await page.goto(icelandicAndNoPopupUrl('/minarsidur/eignir/vinnuvelar')) + try { + await page.goto(icelandicAndNoPopupUrl('/minarsidur/eignir/vinnuvelar')) + } catch (error) { + throw new Error(`Failed to navigate to work machines page: ${error}`) + } // Act const filterButton = page .getByRole('button', { name: label(m.openFilter) }) .first() const inputField = page.getByRole('textbox', { name: label(m.searchLabel), }) await inputField.click() - await inputField.type('hys', { delay: 200 }) + await inputField.type(TEST_SEARCH_TERM, { delay: TYPE_DELAY_MS })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const TYPE_DELAY_MS = 200; const TEST_SEARCH_TERM = 'hys'; test('work machines', async () => { const page = await context.newPage() await disableI18n(page) await test.step('should display data and filter overview', async () => { // Arrange try { await page.goto(icelandicAndNoPopupUrl('/minarsidur/eignir/vinnuvelar')) } catch (error) { throw new Error(`Failed to navigate to work machines page: ${error}`) } // Act const filterButton = page .getByRole('button', { name: label(m.openFilter) }) .first() const inputField = page.getByRole('textbox', { name: label(m.searchLabel), }) await inputField.click() await inputField.type(TEST_SEARCH_TERM, { delay: TYPE_DELAY_MS }) const actionCardButton = page.getByTestId('action-card-cta').first() const closeFilter = page .getByRole('button', { name: label(m.closeFilter) }) .first() await filterButton.click() // Assert await expect(closeFilter).toBeVisible() await expect(actionCardButton).toBeVisible() await expect(page.getByTestId('icon-reload').first()).toBeVisible() })apps/portals/my-pages/e2e/smoke/auth.spec.ts (2)
31-44:
⚠️ Potential issueFix undefined
switchDelegationfunction.The test uses
switchDelegationfunction but it's not imported or defined. This will cause a runtime error.Either import the function:
import { BrowserContext, expect, Page, test, icelandicAndNoPopupUrl, urls, session, + switchDelegation, } from '@island.is/testing/e2e'Or define it if it's a local helper function.
Committable suggestion skipped: line range outside the PR's diff.
14-29: 🛠️ Refactor suggestion
Consider adding error handling in setup and teardown.
While the setup and teardown are well-structured, consider adding try-catch blocks to handle potential session initialization failures gracefully.
test.beforeAll(async ({ browser }) => { + try { context = await session({ browser: browser, storageState: 'service-portal-auth.json', homeUrl, phoneNumber: '0104359', idsLoginOn: true, }) + } catch (error) { + console.error('Failed to initialize session:', error) + throw error + } }) test.afterAll(async () => { + try { await context.close() + } catch (error) { + console.error('Failed to close context:', error) + } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('Service portal', () => { let context: BrowserContext test.beforeAll(async ({ browser }) => { try { context = await session({ browser: browser, storageState: 'service-portal-auth.json', homeUrl, phoneNumber: '0104359', idsLoginOn: true, }) } catch (error) { console.error('Failed to initialize session:', error) throw error } }) test.afterAll(async () => { try { await context.close() } catch (error) { console.error('Failed to close context:', error) } })apps/portals/admin/e2e/smoke/tenant-applications.spec.ts (2)
9-11: 🛠️ Refactor suggestion
Consider extracting URL construction to a constants file.
The URL construction logic could be moved to a shared constants file to promote reusability and maintainability. This would also make it easier to update the base path if needed.
-const homeUrl = `${ - urls.islandisBaseUrl -}/stjornbord/innskraningarkerfi/${encodeURIComponent('@island.is')}/forrit`Create a new file
apps/portals/admin/e2e/constants.ts:export const ADMIN_URLS = { getTenantApplicationsUrl: (tenant: string) => `${urls.islandisBaseUrl}/stjornbord/innskraningarkerfi/${encodeURIComponent(tenant)}/forrit` }
30-93: 🛠️ Refactor suggestion
Enhance test maintainability with constants and fixtures.
The test implementation could benefit from the following improvements:
- Extract repeated values to constants
- Move test data to fixtures
- Use data-testid for more reliable selectors
+// Create a new file: apps/portals/admin/e2e/fixtures/applications.ts +export const TEST_APPLICATIONS = { + web: { + id: '@island.is/web', + name: 'Mínar síður Ísland.is', + } +}; + +// Create a new file: apps/portals/admin/e2e/constants.ts +export const SELECTORS = { + search: '[data-testid="application-search"]', +}; test('can manage applications', async () => { + const { web } = TEST_APPLICATIONS; // ... existing code ... await test.step('Filter applications list by name or id', async () => { await page - .getByPlaceholder('Search') + .locator(SELECTORS.search) // filter by id - .fill('Mínar síður') + .fill(web.name) }); await test.step('To link to application page', async () => { - const applicationId = '@island.is/web' + const { id } = web; // ... use id instead of applicationId });Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/inbox.spec.ts (5)
18-34: 🛠️ Refactor suggestion
Consider parameterizing test data
The test setup uses hardcoded values for storage state and phone number. Consider moving these to test configuration or environment variables for better maintainability.
- storageState: 'service-portal-bandarikin.json', - phoneNumber: '0105069', + storageState: process.env.TEST_STORAGE_STATE || 'service-portal-bandarikin.json', + phoneNumber: process.env.TEST_PHONE_NUMBER || '0105069',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('MS - Pósthólf overview', () => { let context: BrowserContext test.beforeAll(async ({ browser }) => { context = await session({ browser: browser, storageState: process.env.TEST_STORAGE_STATE || 'service-portal-bandarikin.json', homeUrl, phoneNumber: process.env.TEST_PHONE_NUMBER || '0105069', idsLoginOn: true, }) }) test.afterAll(async () => { await context.close() })
76-76:
⚠️ Potential issueReplace mouse wheel with more stable interaction
Mouse wheel interactions can be unstable across different environments. Consider using keyboard navigation or direct element interaction.
- await page.mouse.wheel(0, 50) + // If scrolling is needed, use more stable methods: + await page.evaluate(() => { + document.querySelector('.institution-list')?.scrollBy(0, 50) + })Committable suggestion skipped: line range outside the PR's diff.
54-54:
⚠️ Potential issueReplace arbitrary sleep with proper wait conditions
Using arbitrary sleep durations can make tests flaky. Consider using Playwright's built-in wait conditions.
- await sleep(500) + await expect(docFoundText).toBeVisible({ timeout: 5000 })Committable suggestion skipped: line range outside the PR's diff.
79-80: 🛠️ Refactor suggestion
Extract test data to constants
Hardcoded institution name should be moved to a test constants file for better maintainability.
+ // Move to test-constants.ts + const TEST_INSTITUTIONS = { + POLICE: 'Ríkislögreglustjórinn' + } - const institution = 'Ríkislögreglustjórinn' + const institution = TEST_INSTITUTIONS.POLICECommittable suggestion skipped: line range outside the PR's diff.
35-91: 💡 Codebase verification
🛠️ Refactor suggestion
Network request handling should be added to improve test reliability
The test interacts with the inbox API but doesn't verify the network requests. Based on the codebase analysis:
- Other tests in the codebase use
waitForResponsefor API verification- The inbox test performs filtering operations that depend on API responses
- No existing network request handling is implemented for the inbox filtering operations
Add request handling for both filter operations:
// For search filter await Promise.all([ page.waitForResponse(response => response.url().includes('/api/documents') && response.status() === 200 ), inputField.press('Enter') ]); // For institution filter await Promise.all([ page.waitForResponse(response => response.url().includes('/api/documents') && response.status() === 200 ), page.getByLabel(institution).click() ]);🔗 Analysis chain
Add network request handling
The test should handle potential network failures when interacting with the inbox API.
Add request handling:
await Promise.all([ page.waitForResponse(response => response.url().includes('/api/documents') && response.status() === 200 ), inputField.press('Enter') ]);
Let me gather more information about the inbox API endpoints and network requests in the codebase.
Let me check for any API-related code or network request handling patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the test uses request handling utilities rg "waitForResponse|route\." "apps/portals/my-pages/e2e"Length of output: 332
Script:
#!/bin/bash # Search for inbox API endpoints and network request handling in tests rg -A 5 "api/documents|/postholf" "apps/portals/my-pages"Length of output: 990
Script:
#!/bin/bash # Search for API-related code and network request patterns rg -A 5 "api.*documents|waitForResponse.*documents" "apps/portals/my-pages" # Check for any existing network request handling patterns in tests ast-grep --pattern 'await Promise.all([ $$$ page.waitForResponse($_), $$$ ])'Length of output: 583
apps/portals/my-pages/e2e/smoke/information.spec.ts (1)
71-71: 🛠️ Refactor suggestion
Consider more specific wait condition
Using
waitForLoadState('networkidle')might be flaky as it depends on the overall network activity. Consider waiting for specific network requests or UI elements instead.- await page.waitForLoadState('networkidle') + // Wait for specific API response or UI element + await page.waitForResponse(response => + response.url().includes('/api/children') && + response.status() === 200 + )Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/sessions.spec.ts (2)
22-23: 🛠️ Refactor suggestion
Consider moving environment-specific values to a constants file.
The test company name varies by environment. Consider extracting this to a dedicated constants file for better maintainability.
- const testCompanyName = - env === 'staging' ? 'Prófunarfélag GG og HEB' : 'ARTIC ehf.' + const testCompanyName = TEST_COMPANIES[env]Create a new file
test-constants.ts:export const TEST_COMPANIES = { staging: 'Prófunarfélag GG og HEB', production: 'ARTIC ehf.', } as const
55-57: 🛠️ Refactor suggestion
Consider moving test-specific national IDs to constants.
Similar to the company names, these environment-specific national IDs should be moved to a constants file.
- const filterSubjectNationalId = - // eslint-disable-next-line local-rules/disallow-kennitalas - env === 'staging' ? '6609170200' : '5005101370' + const filterSubjectNationalId = TEST_NATIONAL_IDS[env]Add to
test-constants.ts:export const TEST_NATIONAL_IDS = { staging: '6609170200', production: '5005101370', } as constapps/portals/admin/e2e/smoke/tenant-permissions.spec.ts (3)
21-28: 🛠️ Refactor suggestion
Consider making the storage state file path configurable
The storage state file path is hardcoded, which might make it difficult to run tests with different configurations or environments.
+const STORAGE_STATE_FILE = process.env.STORAGE_STATE_FILE || 'service-portal-faereyjar.json' test.beforeAll(async ({ browser }) => { contextGranter = await session({ browser, - storageState: 'service-portal-faereyjar.json', + storageState: STORAGE_STATE_FILE, homeUrl, phoneNumber: '0102399', }) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const STORAGE_STATE_FILE = process.env.STORAGE_STATE_FILE || 'service-portal-faereyjar.json' test.beforeAll(async ({ browser }) => { contextGranter = await session({ browser, storageState: STORAGE_STATE_FILE, homeUrl, phoneNumber: '0102399', }) })
9-13: 🛠️ Refactor suggestion
Consider extracting URL construction to a utility function
The URL construction logic could be moved to a dedicated utility function for better maintainability and reusability. This would also make it easier to update the URL structure if needed.
+const buildHomeUrl = (tenant: string) => + `${urls.islandisBaseUrl}/stjornbord/innskraningarkerfi/${encodeURIComponent(tenant)}/rettindi` -const homeUrl = `${ - urls.islandisBaseUrl -}/stjornbord/innskraningarkerfi/${encodeURIComponent( - '@admin.island.is', -)}/rettindi` +const homeUrl = buildHomeUrl('@admin.island.is')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const buildHomeUrl = (tenant: string) => `${urls.islandisBaseUrl}/stjornbord/innskraningarkerfi/${encodeURIComponent(tenant)}/rettindi` const homeUrl = buildHomeUrl('@admin.island.is')
26-26: 🛠️ Refactor suggestion
Consider making the phone number configurable
The phone number is hardcoded in the session setup. Consider making it configurable through environment variables or test configuration.
+const TEST_PHONE_NUMBER = process.env.TEST_PHONE_NUMBER || '0102399' contextGranter = await session({ browser, storageState: 'service-portal-faereyjar.json', homeUrl, - phoneNumber: '0102399', + phoneNumber: TEST_PHONE_NUMBER, })Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/occupational-licenses.spec.ts (1)
38-64: 🛠️ Refactor suggestion
Refactor duplicate test steps into a parameterized test
The two test steps have identical structure and differ only in the license type being tested. Consider refactoring into a parameterized test for better maintainability.
interface LicenseTest { type: 'health' | 'teacher' title: string urlPath: string selector: 'first' | 'last' } const licenseTests: LicenseTest[] = [ { type: 'health', title: 'Sálfræðingur', urlPath: 'landlaeknir/Sálfræðingur', selector: 'first' }, { type: 'teacher', title: 'Kennari', urlPath: 'mms/Kennari', selector: 'last' } ] for (const testCase of licenseTests) { await test.step( `Can view ${testCase.type} license details`, async () => { await page.goto(icelandicAndNoPopupUrl('/minarsidur/starfsleyfi')) const license = page .locator(`role=button[name="${label(m.view)}"]`) [testCase.selector]() await license.click() const title = page.getByText(testCase.title).first() const isValid = page.getByText('Í gildi').first() const regex = new RegExp(`.*minarsidur/starfsleyfi/${testCase.urlPath}/[0-9]+`) expect(decodeURI(page.url())).toMatch(regex) await expect(title).toBeVisible() await expect(isValid).toBeVisible() } ) }Also applies to: 66-91
apps/portals/my-pages/e2e/smoke/notification-settings.spec.ts (1)
80-81: 🛠️ Refactor suggestion
Consider using a more robust selector for the delegation list
Using test IDs for critical elements is good, but consider using a more semantic selector or combining it with role for better reliability.
-const delegationList = page.getByTestId('actor-profile-settings-list') +const delegationList = page + .getByRole('list') + .filter({ has: page.getByTestId('actor-profile-settings-list') })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const delegationList = page .getByRole('list') .filter({ has: page.getByTestId('actor-profile-settings-list') }) await expect(delegationList).toBeVisible()apps/portals/my-pages/e2e/smoke/company-delegation.spec.ts (2)
55-92: 🛠️ Refactor suggestion
Consider refactoring for better maintainability
The test case has multiple steps that could be extracted into helper functions for better readability and reusability. Additionally, complex locator chains could be simplified.
Consider extracting the delegation selection into a reusable helper:
async function selectDelegation(page: Page, delegationType: string) { await page.locator('data-testid=user-menu >> visible=true').click() await page .locator(`role=button[name="${label(coreDelegationsMessages.delegationErrorButton)}"]`) .click() const delegation = page .locator(`role=button[name*="${label(delegationType)}"]`) .first() await expect(delegation).toBeVisible() await delegation.click() await page.waitForURL(new RegExp(homeUrl), { waitUntil: 'domcontentloaded', }) }Then simplify the test:
test('can view company data', async () => { const page = await context.newPage() await disableI18n(page) await page.goto(icelandicAndNoPopupUrl('/minarsidur')) await selectDelegation(page, m.delegationTypeProcurationHolder) await page .getByRole('link', { name: label(coreMessages.companyTitle) }) .first() .click() await expect(page.getByText(label(mCompany.subtitle)).first()).toBeVisible() await expect(page.getByText('Einkahlutafélag').first()).toBeVisible() })
21-37: 🛠️ Refactor suggestion
Consider parameterizing the storage state filename
The hardcoded storage state filename
'service-portal-faereyjar.json'might make the tests less flexible. Consider making this configurable through environment variables or test parameters for better maintainability.- storageState: 'service-portal-faereyjar.json', + storageState: process.env.SERVICE_PORTAL_STORAGE_STATE || 'service-portal-faereyjar.json',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('Service portal', () => { let context: BrowserContext test.beforeAll(async ({ browser }) => { context = await session({ browser: browser, storageState: process.env.SERVICE_PORTAL_STORAGE_STATE || 'service-portal-faereyjar.json', homeUrl, phoneNumber: '0102399', idsLoginOn: true, }) }) test.afterAll(async () => { await context.close() })apps/portals/my-pages/e2e/smoke/financial-payment-application.spec.ts (2)
62-66: 🛠️ Refactor suggestion
Add error handling for popup interaction.
The popup interaction could fail silently. Consider adding a timeout and error handling.
const [delegationPopup] = await Promise.all([ - page.waitForEvent('popup'), + page.waitForEvent('popup', { timeout: 10000 }).catch((error) => { + throw new Error(`Failed to open delegation popup: ${error.message}`); + }), // Click - Open Link in Popup applicationButton.click(), ])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const [delegationPopup] = await Promise.all([ page.waitForEvent('popup', { timeout: 10000 }).catch((error) => { throw new Error(`Failed to open delegation popup: ${error.message}`); }), // Click - Open Link in Popup applicationButton.click(), ])
36-49: 🛠️ Refactor suggestion
Add retry logic for potential flaky UI interactions.
UI interactions can be flaky. Consider adding retry logic for better test stability.
await test.step('Application button is visible', async () => { // Arrange await page.goto( icelandicAndNoPopupUrl('/minarsidur/fjarmal/greidsluaetlanir'), ) // Act const applicationButton = page.locator( `role=button[name="${label(m.scheduleApplication)}"]`, ) // Assert - await expect(applicationButton).toBeVisible() + await expect(applicationButton).toBeVisible({ timeout: 10000 }) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await test.step('Application button is visible', async () => { // Arrange await page.goto( icelandicAndNoPopupUrl('/minarsidur/fjarmal/greidsluaetlanir'), ) // Act const applicationButton = page.locator( `role=button[name="${label(m.scheduleApplication)}"]`, ) // Assert await expect(applicationButton).toBeVisible({ timeout: 10000 }) })apps/consultation-portal/e2e/smoke/consultation-portal-authenticated.spec.ts (2)
14-29: 🛠️ Refactor suggestion
Consider extracting test configuration to constants.
The hardcoded values like phone number and storage state filename should be moved to a configuration file or constants for better maintainability.
+// Add to a test config file +export const TEST_CONFIG = { + PHONE_NUMBER: '0102989', + AUTH_STORAGE: 'consultation-auth.json', +} test.beforeAll(async ({ browser }) => { context = await session({ browser: browser, - storageState: 'consultation-auth.json', + storageState: TEST_CONFIG.AUTH_STORAGE, idsLoginOn: { nextAuth: { nextAuthRoot: `${urls.islandisBaseUrl}/samradsgatt` }, }, - phoneNumber: '0102989', + phoneNumber: TEST_CONFIG.PHONE_NUMBER,Committable suggestion skipped: line range outside the PR's diff.
35-91: 🛠️ Refactor suggestion
Improve test maintainability and consistency.
Several improvements can be made to the test implementation:
- The page creation/closure pattern is repeated in each test
- Inconsistent test ID naming ('actionCard' vs 'action-card')
- Text-based selectors could be problematic with i18n
Consider these improvements:
+// Add to test utils +const TEST_IDS = { + ACTION_CARD: 'action-card', + SUBSCRIPTIONS_TITLE: 'subscriptions-title', + SUBSCRIPTIONS_BTN: 'subscriptions-btn', +} + +async function withPage(context: BrowserContext, callback: (page: Page) => Promise<void>) { + const page = await context.newPage() + try { + await callback(page) + } finally { + await page.close() + } +} test('advices page should show logged in state', async () => { - const page = await context.newPage() - await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) - - await page.goto('/umsagnir') - await expect(page.getByTestId('actionCard')).not.toBeVisible() - - await page.close() + await withPage(context, async (page) => { + await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) + await page.goto('/umsagnir') + await expect(page.getByTestId(TEST_IDS.ACTION_CARD)).not.toBeVisible() + }) })Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/licenses.spec.ts (3)
77-100:
⚠️ Potential issueImprove selector reliability and add timeout handling.
The child passports test has potential reliability issues:
- Using
last()selector could be fragile if the order changes- Missing timeout for network idle state
- URL assertion could be more specific
test('should display child passports', async () => { - const page = await contextAmerika.newPage() - await disableI18n(page) - await page.goto(icelandicAndNoPopupUrl('/minarsidur/skirteini')) - await page.waitForLoadState('networkidle') + const page = await setupTestPage(contextAmerika) // Act const tabButton = page.getByRole('tab', { name: label(m.licenseTabSecondary), }) await tabButton.click() const childPassportLink = page - .locator(`role=button[name="${label(m.seeDetails)}"]`) - .last() + .locator('data-testid=child-passport-card') + .getByRole('button', { name: label(m.seeDetails) }) await childPassportLink.click() const title1 = page.getByText(label(m.passportName)) // Assert + const expectedUrlPattern = new RegExp( + 'minarsidur/skirteini/tjodskra/vegabref/[a-zA-Z0-9-]+' + ) await expect(page).toHaveURL( - /minarsidur\/skirteini\/tjodskra\/vegabref\/.+/, + expectedUrlPattern, + { timeout: 10000 } )Committable suggestion skipped: line range outside the PR's diff.
16-41: 🛠️ Refactor suggestion
Enhance type safety and error handling in test setup.
Consider the following improvements:
- Add explicit typing for the contexts array
- Add error handling for session creation
- Implement more robust cleanup
- const contexts: BrowserContext[] = [] + const contexts = new Set<BrowserContext>() test.beforeAll(async ({ browser }) => { + try { contextAmerika = await session({ browser: browser, storageState: 'service-portal-amerika.json', homeUrl, phoneNumber: '0102989', idsLoginOn: true, }) contextFaereyjar = await session({ browser: browser, storageState: 'service-portal-faereyjar.json', homeUrl, phoneNumber: '0102399', idsLoginOn: true, }) - contexts.push(contextAmerika, contextFaereyjar) + contexts.add(contextAmerika).add(contextFaereyjar) + } catch (error) { + console.error('Failed to initialize test contexts:', error) + throw error + } }) test.afterAll(async () => { + const closePromises = Array.from(contexts).map(async (context) => { + try { + await context.close() + } catch (error) { + console.error('Failed to close context:', error) + } + }) + await Promise.all(closePromises) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('MS - Skírteini', () => { let contextFaereyjar: BrowserContext let contextAmerika: BrowserContext const contexts = new Set<BrowserContext>() test.beforeAll(async ({ browser }) => { try { contextAmerika = await session({ browser: browser, storageState: 'service-portal-amerika.json', homeUrl, phoneNumber: '0102989', idsLoginOn: true, }) contextFaereyjar = await session({ browser: browser, storageState: 'service-portal-faereyjar.json', homeUrl, phoneNumber: '0102399', idsLoginOn: true, }) contexts.add(contextAmerika).add(contextFaereyjar) } catch (error) { console.error('Failed to initialize test contexts:', error) throw error } }) test.afterAll(async () => { const closePromises = Array.from(contexts).map(async (context) => { try { await context.close() } catch (error) { console.error('Failed to close context:', error) } }) await Promise.all(closePromises) })
43-75: 🛠️ Refactor suggestion
Refactor common setup code and improve URL assertions.
The first two test cases share common setup code that could be extracted to reduce duplication.
+ async function setupTestPage(context: BrowserContext) { + const page = await context.newPage() + await disableI18n(page) + await page.goto(icelandicAndNoPopupUrl('/minarsidur/skirteini')) + await page.waitForLoadState('networkidle', { timeout: 30000 }) + return page + } test('License overview', async () => { - const page = await contextFaereyjar.newPage() - await disableI18n(page) - await page.goto(icelandicAndNoPopupUrl('/minarsidur/skirteini')) + const page = await setupTestPage(contextFaereyjar)Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/access-control-consents.spec.ts (2)
34-54: 🛠️ Refactor suggestion
Consider extracting API endpoints and response handling.
The API endpoint check could be improved by:
- Extracting the GraphQL operation name to a constant
- Adding timeout handling for the API response wait
+const API_OPERATIONS = { + GET_CONSENT_LIST: 'GetConsentList', + PATCH_CONSENT: 'PatchConsent', +} as const; +const waitForApiResponse = async (page, operation: keyof typeof API_OPERATIONS, timeout = 5000) => { + return page.waitForResponse( + (resp) => + resp.url().includes(`/api/graphql?op=${operation}`) && + resp.status() === 200, + { timeout } + ); +}; await test.step('See consents overview and content', async () => { - await page.waitForResponse( - (resp) => - resp.url().includes('/api/graphql?op=GetConsentList') && - resp.status() === 200, - ) + await waitForApiResponse(page, 'GET_CONSENT_LIST');Committable suggestion skipped: line range outside the PR's diff.
56-97:
⚠️ Potential issueFix potential race condition in toggle switch assertion.
The toggle switch state check could be unreliable due to attribute retrieval timing. Also, consider extracting test IDs to constants.
+const TEST_IDS = { + CONSENT_CARD: 'consent-accordion-card', + CONSENT_SCOPE: 'consent-scope', +} as const; await test.step('Interact with consents card', async () => { - const ariaPressedStr = 'aria-pressed' + const ARIA_PRESSED = 'aria-pressed' - const consentScope = await page.getByTestId('consent-scope').count() + const consentScope = await page.getByTestId(TEST_IDS.CONSENT_SCOPE).count() await expect(consentScope).toBeGreaterThan(0) - await page.getByTestId(actionCardDataTestId).first().click() + await page.getByTestId(TEST_IDS.CONSENT_CARD).first().click() const firstToggleSwitch = await page - .getByTestId('consent-scope') + .getByTestId(TEST_IDS.CONSENT_SCOPE) .first() .locator('button') - const orginalAriaPressedValue = await firstToggleSwitch.getAttribute(ariaPressedStr) + const orginalAriaPressedValue = await firstToggleSwitch.getAttribute(ARIA_PRESSED) await firstToggleSwitch.first().click() - await expect( - Boolean(firstToggleSwitch.getAttribute(ariaPressedStr)), - ).not.toBe(orginalAriaPressedValue) + const newAriaPressedValue = await firstToggleSwitch.getAttribute(ARIA_PRESSED) + await expect(Boolean(newAriaPressedValue)).not.toBe(Boolean(orginalAriaPressedValue)) await firstToggleSwitch.first().click() - await page.waitForResponse( - (resp) => - resp.url().includes('/api/graphql?op=PatchConsent') && - resp.status() === 200, - ) + await waitForApiResponse(page, 'PATCH_CONSENT');Committable suggestion skipped: line range outside the PR's diff.
apps/application-system/form/e2e/acceptance/driving-instructor-registrations.spec.ts (5)
5-25: 🛠️ Refactor suggestion
Add type safety for session configuration.
Consider adding an interface for the session configuration to improve type safety and documentation.
interface SessionConfig { browser: Browser; homeUrl: string; phoneNumber: string; idsLoginOn: boolean; }
37-37: 🛠️ Refactor suggestion
Extract test data to constants.
Hardcoded kennitala values should be moved to constants or test data fixtures for better maintainability.
const TEST_DATA = { VALID_STUDENT_ID: '010130-5069', INVALID_STUDENT_ID: '010130-7789' } as const;Also applies to: 76-76
27-81: 🛠️ Refactor suggestion
Consider creating custom test commands for common operations.
The test contains repetitive patterns that could be extracted into custom commands for better readability and maintenance.
Example custom commands:
// Custom commands to add to your test framework async function registerDrivingMinutes(page: Page, minutes: string | number) { await page.locator('label').filter({ hasText: String(minutes) }).click() await page.getByPlaceholder('Veldu dagsetningu').click() await page.getByPlaceholder('Veldu dagsetningu').press('Enter') await page.getByRole('button', { name: 'Vista' }).click() } async function modifyLastRegistration(page: Page, minutes: string | number) { await page .getByRole('row') .getByRole('button', { name: 'Breyta' }) .first() .click() const manualMinutes = page.getByText('Slá inn mínútur') await manualMinutes.click() await manualMinutes.fill(String(minutes)) }
52-52:
⚠️ Potential issueReplace arbitrary wait with proper assertion.
Using
waitForTimeoutcan make tests flaky. Instead, wait for a specific condition or state change.- await page.waitForTimeout(5000) + await expect(page.getByRole('row').getByRole('button', { name: 'Breyta' })).toBeVisible()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await expect(page.getByRole('row').getByRole('button', { name: 'Breyta' })).toBeVisible()
15-17: 💡 Codebase verification
Uncomment the disabler functions - they are essential test utilities
These commented-out disabler functions are part of the E2E testing infrastructure and are actively used throughout the codebase. The functions are properly implemented in
libs/testing/e2e/src/lib/support/disablers.tsand are used to mock GraphQL responses during testing. They should be uncommented as they serve important testing purposes:
disableObjectKey: Mocks specific object keys in GraphQL responsesdisablePreviousApplications: Mocks empty application list responsesdisableI18n: Mocks translation responses🔗 Analysis chain
Clean up or implement commented disabler functions.
There are three commented-out disabler functions. Either implement them if they're needed or remove them to maintain clean code.
Let's check if these disabler functions are used elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for disabler function usage patterns rg -t typescript "disableObjectKey|disablePreviousApplications|disableI18n"Length of output: 112
Script:
#!/bin/bash # Search for disabler function usage patterns without file type restriction rg "disableObjectKey|disablePreviousApplications|disableI18n" # Also search for potential test helper files that might contain these functions fd -e ts -e js "helper|utils|test-utils"Length of output: 48986
Script:
#!/bin/bash # Let's check the implementation of these disabler functions cat libs/testing/e2e/src/lib/support/disablers.tsLength of output: 5069
apps/application-system/form/e2e/acceptance/driving-licence.spec.ts (4)
88-91:
⚠️ Potential issueImprove the keyboard interaction loop.
The current implementation using a for loop with hardcoded iterations is fragile and could break if the form structure changes.
- for (const _ of Array(10)) { - await page.locator('body').press('ArrowRight') - await page.locator('body').press('Tab') - } + const healthDeclarationInputs = await page.$$('[data-testid^="health-declaration-"]') + for (const input of healthDeclarationInputs) { + await input.click() + await input.press('ArrowRight') + }Committable suggestion skipped: line range outside the PR's diff.
14-36: 🛠️ Refactor suggestion
Consider adding error handling for session setup.
The session setup could benefit from explicit error handling to make test failures more descriptive.
const applicationTest = base.extend<{ applicationPage: Page }>({ applicationPage: async ({ browser }, use) => { + try { const applicationContext = await session({ browser, homeUrl, phoneNumber: '0107789', idsLoginOn: true, }) const applicationPage = await applicationContext.newPage() await disableObjectKey(applicationPage, 'existingApplication') await disablePreviousApplications(applicationPage) await disableI18n(applicationPage) await applicationPage.goto(homeUrl) await expect(applicationPage).toBeApplication() await use(applicationPage) await applicationPage.close() await applicationContext.close() + } catch (error) { + console.error('Failed to setup application test:', error) + throw error + } }, })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const applicationTest = base.extend<{ applicationPage: Page }>({ applicationPage: async ({ browser }, use) => { try { const applicationContext = await session({ browser, homeUrl, phoneNumber: '0107789', idsLoginOn: true, }) const applicationPage = await applicationContext.newPage() await disableObjectKey(applicationPage, 'existingApplication') //TODO: allow multiple mocked keys //await disableObjectKey(applicationPage, 'currentLicence') await disablePreviousApplications(applicationPage) await disableI18n(applicationPage) await applicationPage.goto(homeUrl) await expect(applicationPage).toBeApplication() await use(applicationPage) await applicationPage.close() await applicationContext.close() } catch (error) { console.error('Failed to setup application test:', error) throw error } }, })
38-56: 🛠️ Refactor suggestion
Improve environment handling and fake data setup.
The current environment check and fake data setup could be more maintainable:
- Consider using a proper enum or constant for environment names
- Extract the fake data setup into a separate helper function
+const SUPPORTED_TEST_ENVIRONMENTS = ['local', 'dev'] as const +type TestEnvironment = typeof SUPPORTED_TEST_ENVIRONMENTS[number] + +const setupFakeData = async (page: Page) => { + await page.getByLabel('Já').check() + await page + .getByRole('region', { name: 'Núverandi ökuréttindi umsækjanda' }) + .getByLabel('Engin') + .check() + await page + .getByRole('region', { name: 'Gervimynd eða enga mynd?' }) + .locator('label') + .first() + .click() + await page.getByTestId('proceed').click() +} + applicationTest.describe('Driving Instructor Registrations', () => { applicationTest.beforeEach(async ({ applicationPage }) => { const page = applicationPage - if (env == 'local' || env == 'dev') { - await page.getByLabel('Já').check() - await page - .getByRole('region', { name: 'Núverandi ökuréttindi umsækjanda' }) - .getByLabel('Engin') - .check() - await page - .getByRole('region', { name: 'Gervimynd eða enga mynd?' }) - .locator('label') - .first() - .click() - await page.getByTestId('proceed').click() + if (SUPPORTED_TEST_ENVIRONMENTS.includes(env as TestEnvironment)) { + await setupFakeData(page) } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const SUPPORTED_TEST_ENVIRONMENTS = ['local', 'dev'] as const type TestEnvironment = typeof SUPPORTED_TEST_ENVIRONMENTS[number] const setupFakeData = async (page: Page) => { await page.getByLabel('Já').check() await page .getByRole('region', { name: 'Núverandi ökuréttindi umsækjanda' }) .getByLabel('Engin') .check() await page .getByRole('region', { name: 'Gervimynd eða enga mynd?' }) .locator('label') .first() .click() await page.getByTestId('proceed').click() } applicationTest.describe('Driving Instructor Registrations', () => { applicationTest.beforeEach(async ({ applicationPage }) => { const page = applicationPage if (SUPPORTED_TEST_ENVIRONMENTS.includes(env as TestEnvironment)) { await setupFakeData(page) } })
57-98: 💡 Codebase verification
🛠️ Refactor suggestion
Refactoring recommendation validated with existing utilities
The review suggestion is valid and can be enhanced by leveraging existing utilities:
- The
locator-helpers.tsalready provides reusable helpers likefindByTestIdandproceedpageHelpers.tsoffers input locator utilities- Both test files are identical and would benefit from shared test components
Key findings:
- Duplicate test implementation exists in two locations
- Existing helper utilities are underutilized
- No page object pattern implementation exists yet, despite having a foundation of helper utilities
- The health declaration loop pattern appears in both files, indicating a need for abstraction
The original refactoring suggestion should be implemented with these additional points:
- Utilize existing
helpersfromlocator-helpers.tsfor common actions- Create a shared page object that both test files can use
- Extract test data to fixtures
- Abstract the health declaration loop into a reusable helper
🔗 Analysis chain
Improve test maintainability and data management.
The test case could benefit from several improvements:
- Break down the test into smaller, reusable steps
- Extract test data to a separate fixture
- Handle the TODO for payment testing
Consider restructuring the test using page objects and test data fixtures:
interface ApplicationTestData { email: string phone: string // Add other test data fields } const testData: ApplicationTestData = { email: 'email@domain.com', phone: '7654321', // Add other test data } class DrivingLicenceApplicationPage { constructor(private page: Page) {} async agreeToDataProviders() { await this.page.getByTestId('agree-to-data-providers').click() await this.page.getByTestId('proceed').click() } // Add other step methods } applicationTest('should be able to apply', async ({ applicationPage }) => { const page = new DrivingLicenceApplicationPage(applicationPage) await page.agreeToDataProviders() await page.selectLicenceType() // Continue with other steps })
Let me check the content of both files to better understand the test patterns.
Refactoring needed across duplicate test implementations
The verification reveals identical test implementations in two locations with the same maintainability issues:
apps/system-e2e/src/tests/islandis/application-system/acceptance/driving-licence.spec.tsapps/application-system/form/e2e/acceptance/driving-licence.spec.tsBoth files contain:
- Duplicate test steps with hardcoded test data
- The same
forloop pattern for health declaration- Identical TODO comments about payment handling
- Similar setup and configuration code
Let's check if there are any shared test utilities that could be leveraged.
Let me check if there's a shared testing package or utilities since both files import from '@island.is/testing/e2e'.
Let me check the content of the e2e utility files to see what helpers are available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for similar test patterns that could benefit from the same improvements rg -l "for \(const _ of Array\(" apps/Length of output: 197
Script:
#!/bin/bash # Check the content of both files echo "=== Content of first file ===" cat apps/system-e2e/src/tests/islandis/application-system/acceptance/driving-licence.spec.ts echo -e "\n=== Content of second file ===" cat apps/application-system/form/e2e/acceptance/driving-licence.spec.tsLength of output: 6608
Script:
#!/bin/bash # Look for existing test utilities or page objects rg -g '*.ts' -l "PageObject|page.object|test.utils|test.helpers" apps/Length of output: 166
Script:
#!/bin/bash # Look for testing package and its exports fd -e ts -e js "index|testing" apps/testing # Check for any existing page models or test utilities in the e2e directories fd -e ts "model|util|helper|page" apps/ -t f | grep -i "e2e\|test"Length of output: 861
Script:
#!/bin/bash # Check content of e2e utility files echo "=== Content of locator-helpers.ts ===" cat apps/system-e2e/src/support/locator-helpers.ts echo -e "\n=== Content of utils.ts ===" cat apps/system-e2e/src/support/utils.ts echo -e "\n=== Content of pageHelpers.ts ===" cat apps/system-e2e/src/utils/pageHelpers.tsLength of output: 1968
apps/application-system/form/e2e/acceptance/announcement-of-death.spec.ts (6)
35-97: 🛠️ Refactor suggestion
Split the test into smaller, focused test cases.
The current test is too long and violates the Single Responsibility Principle. Consider breaking it down into smaller, focused test cases for better maintainability and easier debugging.
Suggested structure:
applicationTest.describe('Announcement of Death', () => { applicationTest('should handle data provider consent', async ({ applicationPage }) => { // Test data provider consent flow }) applicationTest('should handle applicant information', async ({ applicationPage }) => { // Test applicant information flow }) applicationTest('should handle heir management', async ({ applicationPage }) => { // Test heir management flow }) // Additional focused test cases... })
36-96:
⚠️ Potential issueAdd assertions between form steps.
The test lacks assertions between steps to verify the form state.
Add assertions after each significant action:
// Example assertions to add await expect(page.getByText('Data providers accepted')).toBeVisible() await expect(page.getByRole('heading', { name: 'Relations' })).toBeVisible() await expect(page.getByText('Heir added successfully')).toBeVisible()
11-11: 🛠️ Refactor suggestion
Consider using environment-based URL configuration.
The hardcoded URL might cause issues when running tests across different environments (staging, production, etc.).
Consider using environment variables or test configuration:
-const homeUrl = '/umsoknir/andlatstilkynningar' +const homeUrl = process.env.BASE_URL ?? '/umsoknir/andlatstilkynningar'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const homeUrl = process.env.BASE_URL ?? '/umsoknir/andlatstilkynningar'
26-27:
⚠️ Potential issueAdd error handling for page navigation.
The page navigation should include error handling and timeout configuration.
- await applicationPage.goto(homeUrl) - await expect(applicationPage).toBeApplication() + try { + await applicationPage.goto(homeUrl, { timeout: 30000 }) + await expect(applicationPage).toBeApplication() + } catch (error) { + throw new Error(`Failed to navigate to application page: ${error.message}`) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { await applicationPage.goto(homeUrl, { timeout: 30000 }) await expect(applicationPage).toBeApplication() } catch (error) { throw new Error(`Failed to navigate to application page: ${error.message}`) }
53-55:
⚠️ Potential issueImprove element selectors as noted in TODO comment.
The current text-based selectors are brittle and may break with UI changes or translations.
- await page.locator('text=Veldu tengsl').click() - await page.locator('div:text("Systir")').click() + await page.getByRole('combobox', { name: 'relations' }).click() + await page.getByRole('option', { name: 'sister' }).click()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// TODO improve selectability in this screen await page.getByRole('combobox', { name: 'relations' }).click() await page.getByRole('option', { name: 'sister' }).click()
18-19: 🛠️ Refactor suggestion
Extract test constants for better maintainability.
The hardcoded phone number should be moved to a constants file or test configuration.
+const TEST_PHONE_NUMBER = '0102399' + const applicationContext = await session({ browser, homeUrl, - phoneNumber: '0102399', + phoneNumber: TEST_PHONE_NUMBER, idsLoginOn: true, })Committable suggestion skipped: line range outside the PR's diff.
apps/consultation-portal/project.json (1)
108-112: 🛠️ Refactor suggestion
Consider enhancing the e2e target configuration.
To improve the integration with your build pipeline, consider adding:
- Environment-specific configurations
- Dependencies on build targets
Here's how you could enhance the configuration:
"e2e": { "executor": "@nx/playwright:playwright", "options": { "config": "{projectRoot}/playwright.config.ts" - } + }, + "configurations": { + "development": { + "config": "{projectRoot}/playwright.config.dev.ts" + }, + "production": { + "config": "{projectRoot}/playwright.config.prod.ts" + } + }, + "dependsOn": [ + "build" + ] }Committable suggestion skipped: line range outside the PR's diff.
apps/portals/admin/e2e/smoke/endorsements.spec.ts (2)
35-47: 🛠️ Refactor suggestion
Improve test maintainability and robustness.
Several improvements could enhance the test suite:
- The hardcoded date '13.05.2023' should be dynamically generated to prevent test failures over time.
- Add error handling and timeout configurations for network operations.
- Consider extracting common navigation steps into a helper function.
- Enhance comments to better describe test intentions and expectations.
Example implementation for improvements:
// Helper function for common navigation async function navigateToEndorsementList(page: Page) { await page.goto(icelandicAndNoPopupUrl(homeUrl)) await page.click(delegationTarget) await page.click('[title="Undirskriftalistar"]') await page.getByRole('tab', { name: 'Liðnir listar' }).click() } // Dynamic date generation const getPastDate = (daysAgo: number) => { const date = new Date() date.setDate(date.getDate() - daysAgo) return date.toLocaleDateString('is-IS') } test('Access and edit a list', async ({ page }) => { test.slow() await navigateToEndorsementList(page) const exampleDateInThePast = getPastDate(30) // 30 days ago // ... rest of the test })Also applies to: 49-61, 63-92
11-11: 🛠️ Refactor suggestion
Consider using a more robust selector for delegationTarget.
Using a text selector
'text="65° ARTIC ehf."'might be fragile if the text changes. Consider using a more stable selector like a data-testid or aria-label.-const delegationTarget = 'text="65° ARTIC ehf."' +const delegationTarget = '[data-testid="delegation-target-arctic"]'Committable suggestion skipped: line range outside the PR's diff.
apps/application-system/form/e2e/acceptance/criminal-record.spec.ts (6)
14-31: 🛠️ Refactor suggestion
Consider improving test data management.
The test configuration uses a hardcoded phone number which could make test data management difficult. Consider using test data factories or configuration for better maintainability.
Consider using a test data helper:
// testData.ts export const TEST_USERS = { default: { phoneNumber: '0103019', // other user properties }, // other test users } // In your test const applicationContext = await session({ browser, homeUrl, phoneNumber: TEST_USERS.default.phoneNumber, idsLoginOn: true, })
63-73: 🛠️ Refactor suggestion
Improve card details management and date handling.
Several improvements needed in the card details step:
- Test card details should not be hardcoded in the test
- Date manipulation for card expiry could be more robust
Consider this improved implementation:
const TEST_CARDS = { valid: { number: '2223000010246699', cvv: '123', holder: 'Valitortestfyrirtgr', } } const getCardExpiryDate = () => { const date = new Date() const month = String(date.getMonth() + 1).padStart(2, '0') const year = String(date.getFullYear() + 1).slice(-2) // Use next year to avoid expiry return `${month}${year}` } // In test await page.getByPlaceholder('Nafn korthafa').fill(TEST_CARDS.valid.holder) await page.getByPlaceholder('Kortanúmer').fill(TEST_CARDS.valid.number) await page.getByPlaceholder('Öryggiskóði').fill(TEST_CARDS.valid.cvv) await page.getByPlaceholder('MM/ÁÁ').fill(getCardExpiryDate())
78-78:
⚠️ Potential issueFix missing await in assertions.
Some assertions are missing await which could lead to flaky tests.
Apply these fixes:
-await page.getByText(textGreiðslaTókst).isVisible() +await expect(page.getByText(textGreiðslaTókst)).toBeVisible() -await page.getByTestId('application-card').first().isVisible() -expect(await page.getByText(textAfgreidd).first().isVisible()).toBe( - true, -) +await expect(page.getByTestId('application-card').first()).toBeVisible() +await expect(page.getByText(textAfgreidd).first()).toBeVisible()Also applies to: 91-94
39-46: 🛠️ Refactor suggestion
Consider using a constants file for text content.
Text content and button labels are defined inline. Consider moving them to a separate constants file for better maintainability and reuse.
Create a separate constants file:
// test-constants.ts export const PAYMENT_TEST_CONTENT = { buttons: { confirm: 'Staðfesta', addCard: 'Bæta við korti', pay: 'Greiða', submit3D: 'Submit 3D data', }, text: { testUser: 'Gervimaður Afríka', paymentSuccess: 'Greiðsla tókst', // ... other text content } }
12-12: 🛠️ Refactor suggestion
Consider using environment-based URL configuration.
The hardcoded URL might cause issues when running tests across different environments (development, staging, production).
Consider using environment variables or configuration:
-const homeUrl = '/umsoknir/sakavottord' +const homeUrl = process.env.BASE_URL ?? '/umsoknir/sakavottord'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const homeUrl = process.env.BASE_URL ?? '/umsoknir/sakavottord'
60-60: 🛠️ Refactor suggestion
Consider environment-based payment URL.
The payment URL is hardcoded which might cause issues across different environments.
Consider using environment configuration:
-await page.waitForURL('https://uat.arkid.is/quickpay/card') +await page.waitForURL(process.env.PAYMENT_URL ?? 'https://uat.arkid.is/quickpay/card')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await page.waitForURL(process.env.PAYMENT_URL ?? 'https://uat.arkid.is/quickpay/card')apps/consultation-portal/e2e/smoke/consultation-portal-unauthenticated.spec.ts (5)
28-111: 🛠️ Refactor suggestion
Add error handling for proper cleanup
Consider wrapping the test bodies in try-catch blocks to ensure proper page cleanup even if tests fail.
Example pattern to apply to all tests:
test('test name', async () => { const page = await context.newPage() try { // test logic here } finally { await page.close() } })
48-53:
⚠️ Potential issueAdd await for click operation
The click operation should be awaited to prevent potential race conditions.
- page - .getByRole('button', { - name: 'Skrá mig inn', - }) - .click() + await page + .getByRole('button', { + name: 'Skrá mig inn', + }) + .click()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await page .getByRole('button', { name: 'Skrá mig inn', }) .click() await page.waitForURL(authLink)
10-10:
⚠️ Potential issueFix typo in test description
There's a typo in 'unathenticated' - it should be 'unauthenticated'.
-test.describe('Consultation portal unathenticated', () => { +test.describe('Consultation portal unauthenticated', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.describe('Consultation portal unauthenticated', () => {
97-98:
⚠️ Potential issueAdd await for click operation and assertion
The click operation should be awaited, and an assertion should be added to verify the login button state before navigation.
- page.getByTestId('menu-login-btn').click() + const loginButton = page.getByTestId('menu-login-btn') + await expect(loginButton).toBeEnabled() + await loginButton.click()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const loginButton = page.getByTestId('menu-login-btn') await expect(loginButton).toBeEnabled() await loginButton.click() await page.waitForURL(authLink)
65-70:
⚠️ Potential issueAdd await for click operation and assertion after navigation
The click operation should be awaited, and an assertion should be added after navigation to verify the page load.
- page - .getByRole('link', { - name: 'Hægt er að afskrá sig hér', - }) - .click() + await page + .getByRole('link', { + name: 'Hægt er að afskrá sig hér', + }) + .click() await page.waitForURL(`**/minaraskriftir`) + await expect(page).toHaveURL(/.*\/minaraskriftir/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await page .getByRole('link', { name: 'Hægt er að afskrá sig hér', }) .click() await page.waitForURL(`**/minaraskriftir`) await expect(page).toHaveURL(/.*\/minaraskriftir/)apps/application-system/form/e2e/acceptance/car-recycling.spec.ts (1)
81-83: 🛠️ Refactor suggestion
Simplify selector for better maintainability.
The role-based selector for the mileage input could be simplified by using a dedicated test ID instead.
-const mileage = page.getByRole('textbox', { - name: label(carRecyclingMessages.cars.mileage), -}) +const mileage = page.getByTestId('mileage-input')This change requires adding the test ID to the corresponding input component:
<input data-testid="mileage-input" // ... other props />apps/application-system/form/e2e/acceptance/social-insurance-administration/pension-supplement.spec.ts (3)
25-44: 🛠️ Refactor suggestion
Extract test user data to a shared configuration.
The test user phone number is hardcoded. Consider moving test user data to a shared configuration file to improve maintainability and make it easier to update test user information across all tests.
// testUsers.ts export const TEST_USERS = { GERVIMADUR_AFRIKA: { phoneNumber: '0103019', name: 'Gervimaður Afríka' } } as const // In your test file: const testUser = TEST_USERS.GERVIMADUR_AFRIKA
23-23: 🛠️ Refactor suggestion
Consider making the homeUrl configurable.
Instead of hardcoding the URL, consider making it configurable through environment variables or test configuration. This would make the tests more flexible across different environments.
-const homeUrl = '/umsoknir/uppbot-a-lifeyri' +const homeUrl = process.env.PENSION_SUPPLEMENT_URL || '/umsoknir/uppbot-a-lifeyri'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const homeUrl = process.env.PENSION_SUPPLEMENT_URL || '/umsoknir/uppbot-a-lifeyri'
46-126: 💡 Codebase verification
Consider adding error case tests, following established patterns
Looking at the driving-school-confirmation.spec.ts test file, we can see a good example of error case testing that includes:
- Negative test case with descriptive name "Student without a Driving Licence Book should only get error"
- Validation of error messages using
expect(errorMessage).toBeVisible()- Verification that valid UI elements are not shown in error state using
expect(datePicker).not.toBeVisible()Consider adding similar error cases to the Pension Supplement tests:
- Invalid national ID input
- Missing required fields
- Unauthorized access scenarios
- Form validation errors
This would align with the error testing patterns already established in the codebase.
🔗 Analysis chain
Consider adding error case tests.
The current test only covers the happy path. Consider adding tests for:
- Invalid input validation
- Server error handling
- Session timeout scenarios
- Form submission failures
This would ensure the application handles error cases gracefully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any error case tests in the codebase rg -l "error|failure|invalid|timeout" apps/application-system/form/e2e/acceptance/social-insurance-administration/Length of output: 116
Script:
#!/bin/bash # Let's broaden the search to check for error test cases across all e2e tests rg -l "test.*error|test.*failure|test.*invalid|test.*timeout|test.*negative" apps/application-system/form/e2e/Length of output: 192
Script:
#!/bin/bash # Let's also check for specific error handling patterns in the test files rg "expect.*toHaveText.*error|expect.*error.*message|expect.*validation" apps/application-system/form/e2e/ -A 2Length of output: 113
Script:
#!/bin/bash # Check for any existing error test files fd "error" apps/application-system/form/e2e/Length of output: 44
Script:
#!/bin/bash # Let's also check the test helper functions for error handling rg "error|invalid|failure" apps/application-system/form/e2e/helpers/Length of output: 152
Script:
#!/bin/bash # Let's check the driving school confirmation test that has error cases cat apps/application-system/form/e2e/acceptance/driving-school-confirmation.spec.tsLength of output: 4241
Script:
#!/bin/bash # Let's also check for test patterns with negative assertions rg "not\.toBeVisible|not\.toBeEnabled|toBeDisabled|toBeFalsy" apps/application-system/form/e2e/acceptance/social-insurance-administration/ -A 2Length of output: 145
Script:
#!/bin/bash # Check for validation message patterns rg "validation.*message|error.*message|invalid.*input" apps/application-system/form/e2e/acceptance/social-insurance-administration/Length of output: 133
apps/application-system/form/e2e/acceptance/social-insurance-administration/shared.ts (1)
32-73: 🛠️ Refactor suggestion
Consider simplifying the payment info selectors and extracting constants.
The payment info section could benefit from some improvements:
- The deeply nested selectors for tax-related fields could be extracted into helper functions
- The magic number '100' for personal allowance should be defined as a constant
Consider refactoring like this:
+const DEFAULT_PERSONAL_ALLOWANCE = '100' + +const getTaxRegionSelector = (message: MessageDescriptor) => + page.getByRole('region', { name: label(message) }) + export const fillPaymentInfo = async (page: Page, includeTax: boolean) => { // ... existing code ... if (includeTax) { - await page - .getByRole('region', { - name: label( - socialInsuranceAdministrationMessage.payment.personalAllowance, - ), - }) + await getTaxRegionSelector( + socialInsuranceAdministrationMessage.payment.personalAllowance + ) .getByRole('radio', { name: label(socialInsuranceAdministrationMessage.shared.yes), }) .click() const personalAllowance = page.getByTestId('personal-allowance-usage') await personalAllowance.selectText() - await personalAllowance.fill('100') + await personalAllowance.fill(DEFAULT_PERSONAL_ALLOWANCE)Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/acceptance/health.spec.ts (3)
63-64:
⚠️ Potential issueImprove selector reliability
Using
first()andlast()withgetByRole('row')can be brittle if the table data changes. Consider using more specific selectors or data-testid attributes.- const row = page.getByRole('row').last() + const row = page.getByTestId('dentist-registration-row')Also applies to: 120-123
48-86: 🛠️ Refactor suggestion
Refactor duplicate test patterns
The registration tests for dentists and health centers share similar patterns. Consider extracting common functionality into a helper function to improve maintainability.
async function performRegistration(page: Page, url: string, successHeading: string) { await page.goto(icelandicAndNoPopupUrl(url)) const changeRegistrationLink = page.getByRole('link', { name: label(messages.changeRegistration), }) await expect(changeRegistrationLink).toBeVisible() await changeRegistrationLink.click() // ... rest of the common registration logic const successMessage = page.getByRole('heading', { name: successHeading, }) await expect(successMessage).toBeVisible() } // Usage in tests: test('dentist registration', async () => { const page = await context.newPage() await disableI18n(page) await performRegistration( page, '/minarsidur/heilsa/tannlaeknar', 'Nýr tannlæknir skráður' ) })Also applies to: 103-144
73-79:
⚠️ Potential issueAdd error handling for modal interactions
The modal interaction lacks error handling. Consider adding timeout and error handling for modal appearances.
+ // Wait for modal with timeout + await expect(agreeButton).toBeVisible({ timeout: 5000 }) + + try { await agreeButton.click() + } catch (error) { + throw new Error('Failed to confirm registration: Modal interaction failed') + }Also applies to: 132-138
apps/portals/admin/e2e/smoke/access-control.spec.ts (2)
113-129: 🛠️ Refactor suggestion
Consider adding retry logic for flaky conditions.
The verification step might be susceptible to timing issues in CI environments.
Add retry logic for the URL assertion:
await test.step('Verify delegation and automatic redirect', async () => { + const REDIRECT_TIMEOUT = 10000; + const RETRY_OPTIONS = { intervals: [1000, 2000, 5000] }; // ... existing setup code ... - await expect(receiverPage.url()).toBe(`${homeUrl}loftbru`) + await expect + .poll( + async () => receiverPage.url(), + { + timeout: REDIRECT_TIMEOUT, + intervals: RETRY_OPTIONS.intervals, + message: 'Waiting for redirect to Loftbru page' + } + ) + .toBe(`${homeUrl}loftbru`); })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await test.step('Verify delegation and automatic redirect', async () => { const REDIRECT_TIMEOUT = 10000; const RETRY_OPTIONS = { intervals: [1000, 2000, 5000] }; // Arrange const contextReceiver = await session({ browser, homeUrl, phoneNumber: '0103019', }) const receiverPage = await contextReceiver.newPage() await receiverPage.goto(homeUrl) // Act await expect( receiverPage.getByRole('heading', { name: 'Yfirlit' }), ).toBeVisible() await expect .poll( async () => receiverPage.url(), { timeout: REDIRECT_TIMEOUT, intervals: RETRY_OPTIONS.intervals, message: 'Waiting for redirect to Loftbru page' } ) .toBe(`${homeUrl}loftbru`); }) })
72-90: 🛠️ Refactor suggestion
Enhance delegation removal robustness.
The delegation removal loop could benefit from additional error handling and constants.
Consider these improvements:
+ const DELETION_TIMEOUT = 5000; + const AFRICA_USER_NAME = 'Gervimaður Afríka'; await test.step('Remove delegation', async () => { const delegations = granterPage .locator('data-testid=access-card') - .filter({ hasText: 'Gervimaður Afríka' }) + .filter({ hasText: AFRICA_USER_NAME }) const deleteButton = delegations.first().locator('role=button[name=Eyða]') const confirmButton = granterPage.locator( '[data-dialog-ref="access-delete-modal"] >> role=button[name*=Eyða]', ) const count = await delegations.count() for (let i = 0; i < count; ++i) { + try { await deleteButton.click() await confirmButton.click() - await confirmButton.waitFor({ state: 'hidden' }) + await confirmButton.waitFor({ state: 'hidden', timeout: DELETION_TIMEOUT }) + } catch (error) { + console.error(`Failed to delete delegation ${i + 1}/${count}:`, error) + throw error + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const DELETION_TIMEOUT = 5000; const AFRICA_USER_NAME = 'Gervimaður Afríka'; await test.step('Remove delegation', async () => { const delegations = granterPage .locator('data-testid=access-card') .filter({ hasText: AFRICA_USER_NAME }) const deleteButton = delegations.first().locator('role=button[name=Eyða]') const confirmButton = granterPage.locator( '[data-dialog-ref="access-delete-modal"] >> role=button[name*=Eyða]', ) const count = await delegations.count() for (let i = 0; i < count; ++i) { try { await deleteButton.click() await confirmButton.click() await confirmButton.waitFor({ state: 'hidden', timeout: DELETION_TIMEOUT }) } catch (error) { console.error(`Failed to delete delegation ${i + 1}/${count}:`, error) throw error } } // Assert await expect(delegations).toHaveCount(0) })apps/application-system/form/e2e/acceptance/driving-school-confirmation.spec.ts (4)
51-52: 🛠️ Refactor suggestion
Consider extracting test data to a separate configuration file
The hardcoded national ID '010130-5069' should be moved to a test data configuration file for better maintainability.
// test-data.config.ts export const TEST_DATA = { validStudent: { nationalId: '010130-5069', expectedName: 'Gervimaður Bandaríkin' } };
41-42: 🛠️ Refactor suggestion
Improve date handling with test utilities
The date manipulation could be made more readable and maintainable using a dedicated test utility.
// date-utils.ts export const getTestDate = (monthsAgo: number) => { const date = new Date(); date.setMonth(date.getMonth() - monthsAgo); return format(date, 'dd.MM.yyyy'); }; // In test await dateField.fill(getTestDate(1));Also applies to: 62-62
112-112:
⚠️ Potential issueFix data inconsistency in name comparison
The comment about trailing space indicates a data quality issue that should be addressed at the source.
-await expect(nameField).toHaveValue('Gervimaður Ameríku ') // Trailing space 😭 +await expect(nameField).toHaveValue('Gervimaður Ameríku'.trim())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await expect(nameField).toHaveValue('Gervimaður Ameríku'.trim())
108-111: 🛠️ Refactor suggestion
Simplify keyboard interactions
The current implementation uses multiple keyboard interactions that could be simplified.
-await nationalIdField.click() -await nationalIdField.press('Control+a') -await nationalIdField.fill('010130-2989') -await nationalIdField.press('Tab') +await nationalIdField.fill('010130-2989', { force: true })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await nationalIdField.fill('010130-2989', { force: true })apps/application-system/form/e2e/acceptance/operating-licence.spec.ts (6)
13-13: 🛠️ Refactor suggestion
Consider moving the URL to a constants file.
For better maintainability and reusability, consider moving
homeUrlto a shared constants file that can be used across different test suites.// constants/urls.ts export const ROUTES = { OPERATING_LICENSE: '/umsoknir/rekstrarleyfi' } as const
71-73: 🛠️ Refactor suggestion
Extract test data to a separate fixture file.
Hardcoded test values should be moved to a dedicated test fixtures file for better maintainability.
// fixtures/operating-licence.ts export const testData = { businessName: 'Cool Place', vatNumber: '123456', // ... other test data } as const
38-129: 🛠️ Refactor suggestion
Modularize test steps into reusable functions.
The test case contains many steps that could be extracted into reusable helper functions for better maintainability and reusability.
// helpers/operating-licence.ts export async function fillAccommodationInfo(page: Page, data: AccommodationData) { await page.getByLabel('Heiti veitinga-/ gististaðar').fill(data.businessName) await page.getByLabel('Virðisaukaskattsnúmer').fill(data.vatNumber) await page.getByTestId('proceed').click() } // In test file: await fillAccommodationInfo(page, testData)
91-103: 🛠️ Refactor suggestion
Improve file handling in document upload section.
The current implementation of creating and deleting mock PDF files could be improved using a more robust approach.
import { withTempFile } from '@island.is/testing/e2e' await withTempFile('mockPdf.pdf', async (filePath) => { for (const uploadButton of await page .getByRole('button', { name: 'Velja skjöl til að hlaða upp', exact: true }) .all()) { await uploadButton.uploadFile(filePath) } })
104-104:
⚠️ Potential issueReplace arbitrary sleep with proper wait conditions.
Using
sleep(1000)is fragile and could lead to flaky tests. Instead, wait for specific conditions.- await sleep(1000) + await page.waitForSelector('[data-testid="upload-complete"]', { state: 'attached' })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await page.waitForSelector('[data-testid="upload-complete"]', { state: 'attached' })
15-36: 🛠️ Refactor suggestion
Consider adding error handling for session setup.
The session setup could benefit from explicit error handling to provide better debugging information when the setup fails.
applicationPage: async ({ browser }, use) => { + try { const applicationContext = await session({ browser, homeUrl, phoneNumber: '0103019', idsLoginOn: true, }) const applicationPage = await applicationContext.newPage() await disablePreviousApplications(applicationPage) await disableI18n(applicationPage) await applicationPage.goto(homeUrl) await expect(applicationPage).toBeApplication() await use(applicationPage) await applicationPage.close() await applicationContext.close() + } catch (error) { + console.error('Failed to setup test session:', error) + throw error + } },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const applicationTest = base.extend<{ applicationPage: Page }>({ applicationPage: async ({ browser }, use) => { try { const applicationContext = await session({ browser, homeUrl, phoneNumber: '0103019', idsLoginOn: true, }) const applicationPage = await applicationContext.newPage() // await disableObjectKey(applicationPage, 'existingApplication') await disablePreviousApplications(applicationPage) await disableI18n(applicationPage) // await disableDelegations(applicationPage) await applicationPage.goto(homeUrl) await expect(applicationPage).toBeApplication() await use(applicationPage) await applicationPage.close() await applicationContext.close() } catch (error) { console.error('Failed to setup test session:', error) throw error } }, })apps/portals/my-pages/e2e/acceptance/licenses.spec.ts (2)
27-82: 🛠️ Refactor suggestion
Enhance test maintainability and reliability.
Consider these improvements:
- Move the license list to a separate test data file
- Adjust the timeout value to be more reasonable
- Add retry logic for flaky element checks
// testData/licenses.ts export const licenseTypes = { drivingLicense: 'Ökuréttindi', firearmLicense: 'Skotvopnaleyfi', // ... other licenses } // In your test file: import { licenseTypes } from './testData/licenses'- await element.waitFor({ timeout: 30000 }) + await element.waitFor({ timeout: 5000 })
82-109: 🛠️ Refactor suggestion
Enhance test coverage and reduce duplication.
The current implementation could benefit from:
- Adding specific assertions for each license type's unique elements
- Implementing a test helper to reduce code duplication
- Adding error screenshots on failure
async function validateLicenseScreen(page: Page, license: { title: string, ref: string, specificElements?: string[] }) { const element = page.locator(`[href*="${license.ref}"]`) await element.click() const title = page.getByRole('heading', { name: license.title }) await expect(title).toBeVisible() if (license.specificElements) { for (const selector of license.specificElements) { await expect(page.locator(selector)).toBeVisible() } } await page.goBack() }apps/portals/my-pages/e2e/smoke/financials.spec.ts (2)
18-26: 🛠️ Refactor suggestion
Consider extracting test configuration.
The test configuration contains hardcoded values that should be moved to a configuration file:
- Storage state filename
- Phone number
- IDS login flag
Consider creating a test config file:
// test-config.ts export const config = { storageState: process.env.STORAGE_STATE ?? 'service-portal-bretland.json', phoneNumber: process.env.TEST_PHONE ?? '0104929', idsLoginOn: process.env.IDS_LOGIN === 'true', }
32-47: 🛠️ Refactor suggestion
Improve test maintainability and reduce duplication.
The test suites have several areas for improvement:
- Common setup code is duplicated across all tests
- Test data contains hardcoded values
- Similar patterns in filtering could be abstracted
Consider these improvements:
- Extract common setup into a fixture:
const test = base.extend({ financePage: async ({ context }, use) => { const page = await context.newPage() await disableI18n(page) await use(page) } })
- Move test data to a separate file:
// test-data.ts export const testData = { searchTerms: { valid: 'Sakavottorð', invalid: 'Vegabréf' }, dates: { start: '15.01.2023', end: '27.01.2023' } }
- Create a helper for filtering operations:
async function filterByDate(page: Page, date: string) { const filterButton = page .locator(`role=button[name="${label(m.openFilter)}"]`) .first() await filterButton.click() const inputField = page.getByPlaceholder(label(m.datepickLabel)).first() await inputField.fill(date) }Also applies to: 49-75, 77-112, 114-146
apps/application-system/form/e2e/acceptance/marriage-conditions.spec.ts (2)
39-124: 🛠️ Refactor suggestion
Improve test reliability and maintainability.
Several improvements could enhance the test:
- Replace
sleep()with proper wait conditions- Extract test data to constants or fixtures
- Consider splitting into smaller test cases for better maintainability
Example refactor for the first issue:
- await sleep(500) + await expect(page.getByTestId('proceed')).toBeEnabled() await page.getByTestId('proceed').click()Example for test data extraction:
const TEST_DATA = { spouse: { nationalId: '010130-3019', phone: '777-3019', email: 'test@test.test' }, witness1: { nationalId: '010130-2399', phone: '777-5069', email: 'test@test.test' }, witness2: { nationalId: '010130-2399', phone: '777-2399', email: 'test@test.test' } };
39-124: 🛠️ Refactor suggestion
Enhance test maintainability and accessibility.
Consider these improvements:
- Extract duplicate selectors into constants
- Use more semantic selectors (prefer role and label over testId where possible)
Example refactor:
const SELECTORS = { proceed: '[data-testid="proceed"]', witness: { nationalId: 'label:has-text("Kennitala")', phone: 'label:has-text("Símanúmer")', email: 'label:has-text("Netfang")' } }; // Usage: await page.locator(SELECTORS.proceed).click();apps/portals/my-pages/e2e/smoke/vehicles.spec.ts (4)
32-34: 🛠️ Refactor suggestion
Add error handling to context cleanup
Consider adding try-catch block to ensure proper cleanup even if an error occurs.
test.afterAll(async () => { + try { await context.close() + } catch (error) { + console.error('Failed to close browser context:', error) + } })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test.afterAll(async () => { try { await context.close() } catch (error) { console.error('Failed to close browser context:', error) } })
136-169: 🛠️ Refactor suggestion
Improve test data handling and accessibility testing
Consider the following improvements:
- Use test data constants instead of hardcoded strings
- Add proper date picker interaction
- Include accessibility checks
+ const TEST_DATA = { + STATUS: { + REGISTERED: 'Forskráð', + OK: 'Í lagi', + }, + } as const - await expect(table).toContainText('Forskráð') + await expect(table).toContainText(TEST_DATA.STATUS.REGISTERED) + // Add accessibility check + await test.step('should be accessible', async () => { + const violations = await page.evaluate(async () => { + // Run axe-core accessibility tests + }) + expect(violations).toEqual([]) + })Committable suggestion skipped: line range outside the PR's diff.
41-72: 🛠️ Refactor suggestion
Enhance test maintainability and reliability
Consider the following improvements:
- Extract URL paths to constants
- Add data-testid attributes for more reliable element selection
- Add timeout handling for network-dependent operations
+ const ROUTES = { + MY_VEHICLES: '/minarsidur/eignir/okutaeki/min-okutaeki', + } as const await test.step('should display data and filter overview', async () => { // Arrange await page.goto( - icelandicAndNoPopupUrl('/minarsidur/eignir/okutaeki/min-okutaeki'), + icelandicAndNoPopupUrl(ROUTES.MY_VEHICLES), ) + await page.waitForLoadState('networkidle', { timeout: 30000 })Committable suggestion skipped: line range outside the PR's diff.
102-134:
⚠️ Potential issueImprove test robustness and coverage
The test has several areas for improvement:
- Replace arbitrary typing delay with proper wait conditions
- Add test case for successful vehicle lookup
- Add error handling for network failures
- await inputField.type('ísland.is', { delay: 200 }) + await inputField.type('ísland.is') + await page.waitForResponse( + response => + response.url().includes('/api/vehicles/search') && + response.status() === 200 + ) + await test.step('should handle network errors', async () => { + await page.route('**/api/vehicles/search', route => + route.abort('failed') + ) + // Test error handling + })Committable suggestion skipped: line range outside the PR's diff.
apps/portals/admin/e2e/smoke/tenant-permission.spec.ts (3)
35-50: 🛠️ Refactor suggestion
Enhance type safety for the helper function.
Consider adding TypeScript types and making the helper function more reusable.
-const buttonSaveTest = async (title: string, disabled = true) => { +type ButtonTestParams = { + title: string + disabled?: boolean + page: Page +} +const buttonSaveTest = async ({ title, disabled = true, page }: ButtonTestParams) => { const dataTestId = `button-save-${title}` if (disabled) { await expect(page.getByTestId(dataTestId)).toBeDisabled() } else { await expect(page.getByTestId(dataTestId)).not.toBeDisabled() } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test('can manage permission', async () => { // Arrange const page = await contextGranter.newPage() // Act await page.goto(homeUrl) type ButtonTestParams = { title: string disabled?: boolean page: Page } const buttonSaveTest = async ({ title, disabled = true, page }: ButtonTestParams) => { const dataTestId = `button-save-${title}` if (disabled) { await expect(page.getByTestId(dataTestId)).toBeDisabled() } else { await expect(page.getByTestId(dataTestId)).not.toBeDisabled() } }
122-157: 🛠️ Refactor suggestion
Add type safety for checkbox configuration.
Consider adding TypeScript types for the checkbox configuration to improve maintainability and type safety.
+type CheckboxName = + | 'isAccessControlled' + | 'grantToAuthenticatedUser' + | 'grantToProcuringHolders' + | 'grantToLegalGuardians' + | 'allowExplicitDelegationGrant' + | 'grantToPersonalRepresentatives' -const checkboxes = [ +const checkboxNames: CheckboxName[] = [ 'isAccessControlled', 'grantToAuthenticatedUser', // ... other checkboxes ].map((name) => page.locator(getInputByName(name)))Committable suggestion skipped: line range outside the PR's diff.
79-121: 🛠️ Refactor suggestion
Improve maintainability by extracting constants.
Consider extracting repeated strings and selectors into constants for better maintainability.
+const LANGUAGE_SELECTORS = { + IS_DISPLAY_NAME: 'is_displayName', + EN_DISPLAY_NAME: 'en_displayName', + IS_DESCRIPTION: 'is_description', + EN_DESCRIPTION: 'en_description', +} as const + +const LANGUAGES = { + ENGLISH: 'English', + ICELANDIC: 'Icelandic', +} as const await test.step('See content card and interact with it', async () => { const title = 'Content' - const isDisplayNameInput = getInputByName('is_displayName') + const isDisplayNameInput = getInputByName(LANGUAGE_SELECTORS.IS_DISPLAY_NAME) // ... update other selectors similarlyCommittable suggestion skipped: line range outside the PR's diff.
apps/application-system/form/e2e/acceptance/financial-statements-inao.spec.ts (3)
164-173:
⚠️ Potential issueReplace arbitrary timeout with proper wait condition.
Using
waitForTimeoutis unreliable and can lead to flaky tests. Instead, wait for a specific condition that indicates the upload is complete.- await page.waitForTimeout(1000) + await expect(page.locator('[data-testid="upload-success-indicator"]')).toBeVisible();Also, consider moving the mock PDF creation/deletion to the test setup/teardown to ensure cleanup even if the test fails.
Committable suggestion skipped: line range outside the PR's diff.
49-54:
⚠️ Potential issueImprove dropdown selection reliability.
The current implementation relies on hardcoded keyboard navigation which might break if the dropdown options change. Consider selecting by value or text instead.
- const election = findByTestId('select-election.selectElection') - await election.click() - await page.keyboard.press('ArrowDown') - await page.keyboard.press('ArrowDown') - await page.keyboard.press('Enter') + const election = page.locator('select[data-testid="select-election.selectElection"]'); + await election.selectOption({ label: 'Expected Option Text' });Committable suggestion skipped: line range outside the PR's diff.
14-185: 💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Test needs to be enabled and is the sole coverage for financial statements
This is currently the only E2E test for the financial statements functionality, and it's marked as skipped. The test provides comprehensive coverage of the form submission flow, including:
- Data provider agreement
- Election selection
- Financial data entry
- File upload
- Form submission
- The test should be enabled by removing the
test.skipto ensure continuous validation of this critical flow- Consider adding edge cases and error scenarios as separate test cases in this file
🔗 Analysis chain
Verify test coverage with actual user scenarios.
The test provides good coverage of the financial statements submission flow. However, since it's marked as skipped, verify that it covers all critical user scenarios when enabling it.
Let me gather more information about test coverage and skipped tests.
Let me check for any related test files or implementation files to understand the full context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other financial statement related tests to ensure complete coverage rg -l "Financial Statements" --type ts apps/application-system/form/e2e/Length of output: 152
Script:
#!/bin/bash # Check for other skipped tests and their context rg "test.skip" -B2 -A2 apps/application-system/form/e2e/ # Look for any other financial statement related files rg -l "skilarsreikninga" apps/application-system/form/Length of output: 1338
Script:
#!/bin/bash # Search for related implementation files rg -l "Financial|skilarsreikninga" apps/application-system/form/src/ # Check for any test helper files that might be used rg -l "helpers" apps/application-system/form/e2e/Length of output: 766
apps/application-system/form/e2e/acceptance/social-insurance-administration/old-age-pension.spec.ts (4)
23-184: 🛠️ Refactor suggestion
Consider adding error handling for failed steps.
The test function could benefit from try-catch blocks around critical steps to provide better error messages and cleanup in case of failures.
Example implementation:
const executeTestStep = async ( stepName: string, action: () => Promise<void> ) => { try { await action() } catch (error) { throw new Error(`Failed at step "${stepName}": ${error.message}`) } } // Usage example: await executeTestStep('Fill in applicant info', () => fillApplicantInfo(page) )
116-121: 🛠️ Refactor suggestion
Enhance email validation in employer registration.
The current email validation is basic. Consider adding proper email format validation to catch invalid email addresses early in the test.
- await employerEmail.fill('mockEmail@mail.is') + const validTestEmail = 'test.user@example.is' + await employerEmail.fill(validTestEmail) + // Verify email format + await expect(employerEmail).toHaveValue(validTestEmail)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const employerEmail = page.getByRole('textbox', { name: label(oldAgePensionFormMessage.employer.email), }) await employerEmail.selectText() const validTestEmail = 'test.user@example.is' await employerEmail.fill(validTestEmail) // Verify email format await expect(employerEmail).toHaveValue(validTestEmail)
188-207: 🛠️ Refactor suggestion
Consider making test user configurable.
The test user ID ('0103019') is hardcoded. Consider moving this to a configuration file or environment variable for better maintainability and flexibility across different test environments.
+// Add to a config file or .env +const TEST_USER_CONFIG = { + phoneNumber: '0103019', // Gervimaður Afríka + description: 'Test user for pension applications' +} const applicationTest = base.extend<{ applicationPage: Page }>({ applicationPage: async ({ browser }, use) => { const applicationContext = await session({ browser, homeUrl, - phoneNumber: '0103019', // Gervimaður Afríka + phoneNumber: process.env.TEST_USER_PHONE || TEST_USER_CONFIG.phoneNumber, idsLoginOn: true, })Committable suggestion skipped: line range outside the PR's diff.
128-133: 🛠️ Refactor suggestion
Add validation for employment ratio input.
The employment ratio input should be validated to ensure it's within acceptable bounds (e.g., 0-100%).
await employmentRatio.selectText() - await employmentRatio.fill('50') + const ratio = '50' + await employmentRatio.fill(ratio) + // Verify ratio is within valid range + await expect(Number(ratio)).toBeGreaterThanOrEqual(0) + await expect(Number(ratio)).toBeLessThanOrEqual(100)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const employmentRatio = page.getByRole('textbox', { name: label(oldAgePensionFormMessage.employer.ratio), }) await employmentRatio.selectText() const ratio = '50' await employmentRatio.fill(ratio) // Verify ratio is within valid range await expect(Number(ratio)).toBeGreaterThanOrEqual(0) await expect(Number(ratio)).toBeLessThanOrEqual(100) await proceed()apps/application-system/form/e2e/acceptance/data-protection-complaint.spec.ts (5)
124-239: 🛠️ Refactor suggestion
Refactor the skipped test case for better maintainability.
The skipped test case is too long and handles multiple responsibilities. Consider breaking it down:
Split into smaller, focused test cases:
- Test form navigation
- Test file upload separately
- Test form submission
Extract selectors to constants:
+const SELECTORS = { + AGREE_TO_DATA_PROVIDERS: 'agree-to-data-providers', + PROCEED: 'proceed', + STEP_BACK: 'step-back', +} as const;
- Create a helper for repetitive form filling:
async function fillNoOptions(page: Page, questions: string[]) { for (const question of questions) { await expect(page.getByText(question)).toBeVisible() await page.getByLabel(noOptionLabel).check() await page.getByTestId(SELECTORS.PROCEED).click() } }
44-85: 🛠️ Refactor suggestion
Extract test data to constants or test fixtures.
The test contains hard-coded test data that should be extracted for better maintainability:
+const TEST_DATA = { + PHONE_NUMBER: '0103019', + TEST_USER: 'Gervimaður Afríka', +} as const; applicationTest( 'Should be able to start the application and see it added on the application overview', async ({ applicationPage }) => { // ... - const gervimadurAfricaText = 'Gervimaður Afríka' + const gervimadurAfricaText = TEST_DATA.TEST_USER // ... } )Committable suggestion skipped: line range outside the PR's diff.
208-216:
⚠️ Potential issueImprove file upload test reliability.
The file upload test uses a non-existent file and lacks proper error handling:
-await chooser.setFiles('someFileThatDoesNotExist.txt') +const testFilePath = path.join(__dirname, '../fixtures/test-upload.pdf') +await chooser.setFiles(testFilePath)Add a test fixture file at
e2e/fixtures/test-upload.pdffor consistent testing.Committable suggestion skipped: line range outside the PR's diff.
39-42: 🛠️ Refactor suggestion
Add error handling and documentation to the helper function.
While the function is well-structured, consider these improvements for better maintainability and reliability:
+/** + * Counts the number of visible application cards on the page + * @param page - Playwright Page object + * @returns Promise<number> - Number of visible application cards + * @throws Error if the page fails to load or elements are not found + */ const countApplicationsVisible = async (page: Page) => { await page.waitForLoadState('domcontentloaded') + try { return page.getByTestId('application-card').count() + } catch (error) { + throw new Error(`Failed to count applications: ${error.message}`) + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Counts the number of visible application cards on the page * @param page - Playwright Page object * @returns Promise<number> - Number of visible application cards * @throws Error if the page fails to load or elements are not found */ const countApplicationsVisible = async (page: Page) => { await page.waitForLoadState('domcontentloaded') try { return page.getByTestId('application-card').count() } catch (error) { throw new Error(`Failed to count applications: ${error.message}`) } }
86-122:
⚠️ Potential issueAdd retry logic for flaky deletion test.
The deletion test might be flaky due to timing issues with the GraphQL response. Consider adding retry logic:
await page.getByRole('button', { name: deleteConfirmationText }).click() -await page.waitForResponse('**/api/graphql?op=ApplicationApplications') +await Promise.all([ + page.waitForResponse('**/api/graphql?op=ApplicationApplications'), + page.waitForFunction(() => + document.querySelectorAll('[data-testid="application-card"]').length < visibleApplicationsAfterCreation + ) +])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.applicationTest( 'Should be able to delete an application and that its not visible on overview', async ({ applicationPage }) => { const iconTrashTestId = 'icon-trash' const deleteConfirmationText = label( coreMessages.deleteApplicationDialogConfirmLabel, ) const agreeToDataProvidersTestId = 'agree-to-data-providers' await applicationTest.step( 'Delete an application and check the number of applications after deletion', async () => { const page = applicationPage const applicationAtStart = await createApplication(page) await page.goto(`${homeUrl}`, { waitUntil: 'networkidle' }) const visibleApplicationsAfterCreation = await countApplicationsVisible(page) await page.getByTestId(iconTrashTestId).first().click() await page .getByRole('button', { name: deleteConfirmationText }) .click() await Promise.all([ page.waitForResponse('**/api/graphql?op=ApplicationApplications'), page.waitForFunction(() => document.querySelectorAll('[data-testid="application-card"]').length < visibleApplicationsAfterCreation ) ]) if (visibleApplicationsAfterCreation - 1 > 0) { const numberOfApplicationsAfterDeletion = await countApplicationsVisible(page) expect(numberOfApplicationsAfterDeletion).toBe(applicationAtStart) } else { await page.getByTestId(agreeToDataProvidersTestId) await expect(applicationPage).toBeApplication() } }, ) }, )apps/portals/my-pages/e2e/mocks/service_portal_mocks.json (2)
1131-1173: 🛠️ Refactor suggestion
Security headers configuration could be enhanced
While basic CORS and content-type headers are present, additional security headers could be added to better simulate production behavior.
Consider adding these security headers:
"headers": [ + { + "key": "X-Content-Type-Options", + "value": "nosniff" + }, + { + "key": "X-Frame-Options", + "value": "DENY" + }, + { + "key": "X-XSS-Protection", + "value": "1; mode=block" + },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."proxyMode": false, "proxyHost": "", "proxyRemovePrefix": false, "tlsOptions": { "enabled": false, "type": "CERT", "pfxPath": "", "certPath": "", "keyPath": "", "caPath": "", "passphrase": "" }, "cors": true, "headers": [ { "key": "Content-Type", "value": "application/json" }, { "key": "Access-Control-Allow-Origin", "value": "*" }, { "key": "Access-Control-Allow-Methods", "value": "GET,POST,PUT,PATCH,DELETE,HEAD,OPTIONS" }, { "key": "Access-Control-Allow-Headers", "value": "Content-Type, Origin, Accept, Authorization, Content-Length, X-Requested-With" }, { "key": "X-Content-Type-Options", "value": "nosniff" }, { "key": "X-Frame-Options", "value": "DENY" }, { "key": "X-XSS-Protection", "value": "1; mode=block" } ], "proxyReqHeaders": [ { "key": "", "value": "" } ], "proxyResHeaders": [ { "key": "", "value": "" } ],
175-1103:
⚠️ Potential issueMock responses contain sensitive information in plain text
The mock responses contain what appears to be test data with personal information (names, IDs, addresses). While this is mock data, it's best practice to avoid using potentially real-looking personal data.
Consider:
- Using obviously fake data (e.g., "MOCK_NAME_1" instead of "Bubbi bílakall")
- Adding a clear "TEST DATA" marker in responses
- Documenting the test data pattern in a comment
Example:
"body": { + "_test_data_notice": "This is mock data for testing purposes only", "name": "MOCK_USER_1", "socialSecurityNumber": "TEST_SSN_001",Committable suggestion skipped: line range outside the PR's diff.
apps/portals/my-pages/e2e/smoke/access-control.spec.ts (3)
42-48: 🛠️ Refactor suggestion
Consider translating code comments to English for broader accessibility
The code comments from lines 42 to 48 are currently in Icelandic. Translating them to English can enhance maintainability and make the codebase more accessible to all team members.
105-109:
⚠️ Potential issueAvoid hardcoding sensitive personal identification numbers in test code
Hardcoding sensitive data like personal identification numbers (
kennitala) can pose security risks and might lead to compliance issues. Instead, consider using mock data, environment variables, or a secured test data management system to handle such information safely.
66-79:
⚠️ Potential issueFix the delegation deletion logic to correctly delete all matching delegations
The current implementation may not delete all delegations as intended because
deleteButtonanddelegationsare not updated within the loop after each deletion. This could lead to only the first delegation being deleted repeatedly without affecting subsequent ones. Updating thedelegationsanddeleteButtoninside the loop ensures that all matching delegations are deleted.Apply this diff to fix the deletion logic:
-const delegations = granterPage - .locator('data-testid=access-card') - .filter({ hasText: 'Gervimaður Ameríku' }) -const deleteButton = delegations.first().locator('role=button[name=Eyða]') -const confirmButton = granterPage.locator( - '[data-dialog-ref="access-delete-modal"] >> role=button[name*=Eyða]', -) -const count = await delegations.count() -for (let i = 0; i < count; ++i) { - await deleteButton.click() - await confirmButton.click() - await confirmButton.waitFor({ state: 'hidden' }) -} +let delegations = granterPage + .locator('data-testid=access-card') + .filter({ hasText: 'Gervimaður Ameríku' }) +while (await delegations.count() > 0) { + const deleteButton = delegations.first().locator('role=button[name=Eyða]') + await deleteButton.click() + const confirmButton = granterPage.locator( + '[data-dialog-ref="access-delete-modal"] >> role=button[name*=Eyða]', + ) + await confirmButton.click() + await confirmButton.waitFor({ state: 'hidden' }) + delegations = granterPage + .locator('data-testid=access-card') + .filter({ hasText: 'Gervimaður Ameríku' }) +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let delegations = granterPage .locator('data-testid=access-card') .filter({ hasText: 'Gervimaður Ameríku' }) while (await delegations.count() > 0) { const deleteButton = delegations.first().locator('role=button[name=Eyða]') await deleteButton.click() const confirmButton = granterPage.locator( '[data-dialog-ref="access-delete-modal"] >> role=button[name*=Eyða]', ) await confirmButton.click() await confirmButton.waitFor({ state: 'hidden' }) delegations = granterPage .locator('data-testid=access-card') .filter({ hasText: 'Gervimaður Ameríku' }) }apps/portals/admin/e2e/smoke/tenant-application.spec.ts (5)
28-28:
⚠️ Potential issueAvoid hardcoding potentially sensitive data like phone numbers
The phone number
'0102399'may represent personally identifiable information (PII). Consider using a placeholder or loading test data from a secure configuration to prevent potential exposure of sensitive information.[security]
150-174:
⚠️ Potential issueUse realistic URLs for testing
redirectUrisandpostLogoutRedirectUrisFilling the
redirectUrisandpostLogoutRedirectUrisfields with'This is a dummy text'may not effectively simulate real-world scenarios and could lead to insufficient test coverage. Use properly formatted URLs like'https://example.com/callback'to ensure the application correctly handles valid URIs.
293-295:
⚠️ Potential issueRemove unnecessary
awaitbeforeexpectin synchronous assertionsThe
expectfunction is synchronous when asserting values like numbers. Theawaitkeyword is unnecessary and can be omitted for clarity.Apply this diff to remove the unnecessary
await:- await expect(await page.getByTestId('permission-row').count()).toBe( + expect(await page.getByTestId('permission-row').count()).toBe( initialPermissionCount, )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.expect(await page.getByTestId('permission-row').count()).toBe( initialPermissionCount, )
266-270: 🛠️ Refactor suggestion
Enhance selector specificity to improve test robustness
Using generic selectors like
'#add-permissions tbody tr td'may make the test brittle if the DOM structure changes. Utilize more specific selectors ordata-testidattributes to target elements reliably.Apply this diff to improve the selector:
- await page - .locator('#add-permissions tbody tr td') + await page + .getByTestId('permission-checkbox')Ensure to add the
data-testid="permission-checkbox"attribute to the checkbox elements in the application code if not already present.Committable suggestion skipped: line range outside the PR's diff.
318-319:
⚠️ Potential issueAwait
expectassertions on locators to ensure proper asynchronous handlingWhen asserting conditions on locators, the
expectcalls should be awaited to handle the asynchronous nature of these operations correctly.Apply this diff to await the
expectcalls:await Promise.all( - checkboxes.map((checkbox) => expect(checkbox).not.toBeVisible()), + checkboxes.map(async (checkbox) => await expect(checkbox).not.toBeVisible()), )Alternatively, you can iterate over the checkboxes:
+ for (const checkbox of checkboxes) { + await expect(checkbox).not.toBeVisible() + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await Promise.all( checkboxes.map(async (checkbox) => await expect(checkbox).not.toBeVisible()), ) ``` Solution 2 (using for...of): ```suggestion for (const checkbox of checkboxes) { await expect(checkbox).not.toBeVisible() }apps/application-system/form/e2e/acceptance/parental-leave.spec.ts (5)
1-742: 🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
The test code contains duplicated steps across the two test cases for the primary and other parent applications. Refactoring common steps into reusable functions can improve maintainability and readability.
Example:
async function fillPersonalInformation(page: Page, email: string) { const emailBox = page.getByRole('textbox', { name: label(parentalLeaveFormMessages.applicant.email), }) await emailBox.selectText() await emailBox.type(email) const phoneNumber = page.getByRole('textbox', { name: label(parentalLeaveFormMessages.applicant.phoneNumber), }) await phoneNumber.selectText() await phoneNumber.type('6555555') await page .getByRole('region', { name: label(parentalLeaveFormMessages.applicant.languageTitle), }) .getByRole('radio', { name: label(parentalLeaveFormMessages.applicant.icelandic), }) .click() await proceed() }Then, replace the code in both tests with a call to this function.
41-48:
⚠️ Potential issueImprove URL extraction from email using a more robust method
The regex used to extract the employer approval URL may fail if the email HTML structure changes or if the URL is not wrapped exactly as expected. Consider using a HTML parser to reliably extract the URL from the email content.
Suggested change:
const email = await employer.getLastEmail(6) // Require email if (!email || typeof email.html !== 'string') { throw new Error('Email not found, test incomplete') } - const employerUrlMatch = email.html.match(/>(http?:.*)<\/p>/) - if (employerUrlMatch?.length != 2) + const $ = cheerio.load(email.html) + const employerUrl = $('a[href^="http"]').attr('href') + if (!employerUrl) throw new Error( 'Email does not contain the url to approve the parental leave application', ) - const employerUrl = employerUrlMatch[1]Committable suggestion skipped: line range outside the PR's diff.
445-449:
⚠️ Potential issueUse 'test.skip' correctly within test functions
The
test.skip()function is used to conditionally skip tests, but it should be called outside of test functions or with appropriate conditions. In this case, consider skipping the test by wrapping the condition around the test declaration.Suggested change:
- test('Other parent should be able to create application', async () => { - // Skip this test if primary parent was unable to submit application - test.skip( - submitApplicationSuccess !== true, - 'Primary parent unable to submit application', - ) + test.skip( + submitApplicationSuccess !== true, + 'Primary parent unable to submit application', + ) + test('Other parent should be able to create application', async () => {Committable suggestion skipped: line range outside the PR's diff.
56-57:
⚠️ Potential issueAvoid hardcoding sensitive identifiers like kennitala
Including hardcoded kennitala (Icelandic national ID numbers) can be problematic for maintainability and security. Consider storing such values in configuration or mock data files, and reference them appropriately.
Suggested change:
await page .getByRole('textbox') // eslint-disable-next-line local-rules/disallow-kennitalas - .type('5402696029') + .type(EMPLOYER_NATIONAL_ID)And define
EMPLOYER_NATIONAL_IDat the top of your test file or in a configuration file.Committable suggestion skipped: line range outside the PR's diff.
327-337:
⚠️ Potential issueHandle file upload exceptions and ensure file cleanup
While uploading files, it's important to handle any exceptions that may occur during the file chooser interaction. Additionally, ensure that the temporary file is deleted even if an error occurs to prevent residual files.
Suggested change:
createMockPdf() const fileChooserPromise = page.waitForEvent('filechooser') await page .getByRole('button', { name: label(parentalLeaveFormMessages.fileUpload.attachmentButton), }) .click() const filechooser = await fileChooserPromise - await filechooser.setFiles('./mockPdf.pdf') + try { + await filechooser.setFiles('./mockPdf.pdf') + } finally { + deleteMockPdf() + } await page.waitForTimeout(1000) - deleteMockPdf() await proceed()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.createMockPdf() const fileChooserPromise = page.waitForEvent('filechooser') await page .getByRole('button', { name: label(parentalLeaveFormMessages.fileUpload.attachmentButton), }) .click() const filechooser = await fileChooserPromise try { await filechooser.setFiles('./mockPdf.pdf') } finally { deleteMockPdf() } await page.waitForTimeout(1000) await proceed()
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
apps/portals/my-pages/e2e/mocks/service_portal_mocks.json (1)
9-173: Improve folder structure documentationThe folder structure organizes routes by domain but lacks descriptions for each section. Adding descriptions would improve maintainability.
Add descriptions to each folder:
"folders": [ { "uuid": "c3275fb8-3e44-4a63-a1b4-a1bd5ef54fed", "name": "Health Insurance", + "description": "Endpoints for health insurance services including insurance status, dentists, and health centers", "children": [
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/application-system/form/e2e/mocks/application_portal_mocks.json(1 hunks)apps/portals/my-pages/e2e/mocks/service_portal_mocks.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/application-system/form/e2e/mocks/application_portal_mocks.json
🧰 Additional context used
📓 Path-based instructions (1)
apps/portals/my-pages/e2e/mocks/service_portal_mocks.json (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."
| "responses": [ | ||
| { | ||
| "uuid": "2014ef77-3a33-47c2-ab63-23978784d39d", | ||
| "body": "{\n \"isInsured\": true,\n \"explanation\": \"Testing\",\n \"from\": \"current date\",\n \"status\": {\n \"display\": \"test status\",\n \"code\": \"ALM\"\n },\n \"maximumPayment\": 80,\n \"refundDate\": \"2030-05-18T00:00:00.000Z\",\n \"ehicCardExpiryDate\": \"2030-05-18T00:00:00.000Z\"\n}", | ||
| "latency": 0, | ||
| "statusCode": 200, | ||
| "label": "", | ||
| "headers": [], | ||
| "bodyType": "INLINE", | ||
| "filePath": "", | ||
| "databucketID": "", | ||
| "sendFileAsBody": false, | ||
| "rules": [], | ||
| "rulesOperator": "OR", | ||
| "disableTemplating": false, | ||
| "fallbackTo404": false, | ||
| "default": true, | ||
| "crudKey": "id", | ||
| "callbacks": [] | ||
| } |
There was a problem hiding this comment.
Mock response data needs more realistic error scenarios
The mock responses only handle successful scenarios (200/201 status codes). For proper testing, error scenarios should also be included.
Add error responses for common scenarios:
"responses": [
{
"uuid": "2014ef77-3a33-47c2-ab63-23978784d39d",
"body": "{\n \"isInsured\": true,\n \"explanation\": \"Testing\",\n \"from\": \"current date\" }",
"statusCode": 200,
"default": true
},
+ {
+ "uuid": "error-unauthorized",
+ "body": "{\n \"error\": \"Unauthorized\",\n \"message\": \"Invalid or expired token\" }",
+ "statusCode": 401,
+ "rules": [
+ {
+ "target": "header",
+ "modifier": "Authorization",
+ "value": "",
+ "operator": "equals"
+ }
+ ]
+ },
+ {
+ "uuid": "error-not-found",
+ "body": "{\n \"error\": \"Not Found\",\n \"message\": \"Resource not found\" }",
+ "statusCode": 404,
+ "rules": [
+ {
+ "target": "query",
+ "modifier": "id",
+ "value": "invalid",
+ "operator": "equals"
+ }
+ ]
+ }
]Also applies to: 241-259, 357-375
| "uuid": "94e8eee0-b79e-4a71-a3e1-8323bd3e7c2c", | ||
| "body": "{\n \"kennitala\": \"987654321\",\n \"nafn\": \"Bubbi kall\",\n \"gildirfra\": \"2023-03-08T00:00:00Z\",\n \"gildirtil\": \"2025-10-", |
There was a problem hiding this comment.
Incomplete JSON response data
The response body for endpoint r1/IS-DEV/GOV/10008/TR-Protected/oryrki-v1/faskirteini is truncated and will cause JSON parsing errors.
Fix the truncated JSON:
- "body": "{\n \"kennitala\": \"987654321\",\n \"nafn\": \"Bubbi kall\",\n \"gildirfra\": \"2023-03-08T00:00:00Z\",\n \"gildirtil\": \"2025-10-",
+ "body": "{\n \"kennitala\": \"987654321\",\n \"nafn\": \"Bubbi kall\",\n \"gildirfra\": \"2023-03-08T00:00:00Z\",\n \"gildirtil\": \"2025-10-01T00:00:00Z\"\n}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "uuid": "94e8eee0-b79e-4a71-a3e1-8323bd3e7c2c", | |
| "body": "{\n \"kennitala\": \"987654321\",\n \"nafn\": \"Bubbi kall\",\n \"gildirfra\": \"2023-03-08T00:00:00Z\",\n \"gildirtil\": \"2025-10-", | |
| "uuid": "94e8eee0-b79e-4a71-a3e1-8323bd3e7c2c", | |
| "body": "{\n \"kennitala\": \"987654321\",\n \"nafn\": \"Bubbi kall\",\n \"gildirfra\": \"2023-03-08T00:00:00Z\",\n \"gildirtil\": \"2025-10-01T00:00:00Z\"\n}", |
| "headers": [ | ||
| { | ||
| "key": "Content-Type", | ||
| "value": "application/json" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Origin", | ||
| "value": "*" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Methods", | ||
| "value": "GET,POST,PUT,PATCH,DELETE,HEAD,OPTIONS" | ||
| }, | ||
| { | ||
| "key": "Access-Control-Allow-Headers", | ||
| "value": "Content-Type, Origin, Accept, Authorization, Content-Length, X-Requested-With" | ||
| } | ||
| ], |
There was a problem hiding this comment.
CORS and security headers configuration needs improvement
The current CORS configuration is too permissive for a production environment. While this might be acceptable for testing, it should be documented as a testing-only configuration.
Consider adding security headers:
"headers": [
{
"key": "Content-Type",
"value": "application/json"
},
{
"key": "Access-Control-Allow-Origin",
- "value": "*"
+ "value": "${ALLOWED_ORIGINS}"
},
+ {
+ "key": "X-Content-Type-Options",
+ "value": "nosniff"
+ },
+ {
+ "key": "X-Frame-Options",
+ "value": "DENY"
+ },
+ {
+ "key": "Content-Security-Policy",
+ "value": "default-src 'self'"
+ }
],Committable suggestion skipped: line range outside the PR's diff.
| "data": [], | ||
| "callbacks": [] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider utilizing data and callbacks features
The data and callbacks arrays are empty but could be used to improve the mock service functionality.
Consider adding:
- Data buckets for reusable mock data
- Callbacks for simulating async operations
- "data": [],
- "callbacks": []
+ "data": [
+ {
+ "id": "common-errors",
+ "data": {
+ "unauthorized": {
+ "error": "Unauthorized",
+ "message": "Invalid or expired token"
+ },
+ "notFound": {
+ "error": "Not Found",
+ "message": "Resource not found"
+ }
+ }
+ }
+ ],
+ "callbacks": [
+ {
+ "id": "async-operation",
+ "type": "http",
+ "method": "get",
+ "endpoint": "/status",
+ "responses": [
+ {
+ "status": "completed",
+ "delay": 2000
+ }
+ ]
+ }
+ ]Committable suggestion skipped: line range outside the PR's diff.
| { | ||
| "uuid": "3188fa7c-9286-42d5-8e02-cde7c041d8aa", | ||
| "lastMigration": 32, | ||
| "name": "Service Portal Mocks", | ||
| "endpointPrefix": "", | ||
| "latency": 0, | ||
| "port": 9388, | ||
| "hostname": "", |
There was a problem hiding this comment.
Base configuration needs security enhancements
The base configuration exposes potential security risks:
- No rate limiting configured
- CORS is set to allow all origins
- Port number (9388) should be configurable via environment variables
Consider adding:
{
"uuid": "3188fa7c-9286-42d5-8e02-cde7c041d8aa",
"lastMigration": 32,
"name": "Service Portal Mocks",
"endpointPrefix": "",
"latency": 0,
- "port": 9388,
+ "port": "${MOCK_SERVER_PORT}",
+ "rateLimit": {
+ "windowMs": 15 * 60 * 1000,
+ "max": 100
+ },
"hostname": "",Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (20)
libs/testing/e2e/src/lib/support/pageHelpers.ts (1)
1-5: Consider adding input validation for the name parameter.While the implementation is functional, it could benefit from input validation to prevent potential issues with invalid selector strings.
+const isValidSelectorName = (name: string): boolean => { + return typeof name === 'string' && name.length > 0 && /^[a-zA-Z0-9_-]+$/.test(name); +} + /** Returns a CSS selector for an input element with the specified name attribute. */ -export const getInputByName = (name: string) => `input[name="${name}"]` +export const getInputByName = (name: string) => { + if (!isValidSelectorName(name)) { + throw new Error('Invalid input name for selector'); + } + return `input[name="${name}"]`; +} /** Returns a CSS selector for a textarea element with the specified name attribute. */ -export const getTextareaByName = (name: string) => `textarea[name="${name}"]` +export const getTextareaByName = (name: string) => { + if (!isValidSelectorName(name)) { + throw new Error('Invalid textarea name for selector'); + } + return `textarea[name="${name}"]`; +}libs/testing/e2e/src/lib/support/i18n.ts (2)
16-16: Add JSDoc documentation for better type clarity.While the TypeScript type is correctly used, adding documentation would improve maintainability.
Consider adding documentation:
+/** + * Formats a message descriptor using the configured intl instance + * @param l - The message descriptor to format + * @returns The formatted message string + */ export const label = (l: MessageDescriptor) => intl.formatMessage(l)
8-11: Enhance error handling with structured logging.The current error handling suppresses missing translations but could be more informative for debugging purposes.
Consider this enhancement:
onError: (err) => { - if (err?.code == 'MISSING_TRANSLATION') return - console.warn(err) + if (err?.code === 'MISSING_TRANSLATION') { + // Optionally log missing translations in development + if (process.env.NODE_ENV === 'development') { + console.debug('[i18n] Missing translation:', err.message); + } + return; + } + console.warn('[i18n] Formatting error:', { + code: err?.code, + message: err?.message, + }); },libs/testing/e2e/src/lib/support/locator-helpers.ts (1)
1-3: Consider expanding theRolestype to include more ARIA rolesThe current
Rolestype is limited to only three roles ('heading', 'button', 'radio'). Consider expanding it to include other common ARIA roles that might be needed in testing scenarios (e.g., 'textbox', 'checkbox', 'link', 'menu', etc.).-type Roles = 'heading' | 'button' | 'radio' +type Roles = + | 'heading' + | 'button' + | 'radio' + | 'textbox' + | 'checkbox' + | 'link' + | 'menu' + | 'menuitem' + | 'tab' + | 'tabpanel' + | 'list' + | 'listitem'apps/application-system/form/src/environments/environment.ts (1)
5-5: Update development documentation with new portThe API endpoint port change should be reflected in the development setup documentation to prevent confusion for other developers.
#!/bin/bash # Description: Look for documentation files that might need updating echo "Checking documentation files for port references..." fd -e md -X grep -l "4444"libs/testing/e2e/src/lib/utils/playwright-config.ts (1)
40-41: Simplify port and url configurationThe ternary operations can be simplified as these values are mutually exclusive.
Apply this diff:
- port: port ? port : undefined, - url: port ? undefined : webServerUrl, + port, + url: port ? undefined : webServerUrl,libs/testing/e2e/src/lib/config/playwright-config.ts (3)
11-17: Consider improving configuration defaultsThe default
cwdvalue uses a relative path that could be fragile and hard to maintain. Consider:
- Making it more explicit by using a project-relative path
- Using environment variables or configuration files for such paths
- Adding documentation to explain the expected directory structure
Additionally, consider extracting the timeout constant to a named configuration value for better maintainability.
+const DEFAULT_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes + export const createPlaywrightConfig = ({ webServerUrl, port, command, - cwd = '../../../', + cwd = process.env.PROJECT_ROOT || '../../../', timeoutMs = 5 * 60 * 1000, }: PlaywrightConfigParams) =>
18-39: LGTM! Well-structured Playwright configurationThe configuration follows best practices with good separation of concerns between CI and local environments. Consider making the test patterns configurable through parameters for better flexibility.
+interface TestProject { + name: string; + pattern: string; +} + +const DEFAULT_PROJECTS: TestProject[] = [ + { name: 'everything', pattern: 'e2e/**/*.spec.[tj]s' }, + { name: 'smoke', pattern: 'e2e/smoke/**/*.spec.[tj]s' }, + { name: 'acceptance', pattern: 'e2e/acceptance/**/*.spec.[tj]s' }, +]; + export const createPlaywrightConfig = ({ webServerUrl, port, command, cwd = '../../../', timeoutMs = 5 * 60 * 1000, + projects = DEFAULT_PROJECTS, }: PlaywrightConfigParams) =>
40-48: Simplify webServer URL and port configurationThe current port/URL logic can be simplified for better readability.
webServer: { stdout: 'pipe', - port: port ? port : undefined, - url: port ? undefined : webServerUrl, + ...(port ? { port } : { url: webServerUrl }), command, reuseExistingServer: !process.env.CI, timeout: timeoutMs, cwd, },libs/testing/e2e/src/index.ts (1)
67-68: Consider scoping Playwright exportsRe-exporting Playwright types and utilities directly might cause naming conflicts. Consider namespacing these exports.
-export { test, expect, BrowserContext, Page, Locator } from '@playwright/test' +import * as pw from '@playwright/test'; +export const playwright = { + test: pw.test, + expect: pw.expect, + BrowserContext: pw.BrowserContext, + Page: pw.Page, + Locator: pw.Locator, +} as const;libs/testing/e2e/README.md (4)
1-9: Fix function name inconsistency and add setup instructions.The overview is well-structured, but there are a few improvements needed:
- The function name
createGlobalConfig(line 9) doesn't match the export name mentioned in the summary (createPlaywrightConfig).- Consider adding installation and initial setup instructions for new users.
Add the following sections after the Overview:
## Installation ```bash yarn add -D @island.is/testing/e2eInitial Setup
- Create a
playwright.config.tsfile in your app's root:import { createPlaywrightConfig } from '@island.is/testing/e2e' export default createPlaywrightConfig({ // your config options here })--- `15-19`: **Add version requirements and feature dependencies.** The Mockoon introduction is clear, but consider adding: 1. Required Mockoon version for compatibility 2. Specific Mockoon features that our E2E tests depend on Add this information after line 19: ```markdown ### Requirements - Mockoon Desktop v{version} or higher - Required features: - Response rules - Environment variables - Request logging (for debugging)
38-38: Fix typo in instructions.There's a typo in the word "Nema" which should be "Name".
- - Nema your mock file and choose a location for it e.g. `apps/<my-app>/e2e/mocks/<my-app-mock>.json`. + - Name your mock file and choose a location for it e.g. `apps/<my-app>/e2e/mocks/<my-app-mock>.json`.
45-47: Enhance CLI command example and add troubleshooting section.The CLI command example could be more specific, and a troubleshooting section would be helpful.
Add more detailed examples and a troubleshooting section:
```bash # Example for running the auth service mock yarn mockoon-cli start --data ./apps/auth-admin/e2e/mocks/auth-service.json --port 3333Troubleshooting
Port Already in Use
# Check if port is in use lsof -i :3333 # Kill the process if needed kill -9 <PID>Mock File Not Found
- Ensure the path is relative to the repository root
- Check file permissions
</blockquote></details> <details> <summary>apps/application-system/api/src/environments/environment.ts (2)</summary><blockquote> Line range hint `22-22`: **Security: Avoid hardcoding JWT secret in development config** The JWT secret is hardcoded as 'supersecret' in the development configuration. Even for development environments, it's recommended to use environment variables for sensitive values. Consider using an environment variable with a fallback: ```diff - jwtSecret: 'supersecret', + jwtSecret: process.env.DEV_JWT_SECRET ?? 'supersecret',
Line range hint
11-15: Security: Consider using environment variables for auth configuration in developmentThe authentication configuration contains hardcoded values for the development environment. For better security and consistency with production, consider using environment variables.
Consider updating the auth configuration:
auth: { - issuer: 'https://identity-server.dev01.devland.is', - audience: ['@island.is', '@admin.island.is'], - allowClientNationalId: true, + issuer: process.env.DEV_AUTH_ISSUER ?? 'https://identity-server.dev01.devland.is', + audience: process.env.DEV_AUTH_AUDIENCE?.split(',') ?? ['@island.is', '@admin.island.is'], + allowClientNationalId: process.env.DEV_ALLOW_CLIENT_NATIONAL_ID !== 'false', },apps/system-e2e/src/playwright.config.ts (1)
37-40: Document reporter configuration choicesConsider adding a comment explaining the reporter choices:
- 'line' reporter for CI provides detailed output for debugging CI failures
- 'dot' reporter for local development offers concise feedback
- HTML reporter generates detailed reports for test analysis
/* Reporter to use. See https://playwright.dev/docs/test-reporters */ reporter: [ + // Use line reporter in CI for detailed output, dot reporter locally for concise feedback ...((process.env.CI ? [['line']] : [['dot']]) as ReporterDescription[]), + // Generate HTML report for detailed test analysis, but don't open automatically ['html', { open: 'never' }], ],libs/testing/e2e/src/lib/support/disablers.ts (3)
57-59: PassexactMatchOption in Recursive CallsIn the
deepMockfunction, theexactMatchoption is not passed to recursive calls. To ensure consistent matching behavior throughout recursion, include theexactMatchparameter in the recursive call.Apply this diff to pass
exactMatch:mocked[key] = deepMock(original[key], mockKey, mockData, { deepPath: updatedDeepPath, + exactMatch, })
28-29: Define Type for Options Parameter indeepMockThe options parameter in
deepMockis currently an untyped object. To improve type safety and code clarity, define a type or interface for the options and update the function signature accordingly.Apply this diff:
+ interface DeepMockOptions { + exactMatch?: boolean + deepPath?: string + } const deepMock = <T = Dict>( original: T | T[], mockKey: Matchable, mockData: unknown = {}, - { exactMatch = false, deepPath = 'data' } = {}, + options: DeepMockOptions = {}, ): T | T[] | Dict | Dict[] => { + const { exactMatch = false, deepPath = 'data' } = options
20-20: Export TypeScript Types for ReusabilityTo enhance reusability across different parts of the application, consider exporting the TypeScript types
Matchable,MockGQLOptions, andDict. This aligns with best practices and promotes type safety throughout the codebase.Apply this diff to export the types:
- type Matchable = string | RegExp + export type Matchable = string | RegExp - type MockGQLOptions = { + export type MockGQLOptions = { // ... } - type Dict<T = unknown> = Record<string, T> + export type Dict<T = unknown> = Record<string, T>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
apps/application-system/api/src/environments/environment.ts(1 hunks)apps/application-system/form/playwright.config.ts(1 hunks)apps/application-system/form/src/environments/environment.ts(1 hunks)apps/consultation-portal/playwright.config.ts(1 hunks)apps/portals/admin/playwright.config.ts(1 hunks)apps/portals/my-pages/playwright.config.ts(1 hunks)apps/system-e2e/src/playwright.config.ts(1 hunks)apps/web/playwright.config.ts(1 hunks)infra/package.json(1 hunks)libs/testing/e2e/README.md(1 hunks)libs/testing/e2e/src/index.ts(1 hunks)libs/testing/e2e/src/lib/config/playwright-config.ts(1 hunks)libs/testing/e2e/src/lib/support/disablers.ts(1 hunks)libs/testing/e2e/src/lib/support/email-account.ts(1 hunks)libs/testing/e2e/src/lib/support/i18n.ts(1 hunks)libs/testing/e2e/src/lib/support/locator-helpers.ts(1 hunks)libs/testing/e2e/src/lib/support/pageHelpers.ts(1 hunks)libs/testing/e2e/src/lib/utils/playwright-config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/playwright.config.ts
- infra/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/application-system/form/playwright.config.ts
- apps/consultation-portal/playwright.config.ts
- apps/portals/admin/playwright.config.ts
- apps/portals/my-pages/playwright.config.ts
- libs/testing/e2e/src/lib/support/email-account.ts
🧰 Additional context used
📓 Path-based instructions (11)
apps/application-system/api/src/environments/environment.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/application-system/form/src/environments/environment.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/playwright.config.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."
libs/testing/e2e/README.md (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/disablers.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/i18n.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/locator-helpers.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/support/pageHelpers.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/testing/e2e/src/lib/utils/playwright-config.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (9)
libs/testing/e2e/src/lib/support/pageHelpers.ts (1)
1-5: Verify selector string sanitization in the e2e testing framework.
The implementation looks good and follows the coding guidelines for library code. However, let's verify how these selectors are used in the testing framework to ensure proper escaping of special characters.
libs/testing/e2e/src/lib/support/i18n.ts (1)
1-2: LGTM! Good use of TypeScript types.
The import statement correctly uses TypeScript types from @formatjs/intl, adhering to the coding guidelines for TypeScript usage.
apps/application-system/form/src/environments/environment.ts (1)
5-5: Verify port change consistency across the codebase
The development API endpoint port has been changed from 4444 to 9456. Let's ensure this change is consistent across all related configurations.
libs/testing/e2e/src/lib/utils/playwright-config.ts (2)
1-8: Well-structured TypeScript interface and imports!
Good practices observed:
- Specific named imports support effective tree-shaking
- Clear TypeScript interface definition with appropriate optional properties
32-37: Enhance cross-browser testing coverage
Currently, tests only run on Chromium. For better coverage, consider adding Firefox and WebKit.
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
1-9: LGTM! Well-structured TypeScript interface
The interface definition is clear, properly typed, and follows TypeScript best practices with appropriate optional parameters.
libs/testing/e2e/src/index.ts (3)
1-68: Well-organized exports with improved tree-shaking potential!
The restructuring from wildcard exports to specific named exports is a significant improvement that:
- Enhances code maintainability through logical grouping
- Improves tree-shaking capabilities
- Makes the public API more explicit
34-38: Verify TypeScript type exports for authentication
The CognitoCreds appears to be a type export. Let's verify its usage and ensure proper type definitions are maintained.
#!/bin/bash
# Description: Verify TypeScript type usage and implementations
# Check the type definition and its usage
ast-grep --pattern 'type CognitoCreds = {
$$$
}'
# Check implementations using this type
ast-grep --pattern 'function $_($$_: CognitoCreds) {
$$$
}'49-56: Verify reusability across NextJS apps
The environment utilities seem to be core functionality. Let's verify their usage across different NextJS applications to ensure they're properly shared.
✅ Verification successful
Environment utilities are properly reused across NextJS applications
The usage of environment utilities across different NextJS applications has been verified and they are properly shared.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of environment utilities across NextJS apps
# Find NextJS apps
fd -t d 'pages' -x echo "Found NextJS app: {//}"
# Check usage of environment utilities
rg -l "TestEnvironment|BaseAuthority|getEnvironmentBaseUrl" --type=typescript
Length of output: 894
Script:
#!/bin/bash
# Description: Check usage of environment utilities across NextJS apps
# Find NextJS apps
fd -t d 'pages' -x echo "Found NextJS app: {//}"
# Check usage of environment utilities with correct type
rg -l "TestEnvironment|BaseAuthority|getEnvironmentBaseUrl" --type=ts
Length of output: 1319
| const cache = createIntlCache() | ||
| const intl = createIntl( | ||
| { | ||
| locale: 'is', | ||
| onError: (err) => { | ||
| if (err?.code == 'MISSING_TRANSLATION') return | ||
| console.warn(err) | ||
| }, | ||
| }, | ||
| cache, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the locale configurable for better reusability.
The hardcoded 'is' locale might limit the reusability of this module across different NextJS applications that might need different locales.
Consider this implementation:
+interface IntlConfig {
+ locale?: string;
+ defaultLocale?: string;
+}
const cache = createIntlCache()
-const intl = createIntl(
+export const createCustomIntl = ({ locale = 'is', defaultLocale = 'is' }: IntlConfig = {}) => createIntl(
{
- locale: 'is',
+ locale,
+ defaultLocale,
onError: (err) => {
if (err?.code == 'MISSING_TRANSLATION') return
console.warn(err)
},
},
cache,
)
+const intl = createCustomIntl()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cache = createIntlCache() | |
| const intl = createIntl( | |
| { | |
| locale: 'is', | |
| onError: (err) => { | |
| if (err?.code == 'MISSING_TRANSLATION') return | |
| console.warn(err) | |
| }, | |
| }, | |
| cache, | |
| ) | |
| interface IntlConfig { | |
| locale?: string; | |
| defaultLocale?: string; | |
| } | |
| const cache = createIntlCache() | |
| export const createCustomIntl = ({ locale = 'is', defaultLocale = 'is' }: IntlConfig = {}) => createIntl( | |
| { | |
| locale, | |
| defaultLocale, | |
| onError: (err) => { | |
| if (err?.code == 'MISSING_TRANSLATION') return | |
| console.warn(err) | |
| }, | |
| }, | |
| cache, | |
| ) | |
| const intl = createCustomIntl() |
| export const helpers = (page: Page) => { | ||
| return { | ||
| findByRole: ( | ||
| role: Roles | string, | ||
| name: string | { name: string }, | ||
| ): Locator => { | ||
| return page.locator(locatorByRole(role, name)) | ||
| }, | ||
| findByTestId: (name: string): Locator => | ||
| page.locator(`[data-testid="${name}"]`), | ||
| proceed: async () => await page.locator('[data-testid="proceed"]').click(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve architecture and error handling of the helpers function
- The
proceedmethod is too specific and should be moved to a more appropriate location - Consider adding error handling and timeout configurations
- Methods could benefit from JSDoc documentation for better reusability
export const helpers = (page: Page) => {
+ const DEFAULT_TIMEOUT = 5000
+
return {
+ /**
+ * Finds an element by its ARIA role and name
+ * @param role - The ARIA role of the element
+ * @param name - The accessible name of the element
+ * @param options - Optional timeout configuration
+ */
findByRole: (
role: Roles | string,
name: string | { name: string },
+ options?: { timeout?: number }
): Locator => {
- return page.locator(locatorByRole(role, name))
+ return page.locator(locatorByRole(role, name), {
+ timeout: options?.timeout ?? DEFAULT_TIMEOUT
+ })
},
+ /**
+ * Finds an element by its data-testid attribute
+ * @param name - The value of data-testid
+ * @param options - Optional timeout configuration
+ */
findByTestId: (name: string): Locator =>
- page.locator(`[data-testid="${name}"]`),
- proceed: async () => await page.locator('[data-testid="proceed"]').click(),
+ page.locator(`[data-testid="${name}"]`, {
+ timeout: options?.timeout ?? DEFAULT_TIMEOUT
+ }),
}
}Consider moving the proceed functionality to a more specific test utility or page object:
// e.g., in a checkout-helpers.ts file
export const checkoutHelpers = (page: Page) => ({
proceed: async () => {
const button = page.locator('[data-testid="proceed"]')
await button.waitFor({ state: 'visible' })
await button.click()
}
})| export const locatorByRole = ( | ||
| role: Roles | string, | ||
| name: string | { name: string }, | ||
| ): string => | ||
| typeof name === 'string' | ||
| ? `role=${role}[name="${name}"]` | ||
| : `role=${role}[name="${name.name}"]` |
There was a problem hiding this comment.
Enhance type safety and security of the locatorByRole function
A few potential improvements to consider:
- The role parameter allows any string, which could lead to invalid selectors
- The string concatenation is vulnerable to injection if the name contains quotes
-export const locatorByRole = (
- role: Roles | string,
- name: string | { name: string },
-): string =>
- typeof name === 'string'
- ? `role=${role}[name="${name}"]`
- : `role=${role}[name="${name.name}"]`
+export const locatorByRole = (
+ role: Roles,
+ name: string | { name: string },
+): string => {
+ const escapedName = typeof name === 'string'
+ ? name.replace(/"/g, '\\"')
+ : name.name.replace(/"/g, '\\"')
+ return `role=${role}[name="${escapedName}"]`
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const locatorByRole = ( | |
| role: Roles | string, | |
| name: string | { name: string }, | |
| ): string => | |
| typeof name === 'string' | |
| ? `role=${role}[name="${name}"]` | |
| : `role=${role}[name="${name.name}"]` | |
| export const locatorByRole = ( | |
| role: Roles, | |
| name: string | { name: string }, | |
| ): string => { | |
| const escapedName = typeof name === 'string' | |
| ? name.replace(/"/g, '\\"') | |
| : name.name.replace(/"/g, '\\"') | |
| return `role=${role}[name="${escapedName}"]` | |
| } |
| interface PlaywrightConfigParams { | ||
| webServerUrl: string | ||
| port?: number | ||
| command: string | ||
| cwd?: string | ||
| } | ||
|
|
||
| export const createPlaywrightConfig = ({ | ||
| webServerUrl, | ||
| port, | ||
| command, | ||
| cwd = '../../../', | ||
| timeoutMs = 5 * 60 * 1000, | ||
| }: PlaywrightConfigParams) => |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing type definition for timeoutMs parameter
The timeoutMs parameter is used in the function but not defined in the PlaywrightConfigParams interface.
Apply this diff:
interface PlaywrightConfigParams {
webServerUrl: string
port?: number
command: string
cwd?: string
+ timeoutMs?: number
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface PlaywrightConfigParams { | |
| webServerUrl: string | |
| port?: number | |
| command: string | |
| cwd?: string | |
| } | |
| export const createPlaywrightConfig = ({ | |
| webServerUrl, | |
| port, | |
| command, | |
| cwd = '../../../', | |
| timeoutMs = 5 * 60 * 1000, | |
| }: PlaywrightConfigParams) => | |
| interface PlaywrightConfigParams { | |
| webServerUrl: string | |
| port?: number | |
| command: string | |
| cwd?: string | |
| timeoutMs?: number | |
| } | |
| export const createPlaywrightConfig = ({ | |
| webServerUrl, | |
| port, | |
| command, | |
| cwd = '../../../', | |
| timeoutMs = 5 * 60 * 1000, | |
| }: PlaywrightConfigParams) => |
| webServer: { | ||
| stdout: 'pipe', | ||
| port: port ? port : undefined, | ||
| url: port ? undefined : webServerUrl, | ||
| command, | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 5 * 60 * 1000, | ||
| cwd, | ||
| }, |
There was a problem hiding this comment.
Potential command injection vulnerability in webServer configuration
The command parameter is used directly in the webServer configuration without validation. This could be dangerous if the input comes from an untrusted source.
Consider adding input validation:
+const validateCommand = (cmd: string): string => {
+ if (!/^[\w\s\-./]+$/.test(cmd)) {
+ throw new Error('Invalid command format');
+ }
+ return cmd;
+}
webServer: {
stdout: 'pipe',
port: port ? port : undefined,
url: port ? undefined : webServerUrl,
- command,
+ command: validateCommand(command),
reuseExistingServer: !process.env.CI,
timeout: 5 * 60 * 1000,
cwd,
},| @@ -4,7 +4,7 @@ const devConfig = { | |||
| production: false, | |||
| environment: 'local', | |||
| name: 'local', | |||
| baseApiUrl: 'http://localhost:4444', | |||
| baseApiUrl: `http://localhost:9456`, | |||
There was a problem hiding this comment.
Resolve inconsistent port numbers in development configuration
There's an inconsistency in the development configuration where two different ports are used for baseApiUrl:
- Root level uses port 9456
- templateApi.baseApiUrl uses port 4444
This could lead to confusion and potential issues if different parts of the application expect different ports.
Consider using the same port or environment variables for consistency:
const devConfig = {
// ...
- baseApiUrl: `http://localhost:9456`,
+ baseApiUrl: process.env.DEV_API_URL ?? 'http://localhost:9456',
// ...
templateApi: {
// ...
- baseApiUrl: 'http://localhost:4444',
+ baseApiUrl: process.env.DEV_API_URL ?? 'http://localhost:9456',
// ...
}
}Also applies to: 26-26
| ], | ||
| ] | ||
| : [['dot']]) as ReporterDescription[]), | ||
| ...((process.env.CI ? [['line']] : [['dot']]) as ReporterDescription[]), |
There was a problem hiding this comment.
💡 Codebase verification
Tesults reporter integration still present in package.json
- The
playwright-tesults-reporteris still listed as a dependency in somepackage.jsonfiles, indicating incomplete removal.
🔗 Analysis chain
Verify removal of tesults reporter integration
The simplified reporter configuration looks good, but we should verify that removing the tesults reporter doesn't break any CI/CD dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for tesults reporter usage in CI workflows
# Search for tesults related configurations in CI workflow files
fd -e yml -e yaml . .github/workflows --exec rg -l "playwright-tesults|tesults"
# Check package.json for tesults dependencies
fd package.json -x jq -r '.dependencies + .devDependencies | keys[]' {} | rg "tesults"
Length of output: 875
| responseKey?: string | ||
| camelCaseResponseKey?: boolean | ||
| patchResponse?: boolean | ||
| deepMockKey?: Matchable // TODO type for this: | Matchable[] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update deepMockKey Type Definition
The deepMockKey property in MockGQLOptions has a TODO comment indicating it could be of type Matchable[]. For enhanced type safety and flexibility, consider updating its type definition to accept both Matchable and Matchable[].
Apply this diff to update the type:
- deepMockKey?: Matchable // TODO type for this: | Matchable[]
+ deepMockKey?: Matchable | Matchable[]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deepMockKey?: Matchable // TODO type for this: | Matchable[] | |
| deepMockKey?: Matchable | Matchable[] |
Summary by CodeRabbit
New Features
Documentation
libs/testing/e2elibrary to provide clearer guidance on usage and features.Chores