feat(tests): move e2e tests for consultation-portal to their app #17008
Conversation
WalkthroughThe pull request introduces several enhancements to the consultation portal, focusing on end-to-end (E2E) testing. Key changes include the addition of E2E test files for both authenticated and unauthenticated states, updates to the README to include E2E testing instructions, and modifications to environment configurations. A new Playwright configuration file is created, and the Dockerfile is updated to support a multi-stage build process. Additionally, the GitHub workflow is enhanced to include a new E2E CI job, and new functions for generating Docker Compose files are introduced. 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: 6
🧹 Outside diff range and nitpick comments (10)
apps/consultation-portal/proxy.config.ts (2)
1-6: Add TypeScript type definitions for better type safetyThe proxy configuration object would benefit from explicit TypeScript interfaces to ensure type safety and better IDE support.
Consider adding type definitions:
+interface ProxyConfig { + [key: string]: { + target: string; + secure: boolean; + }; +} -const proxyConfig = { +const proxyConfig: ProxyConfig = { '/api/graphql': { target: process.env.API_URL ?? 'http://localhost:4444', secure: false, }, }
3-3: Document the API_URL environment variableThe fallback URL suggests this is for local development, but the environment variable usage should be documented.
Add a comment explaining the environment variable:
+/** + * GraphQL API proxy configuration + * @environment API_URL - The target GraphQL API URL (defaults to http://localhost:4444 for local development) + */ const proxyConfig: ProxyConfig = {apps/consultation-portal/README.md (1)
86-88: Enhance the More Resources section with additional context.While the link to the E2E Testing Library README is helpful, it would be more useful to:
- Briefly describe what information users can find in the linked documentation
- Consider using a repository-relative path for better maintainability
Here's a suggested enhancement:
### More Resources -For further details, refer to the [E2E Testing Library README](../../libs/testing/e2e/README.md). +For detailed information about our E2E testing framework, including: +- Test writing guidelines +- Advanced configuration options +- Custom commands and utilities +- CI/CD integration + +refer to our [E2E Testing Library documentation](/libs/testing/e2e/README.md).apps/consultation-portal/project.json (1)
102-107: Consider enhancing the e2e configuration with environment-specific options.The current configuration could be improved by adding:
- Environment-specific configurations
- Dependencies on other targets (e.g., build)
Consider adding these configurations:
"e2e": { "executor": "@nx/playwright:playwright", "options": { - "config": "{projectRoot}/playwright.config.ts" + "config": "{projectRoot}/playwright.config.ts", + "devServerTarget": "consultation-portal:serve" }, + "configurations": { + "production": { + "devServerTarget": "consultation-portal:serve:production" + } + }, + "dependsOn": ["build"] }apps/consultation-portal/e2e/consultation-portal-unauthenticated.spec.ts (5)
10-10: Fix typo in test suite nameThere's a typo in "unathenticated" - it should be "unauthenticated".
-test.describe('Consultation portal unathenticated', { tag: '@fast' }, () => { +test.describe('Consultation portal unauthenticated', { tag: '@fast' }, () => {
11-12: Add type annotation for RegExp constantAdd type annotation for better type safety.
let context: BrowserContext - const authLink = new RegExp(`^${urls.authUrl}`) + const authLink: RegExp = new RegExp(`^${urls.authUrl}`)
24-26: Add error handling in afterAll hookThe context closure should handle potential errors.
test.afterAll(async () => { - await context.close() + try { + await context.close() + } catch (error) { + console.error('Failed to close browser context:', error) + } })
65-69: Standardize click handler patternsThe click handlers are implemented inconsistently. Some use chained calls while others use await.
Standardize to this pattern:
- 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()Also applies to: 83-87
1-112: Consider architectural improvements for the E2E test suiteTo enhance the test suite's robustness and maintainability:
Consider implementing test data management:
- Add fixtures for common test data
- Use test data factories
- Implement cleanup mechanisms
Configure parallel test execution:
- Add
test.describe.configure({ mode: 'parallel' })for compatible tests- Group sequential tests that share state
Consider adding visual regression testing:
- Implement screenshot comparisons for critical UI states
- Add viewport size variations for responsive testing
Would you like assistance in implementing any of these improvements?
apps/consultation-portal/e2e/consultation-portal-authenticated.spec.ts (1)
1-13: LGTM! Consider organizing tests into nested describesThe test suite setup is well-structured with proper imports and typing. However, consider organizing related tests into nested describe blocks for better readability and maintenance.
Example structure:
test.describe('Consultation portal authenticated', { tag: '@fast' }, () => { // ... setup code ... test.describe('Authentication state', () => { test('logged in user should be shown instead of login', async () => { // ... test code ... }) test('logout button should be visible', async () => { // ... test code ... }) }) test.describe('Navigation', () => { test('subscriptions page should show logged in state', async () => { // ... test code ... }) // ... other navigation tests ... }) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
apps/consultation-portal/README.md(1 hunks)apps/consultation-portal/e2e/consultation-portal-authenticated.spec.ts(1 hunks)apps/consultation-portal/e2e/consultation-portal-unauthenticated.spec.ts(1 hunks)apps/consultation-portal/infra/samradsgatt.ts(1 hunks)apps/consultation-portal/playwright.config.ts(1 hunks)apps/consultation-portal/project.json(1 hunks)apps/consultation-portal/proxy.config.json(0 hunks)apps/consultation-portal/proxy.config.ts(1 hunks)apps/consultation-portal/server.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/consultation-portal/proxy.config.json
✅ Files skipped from review due to trivial changes (1)
- apps/consultation-portal/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/consultation-portal/README.md (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/consultation-portal/e2e/consultation-portal-authenticated.spec.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/consultation-portal/e2e/consultation-portal-unauthenticated.spec.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/consultation-portal/infra/samradsgatt.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/consultation-portal/project.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."
apps/consultation-portal/proxy.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."
apps/consultation-portal/server.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (7)
apps/consultation-portal/server.ts (2)
2-2: LGTM! Good practice using TypeScript module over JSON
The change from JSON to TypeScript module format is a good practice as it enables:
- Type safety for proxy configuration
- Better IDE support
- Dynamic configuration possibilities
Let's verify the new proxy configuration structure:
✅ Verification successful
Proxy configuration change is safe and follows best practices
The verification shows that:
- The proxy configuration is properly structured with GraphQL endpoint configuration
- The proxy config is only used in
server.tsand not directly referenced in e2e tests - The change from JSON to TypeScript module maintains the same functionality while adding type safety
The migration of e2e tests won't be impacted by this change as the proxy configuration:
- Maintains the same endpoint routing
- Uses environment variables appropriately
- Is properly exported and consumed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the proxy configuration structure and its impact on e2e tests
# Check the new proxy config implementation
cat apps/consultation-portal/proxy.config.ts
# Look for any e2e test configurations that might depend on this proxy
rg -g '*.{ts,js,json}' -l 'proxy.config' apps/consultation-portal/e2e
Length of output: 272
Script:
#!/bin/bash
# Let's verify the e2e test setup and proxy usage more broadly
# Check for any proxy-related configurations in e2e tests
rg -g '*.{ts,js,json}' -A 5 'proxy' apps/consultation-portal/e2e
# Check if there are any other files using this proxy configuration
rg -g '*.{ts,js}' -l 'proxy.config' apps/consultation-portal
# Look for any cypress configuration files that might use proxy
fd -e config.ts -e config.js -e cypress.config.ts cypress apps/consultation-portal
Length of output: 245
Line range hint 4-12: Verify external endpoint dependencies for e2e tests
The bootstrap configuration looks good, but since this PR moves e2e tests, we should ensure the graphql endpoint configuration works correctly in the test environment.
Let's check the e2e test setup:
✅ Verification successful
GraphQL endpoint configuration is properly set up for testing
The verification shows that the GraphQL endpoint configuration is well-structured and will work correctly in the test environment because:
- The
next.config.jsproperly configures both client and server-side GraphQL endpoints with fallback values - The
proxy.config.tscorrectly handles GraphQL requests during development with proper fallback to localhost - The
httpLink.tsimplementation properly uses both server and client runtime configurations
The setup uses environment variables with sensible defaults, making it suitable for different environments including testing:
- Default API_URL: http://localhost:4444
- GraphQL path: /api/graphql
- Proper fallback mechanism in httpLink.ts between server and client configs
No additional changes are needed for e2e tests as the configuration is environment-aware and will work correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify graphql endpoint configuration in e2e tests
# Look for graphql endpoint usage in e2e tests
rg -g '*.{ts,js,json}' 'graphqlEndpoint' apps/consultation-portal/e2e
# Check for environment configurations in e2e setup
fd -e config.ts -e config.js -e env -e env.local apps/consultation-portal/e2e
Length of output: 567
Script:
#!/bin/bash
# Description: Verify graphql endpoint configuration in e2e tests with corrected commands
# Look for graphql endpoint usage in e2e tests
fd . apps/consultation-portal/e2e -t f -e ts -e js -e json --exec rg -l 'graphqlEndpoint' {}
# Check for environment configurations in e2e setup
fd . apps/consultation-portal/e2e -t f -e ts -e js --exec rg -l 'config|env' {}
# Check next.config.js for test environment setup
rg -l "serverRuntimeConfig.*graphqlEndpoint" .
Length of output: 416
Script:
#!/bin/bash
# Let's examine the graphql configuration in the consultation portal
# Check the content of httpLink.ts to understand graphql configuration
cat apps/consultation-portal/graphql/httpLink.ts
# Look for test setup files that might configure the endpoint
fd -t f -e test.ts -e spec.ts -e test.js -e spec.js -e config.ts . apps/consultation-portal --exec rg -l "graphql|endpoint" {}
# Check for environment setup in the root of consultation portal
fd -t f -e env -e env.local -e env.test . apps/consultation-portal
Length of output: 821
Script:
#!/bin/bash
# Let's check the proxy configuration and next.config.js
# Check proxy configuration
cat apps/consultation-portal/proxy.config.ts
# Check next.config.js for the consultation portal
fd next.config.js apps/consultation-portal --exec cat {}
# Look for test setup in the e2e directory
fd . apps/consultation-portal --glob "**/e2e/**/{setup,config}*" --exec cat {}
Length of output: 2265
apps/consultation-portal/infra/samradsgatt.ts (2)
31-31: LGTM! The local environment configuration is well-structured.
The addition of the local environment URL follows the established pattern and maintains consistency with other environments. This change appropriately supports the migration of e2e tests to the consultation portal app.
Line range hint 31-35: Verify URL consistency across the codebase.
Let's ensure the auth endpoint path '/samradsgatt/api/auth' is consistently used across the codebase.
✅ Verification successful
Auth endpoint path '/samradsgatt/api/auth' is consistently used across the codebase
The verification results show that the auth endpoint path '/samradsgatt/api/auth' is consistently used across all relevant files in the consultation portal:
- Environment configuration in
infra/samradsgatt.ts - NextAuth configuration in
pages/_app.tsx - Authentication logic in
lib/idsConfig.ts - Environment setup in
README.md - API route configuration in
pages/api/auth/[...nextauth].ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of auth endpoint paths across the codebase
# Search for auth endpoint path patterns
echo "Checking auth endpoint path patterns:"
rg -A 1 "/samradsgatt/api/auth" apps/consultation-portal
# Search for NextAuth configuration files
echo -e "\nChecking NextAuth configuration files:"
fd "next-auth" apps/consultation-portal
# Search for environment variable usage
echo -e "\nChecking NEXTAUTH_URL usage:"
rg "NEXTAUTH_URL" apps/consultation-portal
Length of output: 2024
apps/consultation-portal/README.md (1)
72-72: LGTM!
The section header follows the document's established formatting style.
apps/consultation-portal/project.json (1)
102-107: LGTM! The e2e target configuration is correctly structured.
The configuration properly integrates Playwright with Nx using the official executor and follows the workspace schema.
apps/consultation-portal/e2e/consultation-portal-authenticated.spec.ts (1)
37-138: 🛠️ Refactor suggestion
Enhance test robustness and reduce duplication
While the test cases are comprehensive, there are several areas for improvement:
- Extract common setup/teardown into test fixtures:
const createTestPage = async (context: BrowserContext) => {
const page = await context.newPage()
await page.goto(icelandicAndNoPopupUrl('/samradsgatt'))
return page
}
test('logged in user should be shown instead of login', async () => {
const page = await createTestPage(context)
// ... test code ...
await page.close()
})- Add error state testing:
test('should handle network errors gracefully', async () => {
const page = await context.newPage()
await page.route('**/*', route => route.abort('failed'))
await page.goto(icelandicAndNoPopupUrl('/samradsgatt'))
await expect(page.getByTestId('error-message')).toBeVisible()
await page.close()
})-
Use more reliable selectors:
-
Add visual regression tests for critical UI states:
test('profile page layout should match snapshot', async () => {
const page = await createTestPage(context)
await page.getByRole('button', { name: 'Gervimaður Ameríku' }).click()
await expect(page).toHaveScreenshot('profile-page.png')
await page.close()
})| target: process.env.API_URL ?? 'http://localhost:4444', | ||
| secure: false, |
There was a problem hiding this comment.
💡 Codebase verification
SSL verification should be environment-dependent
The proxy configuration currently disables SSL verification unconditionally, while the application clearly has separate development and production environments. The API_URL is configurable, but the SSL verification isn't.
- In
apps/consultation-portal/proxy.config.ts:- Make
secureconfigurable based on environment (e.g.,process.env.NODE_ENV === 'production') - Add a comment explaining that SSL verification is only disabled for local development
- Make
🔗 Analysis chain
Document security implications of disabled SSL
The secure: false setting disables SSL certificate validation, which could pose security risks in production environments.
Let's verify if this configuration is used in production:
Consider:
- Adding a comment explaining why SSL is disabled
- Making this configurable based on the environment
- Adding validation to ensure SSL is enabled in production
Let's check the content of these files to understand how the environment is handled.
Let's check how the proxy configuration is used in the server file and if there are any environment-specific controls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any environment-specific configurations or documentation
fd -t f -e md -e env "README|CONTRIBUTING|DEPLOYMENT" apps/consultation-portal/
# Look for any production environment configurations
rg -g "!node_modules" -l "NODE_ENV.*production" apps/consultation-portal/
Length of output: 236
Script:
#!/bin/bash
# Check the README content
cat apps/consultation-portal/README.md
# Check the environment configuration
cat apps/consultation-portal/lib/environment.ts
# Look for any proxy configuration imports or usage
rg -g "!node_modules" "proxy.config" apps/consultation-portal/
Length of output: 4638
Script:
#!/bin/bash
# Check the server file content
cat apps/consultation-portal/server.ts
# Look for any environment variables usage in proxy config
rg -g "!node_modules" "process.env" apps/consultation-portal/proxy.config.ts
Length of output: 522
| ### Quick Start | ||
|
|
||
| To run the E2E tests for the `consultation-portal` app: | ||
|
|
||
| ```bash | ||
| # Install dependencies | ||
| yarn install && yarn codegen | ||
|
|
||
| # Start the server | ||
| yarn nx e2e consultation-portal | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the Quick Start guide with additional setup details.
The current instructions might be insufficient for first-time users. Consider adding:
- Prerequisites (e.g., required environment variables)
- Whether the API needs to be running
- Brief explanation of what the tests verify
- Common issues and troubleshooting steps
Here's a suggested enhancement:
### Quick Start
+Prerequisites:
+- Ensure you have the required environment variables (see Development section above)
+- The API should be running locally (follow steps 1-5 in the Development section)
+
To run the E2E tests for the `consultation-portal` app:
```bash
# Install dependencies
yarn install && yarn codegen
# Start the server
yarn nx e2e consultation-portal+These tests verify both authenticated and unauthenticated user flows in the consultation portal.
+
+#### Troubleshooting
+
+If the tests fail, ensure:
+- All environment variables are properly set
+- The API is running and accessible
+- You have the necessary permissions
<!-- This is an auto-generated comment by CodeRabbit -->
| test('nav links on front page should be visible', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await expect(page.getByTestId('all-cases-btn')).toBeVisible() | ||
| await expect(page.getByTestId('subscriptions-btn')).toBeVisible() | ||
| await expect(page.getByTestId('advices-btn')).toBeVisible() | ||
| await expect(page.getByTestId('menu-login-btn')).toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('subscriptions page should show logged out state', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByTestId('subscriptions-btn').click() | ||
| await expect(page.getByTestId('subscriptions-title')).toBeVisible() | ||
| await expect(page.getByTestId('tab-content')).not.toBeVisible() | ||
| await expect(page.getByTestId('action-card')).toBeVisible() | ||
| page | ||
| .getByRole('button', { | ||
| name: 'Skrá mig inn', | ||
| }) | ||
| .click() | ||
| await page.waitForURL(authLink) | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('my subscriptions page should be empty and redirect user to login', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByTestId('subscriptions-btn').click() | ||
| await expect(page.getByTestId('tab-content')).not.toBeVisible() | ||
|
|
||
| page | ||
| .getByRole('link', { | ||
| name: 'Hægt er að afskrá sig hér', | ||
| }) | ||
| .click() | ||
| await page.waitForURL(`**/minaraskriftir`) | ||
| await expect(page.getByTestId('tab-content')).not.toBeVisible() | ||
| await page.waitForURL(authLink) | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('advices page should show logged out state', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByTestId('advices-btn').click() | ||
| await expect(page.getByTestId('action-card')).toBeVisible() | ||
| page | ||
| .getByRole('button', { | ||
| name: 'Skrá mig inn', | ||
| }) | ||
| .click() | ||
| await page.waitForURL(authLink) | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('login button should redirect to login', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| page.getByTestId('menu-login-btn').click() | ||
| await page.waitForURL(authLink) | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('card should show up on frontpage and show case when clicked', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByTestId('front-page-card').first().click() | ||
| await expect(page.getByTestId('short-description')).toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor test cases for better maintainability
Several improvements can be made to enhance the test suite:
- Extract common page setup/teardown logic
- Add timeouts to waitForURL calls
- Standardize click handler patterns
- Add error state assertions
Consider refactoring using this pattern:
// Helper function for common setup/teardown
async function withPage(context: BrowserContext, action: (page: Page) => Promise<void>) {
const page = await context.newPage()
try {
await page.goto(icelandicAndNoPopupUrl('/samradsgatt'))
await action(page)
} finally {
await page.close()
}
}
// Example refactored test
test('nav links on front page should be visible', async () => {
await withPage(context, async (page) => {
await expect(page.getByTestId('all-cases-btn')).toBeVisible()
await expect(page.getByTestId('subscriptions-btn')).toBeVisible()
await expect(page.getByTestId('advices-btn')).toBeVisible()
await expect(page.getByTestId('menu-login-btn')).toBeVisible()
})
})
// Example with timeout and error assertion
test('login button should redirect to login', async () => {
await withPage(context, async (page) => {
await page.getByTestId('menu-login-btn').click()
await page.waitForURL(authLink, { timeout: 5000 })
// Add error state assertion
await expect(page.getByTestId('error-message')).not.toBeVisible()
})
})| await page.waitForURL(`**/minaraskriftir`) | ||
| await expect(page.getByTestId('tab-content')).not.toBeVisible() | ||
| await page.waitForURL(authLink) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeouts to URL wait operations
Add explicit timeouts to waitForURL calls to prevent hanging tests.
- await page.waitForURL(`**/minaraskriftir`)
+ await page.waitForURL(`**/minaraskriftir`, { timeout: 5000 })Also applies to: 88-88, 98-98
| test.beforeAll(async ({ browser }) => { | ||
| context = await session({ | ||
| browser: browser, | ||
| storageState: 'consultation-auth.json', | ||
| idsLoginOn: { | ||
| nextAuth: { nextAuthRoot: `${urls.islandisBaseUrl}/samradsgatt` }, | ||
| }, | ||
| phoneNumber: '0102989', | ||
| homeUrl: `${urls.islandisBaseUrl}/samradsgatt`, | ||
| authTrigger: async (page) => { | ||
| await page.goto('/samradsgatt') | ||
| await page.getByTestId('menu-login-btn').click() | ||
| return `${urls.islandisBaseUrl}/samradsgatt` | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| test.afterAll(async () => { | ||
| if (context) { | ||
| await context.close() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and avoid hard-coded test data
The setup hooks are functional but could be improved in several areas:
- Add error handling in the auth trigger:
authTrigger: async (page) => {
try {
await page.goto('/samradsgatt')
await page.getByTestId('menu-login-btn').click()
return `${urls.islandisBaseUrl}/samradsgatt`
} catch (error) {
console.error('Auth trigger failed:', error)
throw error
}
}- Move test data to a configuration file:
// test-config.ts
export const TEST_DATA = {
phoneNumber: '0102989',
storageState: 'consultation-auth.json',
// ... other test constants
}- Add timeout handling for the session setup
|
|
||
| await expect( | ||
| page.getByRole('button', { name: 'Gervimaður Ameríku' }), | ||
| ).toBeVisible() | ||
| await expect(page.getByTestId('menu-login-btn')).not.toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('subscriptions page should show logged in state', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByTestId('subscriptions-btn').click() | ||
| await expect(page.getByTestId('subscriptions-title')).toBeVisible() | ||
| await expect(page.getByTestId('action-card')).toBeVisible() | ||
| await expect( | ||
| page.getByRole('button', { | ||
| name: 'Skrá mig inn', | ||
| }), | ||
| ).not.toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('my subscriptions page should show logged in state', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt/minaraskriftir')) | ||
| await expect(page.getByTestId('subscriptions-title')).toBeVisible() | ||
| await expect(page.getByTestId('action-card')).not.toBeVisible() | ||
|
|
||
| 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() | ||
| }) | ||
|
|
||
| test('logout button should be visible', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByRole('button', { name: 'Gervimaður Ameríku' }).click() | ||
| await expect(page.getByRole('button', { name: 'Útskrá' })).toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| // Additional tests | ||
| test('user should be able to navigate to profile page', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByRole('button', { name: 'Gervimaður Ameríku' }).click() | ||
| await page.getByRole('link', { name: 'Mínar síður' }).click() | ||
| await expect(page.getByTestId('profile-page')).toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('user should be able to update profile information', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt/minar-sidur')) | ||
|
|
||
| await page.getByTestId('edit-profile-btn').click() | ||
| await page.fill('input[name="name"]', 'Updated Name') | ||
| await page.getByTestId('save-profile-btn').click() | ||
| await expect(page.getByTestId('profile-name')).toHaveText('Updated Name') | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('user should be able to view notifications', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByRole('button', { name: 'Gervimaður Ameríku' }).click() | ||
| await page.getByRole('link', { name: 'Tilkynningar' }).click() | ||
| await expect(page.getByTestId('notifications-page')).toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
|
|
||
| test('user should be able to log out', async () => { | ||
| const page = await context.newPage() | ||
| await page.goto(icelandicAndNoPopupUrl('/samradsgatt')) | ||
|
|
||
| await page.getByRole('button', { name: 'Gervimaður Ameríku' }).click() | ||
| await page.getByRole('button', { name: 'Útskrá' }).click() | ||
| await expect(page.getByTestId('menu-login-btn')).toBeVisible() | ||
|
|
||
| await page.close() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the shell script results, I can now provide a final response since I have sufficient information about the test coverage across components.
Test coverage improvements needed for critical user flows
The current E2E test suite covers basic authenticated flows but lacks coverage for several critical components and scenarios:
- Error handling components (Error404, Error500, CustomError) need test coverage
- Form validation and user input components (DebouncedSearch, SearchAndSort) lack tests
- UI feedback components (PageLoader, CardSkeleton) are untested
- Navigation components (Menu, Breadcrumbs) need test coverage
- Authentication edge cases in AuthProvider component are not verified
Suggested test additions:
- Error page rendering and navigation
- Search functionality with invalid/empty inputs
- Loading states during async operations
- Menu navigation and breadcrumb trails
- Session handling edge cases
🔗 Analysis chain
Verify test coverage for critical user flows
The test suite covers essential authenticated user flows, but consider adding tests for:
- Form validation errors
- API error responses
- Session timeout handling
- Concurrent session management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for missing test coverage in related components
rg "export|interface|type" apps/consultation-portal/components/ -A 2 | \
while IFS= read -r line; do
if [[ $line =~ "export" ]]; then
component=$(echo "$line" | awk '{print $3}')
echo "Checking test coverage for: $component"
rg "$component" apps/consultation-portal/e2e/ -c || echo "No tests found"
fi
done
Length of output: 63773
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
apps/consultation-portal/playwright.config.ts (1)
1-7: Consider adding additional Playwright configurationsThe current configuration is minimal. Consider adding:
- Test timeout settings
- Retry settings for flaky tests
- Browser configurations
- Screenshot/video capture settings for failed tests
const consultationPortalConfig = createPlaywrightConfig({ webServerUrl: 'http://localhost:4200/samradsgatt', + timeout: 30000, + retries: process.env.CI ? 2 : 0, + use: { + screenshot: 'only-on-failure', + video: 'retain-on-failure', + }, })apps/consultation-portal/e2e/setup-local-env.ts (2)
4-12: Add TypeScript return type annotationThe function is missing a return type annotation. Consider adding
voidor the actual return type fromsetupE2EEnvironment.-const initializeLocalEnvironment = () => { +const initializeLocalEnvironment = (): void => {
8-11: Enhance error handlingThe current error handling simply logs and re-throws. Consider:
- Adding more context to the error
- Implementing proper error recovery if possible
try { setupE2EEnvironment({ app: 'consultation-portal' }) } catch (error) { - logger.error('Error while setting up environment: ', error) - throw error + const enhancedError = new Error( + `Failed to setup E2E environment for consultation-portal: ${error.message}` + ) + logger.error(enhancedError) + throw enhancedError }Dockerfile (3)
55-87: Remove commented-out codeThe file contains a large block of commented-out code that appears to be an older version. This should be removed to maintain cleanliness and avoid confusion.
19-20: Optimize cache usageWhile the current cache mount is good, consider adding more granular caching:
-RUN --mount=type=cache,target=/app/.yarn/cache \ - yarn install --immutable && yarn cache clean +RUN --mount=type=cache,target=/app/.yarn/cache \ + --mount=type=cache,target=/root/.yarn/berry/cache \ + yarn install --immutable --prefer-offline && yarn cache clean
1-54: Consider using multi-platform buildsThe Dockerfile currently targets only Alpine. Consider adding support for multiple platforms using BuildKit's platform specifications.
+# syntax=docker/dockerfile:1.4 ARG NODE_IMAGE_TAG=20.15.0-alpine3.20 +ARG TARGETPLATFORM +ARG BUILDPLATFORM
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.dockerignore(1 hunks).github/workflows/pullrequest.yml(6 hunks)Dockerfile(1 hunks)apps/consultation-portal/e2e/setup-local-env.ts(1 hunks)apps/consultation-portal/infra/samradsgatt.ts(1 hunks)apps/consultation-portal/playwright.config.ts(1 hunks)apps/consultation-portal/project.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .dockerignore
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/consultation-portal/infra/samradsgatt.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/consultation-portal/e2e/setup-local-env.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/consultation-portal/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."
apps/consultation-portal/project.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."
🪛 actionlint (1.7.4)
.github/workflows/pullrequest.yml
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (6)
apps/consultation-portal/playwright.config.ts (1)
3-5: Verify the webServerUrl configuration
The webServerUrl is set to a local development URL. Consider making this configurable based on the environment (local/CI).
✅ Verification successful
Let me analyze the search results for port 4200 usage in the codebase.
The search results show that port 4200 is consistently used across the project:
- In configuration files:
docker-compose.ymluses port 4200 for API servicescripts/static-serve.jsandscripts/e2e-ci.jsset default port to 4200scripts/ci/Dockerfilesets ENV PORT=4200
- In server configuration:
libs/infra-next-server/src/lib/bootstrap.tsuses 4200 as default portlibs/testing/e2e/src/lib/modules/urls.tsuses 4200 as default port
- In authentication configuration:
libs/auth-api-lib/seeders/local/004-island.is.client.jsuses localhost:4200 for redirect URIslibs/auth-api-lib/src/lib/passkeys-core/passkeys-core.config.tsincludes localhost:4200
- In documentation and examples:
- Multiple README files and code examples reference localhost:4200
Port 4200 is correctly configured
The webServerUrl configuration using port 4200 aligns with the project's standard port usage across different components and environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the port 4200 is consistently used across the project
rg -g '!node_modules' -g '!dist' "4200"
Length of output: 68737
apps/consultation-portal/project.json (2)
108-123: Well-structured E2E testing configuration!
The E2E testing setup follows best practices:
- Uses Playwright for modern, reliable end-to-end testing
- Properly separates environment setup from test execution
- Ensures sequential execution of setup and tests
102-107: Verify the TypeScript setup script exists
The setup-env target references a TypeScript setup script. Let's verify its existence and accessibility.
✅ Verification successful
TypeScript setup script exists and is accessible
The script setup-local-env.ts is present at the expected location: apps/consultation-portal/e2e/setup-local-env.ts, matching the path referenced in the project configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the setup script exists
fd "setup-local-env.ts" apps/consultation-portal/e2e
Length of output: 100
.github/workflows/pullrequest.yml (3)
Line range hint 230-268: Well-structured E2E CI job configuration!
The e2e-ci job implementation follows CI/CD best practices:
- Proper dependency management with the prepare job
- Matrix strategy for parallel execution
- Consistent environment setup with other jobs
- Unique build IDs for test isolation
463-471: Comprehensive success criteria!
The success job properly includes the new e2e-ci job in its criteria, ensuring all CI checks must pass.
273-273: Verify the runner label configuration
The ec2-runners label is not recognized by actionlint. If this is a custom self-hosted runner label, it should be configured in actionlint.yaml.
✅ Verification successful
Custom runner label ec2-runners is consistently used across workflows
The ec2-runners label appears to be a deliberate configuration used consistently across multiple workflow files in the repository. It's used in conjunction with a custom container image (public.ecr.aws/m3u4c4h9/island-is/actions-runner-public:latest), indicating this is a properly configured self-hosted runner setup. The warning from actionlint can be safely ignored as it's not aware of custom runner labels.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for actionlint configuration
fd "actionlint.ya?ml" .github/
Length of output: 32
Script:
#!/bin/bash
# Check for any workflow files to understand runner configurations
fd "\.ya?ml$" .github/workflows/
# Check the content of workflow files for runner labels
rg "runs-on:" .github/workflows/ -B 2 -A 2
Length of output: 9929
🧰 Tools
🪛 actionlint (1.7.4)
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
| export default initializeLocalEnvironment | ||
|
|
||
| // Add this line to invoke the function | ||
| initializeLocalEnvironment() |
There was a problem hiding this comment.
Reconsider immediate function execution
The function is both exported and immediately executed, which could lead to unexpected behavior when importing this module. Consider:
- Moving the execution to a separate file
- Making the execution conditional
export default initializeLocalEnvironment
-// Add this line to invoke the function
-initializeLocalEnvironment()
+// Execute only if this is the main module
+if (require.main === module) {
+ initializeLocalEnvironment()
+}📝 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 default initializeLocalEnvironment | |
| // Add this line to invoke the function | |
| initializeLocalEnvironment() | |
| export default initializeLocalEnvironment | |
| // Execute only if this is the main module | |
| if (require.main === module) { | |
| initializeLocalEnvironment() | |
| } |
| FROM node:${NODE_IMAGE_TAG} | ||
|
|
||
| # Install Python (runtime requirement if needed) | ||
| RUN apk add --no-cache python3 py3-pip && ln -sf /usr/bin/python3 /usr/bin/python | ||
|
|
||
| # Enable Corepack and Yarn 3.2.3 | ||
| RUN corepack enable && corepack prepare [email protected] --activate | ||
|
|
||
| # Set working directory | ||
| WORKDIR /app | ||
|
|
||
| # Copy from dependencies stage | ||
| COPY --from=dependencies /app/node_modules ./node_modules | ||
| COPY --from=dependencies /app/yarn.lock ./yarn.lock | ||
| COPY --from=dependencies /app/.yarn/ ./.yarn | ||
| COPY --from=dependencies /app/.yarnrc.yml ./.yarnrc.yml | ||
|
|
||
| # Copy source code and necessary configuration files | ||
| COPY ./apps ./apps | ||
| COPY ./libs ./libs | ||
| COPY ./infra ./infra | ||
| COPY ./tools ./tools | ||
| COPY ./tsconfig.base.json ./ | ||
| COPY ./tsconfig.shared.json ./ | ||
| COPY ./package.json ./ | ||
|
|
There was a problem hiding this comment.
Add security best practices
Consider enhancing the security of the final image:
- Run as non-root user
- Add HEALTHCHECK
- Set proper file permissions
# Copy source code and necessary configuration files
COPY ./apps ./apps
COPY ./libs ./libs
COPY ./infra ./infra
COPY ./tools ./tools
COPY ./tsconfig.base.json ./
COPY ./tsconfig.shared.json ./
COPY ./package.json ./
+
+# Create non-root user
+RUN addgroup -S appgroup && adduser -S appuser -G appgroup
+RUN chown -R appuser:appgroup /app
+USER appuser
+
+# Add healthcheck
+HEALTHCHECK --interval=30s --timeout=3s \
+ CMD wget --no-verbose --tries=1 --spider http://localhost:4200/samradsgatt || exit 1Committable suggestion skipped: line range outside the PR's diff.
Datadog ReportAll test runs ✅ 98 Total Test Services: 0 Failed, 90 Passed Test ServicesThis report shows up to 10 services
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
docker-compose.yml (2)
3-11: Consider using YAML anchors to reduce configuration duplicationThe build configuration is identical between services except for the app-specific values. Consider using YAML anchors and aliases to make the configuration more maintainable.
services: + x-build-config: &build-config + context: ./ + dockerfile: scripts/ci/Dockerfile + target: src + args: + NODE_IMAGE_TAG: 20.15.0-alpine3.20 api: build: - context: ./ - dockerfile: scripts/ci/Dockerfile - target: src - args: - NODE_IMAGE_TAG: 20.15.0-alpine3.20 + <<: *build-config args: + <<: *build-config.args APP: api APP_HOME: apps/api APP_DIST_HOME: dist/apps/api consultation-portal: build: - context: ./ - dockerfile: scripts/ci/Dockerfile - target: src - args: - NODE_IMAGE_TAG: 20.15.0-alpine3.20 + <<: *build-config args: + <<: *build-config.args APP: consultation-portal APP_HOME: apps/consultation-portal APP_DIST_HOME: dist/apps/consultation-portalAlso applies to: 171-179
208-209: Enhance network security configurationThe network configuration could be improved by adding internal network configuration for better service isolation.
networks: local: + internal: true + driver: bridge + driver_opts: + com.docker.network.bridge.name: consultation-network
docker-compose.yml
Outdated
| - ./package.json:/app/package.json | ||
| networks: | ||
| - local | ||
| privileged: true |
There was a problem hiding this comment.
Critical: Remove privileged mode and restrict volume mounts
Running containers in privileged mode grants extensive host system access, which is a significant security risk. Additionally, mounting entire source directories might expose sensitive information.
Consider:
- Remove privileged mode unless absolutely necessary
- Restrict volume mounts to only required files
- Use named volumes for persistent data
volumes:
- - ./apps:/app/apps
- - ./libs:/app/libs
+ - ./apps/api:/app/apps/api
+ - ./libs/api-lib:/app/libs/api-lib
- privileged: trueAlso applies to: 204-204
docker-compose.yml
Outdated
| healthcheck: | ||
| test: | ||
| - CMD | ||
| - wget | ||
| - '--spider' | ||
| - '--quiet' | ||
| - http://localhost:9456/liveness | ||
| interval: 10s | ||
| timeout: 5s | ||
| retries: 30 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve health check reliability
The health check uses wget which might not be available in the Alpine image. Consider using curl which is more commonly available, or add wget to the Dockerfile if needed.
healthcheck:
test:
- CMD
- - wget
- - '--spider'
- - '--quiet'
+ - curl
+ - '--fail'
+ - '--silent'
- http://localhost:9456/liveness📝 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.
| healthcheck: | |
| test: | |
| - CMD | |
| - wget | |
| - '--spider' | |
| - '--quiet' | |
| - http://localhost:9456/liveness | |
| interval: 10s | |
| timeout: 5s | |
| retries: 30 | |
| healthcheck: | |
| test: | |
| - CMD | |
| - curl | |
| - '--fail' | |
| - '--silent' | |
| - http://localhost:9456/liveness | |
| interval: 10s | |
| timeout: 5s | |
| retries: 30 |
docker-compose.yml
Outdated
| NEXTAUTH_URL: http://api:4200/samradsgatt/api/auth | ||
| BACKEND_DL_URL: https://samradapi-test.devland.is/api/Documents/ |
There was a problem hiding this comment.
Fix inconsistent port configuration and hardcoded URLs
There are two issues in the configuration:
NEXTAUTH_URLuses port 4200 but the API service runs on 9456BACKEND_DL_URLis hardcoded to a test environment
environment:
- NEXTAUTH_URL: http://api:4200/samradsgatt/api/auth
+ NEXTAUTH_URL: http://api:9456/samradsgatt/api/auth
- BACKEND_DL_URL: https://samradapi-test.devland.is/api/Documents/
+ BACKEND_DL_URL: ${BACKEND_DL_URL:-http://api:9456/api/Documents/}📝 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.
| NEXTAUTH_URL: http://api:4200/samradsgatt/api/auth | |
| BACKEND_DL_URL: https://samradapi-test.devland.is/api/Documents/ | |
| NEXTAUTH_URL: http://api:9456/samradsgatt/api/auth | |
| BACKEND_DL_URL: ${BACKEND_DL_URL:-http://api:9456/api/Documents/} |
docker-compose.yml
Outdated
| FINANCIAL_STATEMENTS_INAO_BASE_PATH: https://dev-re.crm4.dynamics.com/api/data/v9.1 | ||
| FINANCIAL_STATEMENTS_INAO_ISSUER: https://login.microsoftonline.com/05a20268-aaea-4bb5-bb78-960b0462185e/v2.0 | ||
| FINANCIAL_STATEMENTS_INAO_SCOPE: https://dev-re.crm4.dynamics.com/.default | ||
| FINANCIAL_STATEMENTS_INAO_TOKEN_ENDPOINT: https://login.microsoftonline.com/05a20268-aaea-4bb5-bb78-960b0462185e/oauth2/v2.0/token | ||
| ELECTRONIC_REGISTRATION_STATISTICS_API_URL: https://api-staging.thinglysing.is/business/tolfraedi | ||
| FORM_SYSTEM_API_BASE_PATH: https://profun.island.is/umsoknarkerfi | ||
| CONSULTATION_PORTAL_CLIENT_BASE_PATH: https://samradapi-test.devland.is | ||
| FISKISTOFA_ZENTER_CLIENT_ID: 1114 | ||
| HSN_WEB_FORM_ID: 1dimJFHLFYtnhoYEA3JxRK | ||
| SESSIONS_API_URL: http://localhost:9354 | ||
| AUTH_ADMIN_API_PATH: https://identity-server.dev01.devland.is/backend | ||
| AUTH_ADMIN_API_PATHS: '{"development":"https://identity-server.dev01.devland.is/backend"}' | ||
| AUTH_IDS_API_URL: https://identity-server.dev01.devland.is | ||
| HUNTING_LICENSE_PASS_TEMPLATE_ID: 1da72d52-a93a-4d0f-8463-1933a2bd210b | ||
| XROAD_RSK_PROCURING_SCOPE: '["@rsk.is/prokura","@rsk.is/prokura:admin"]' | ||
| UNIVERSITY_GATEWAY_API_URL: http://localhost:9470 | ||
| WATSON_ASSISTANT_CHAT_FEEDBACK_DB_NAME: island-is-assistant-feedback | ||
| XROAD_ADR_MACHINE_LICENSE_PATH: IS-DEV/GOV/10013/Vinnueftirlitid-Protected/rettindi-token-v1 | ||
| XROAD_COURT_BANKRUPTCY_CERT_PATH: IS-DEV/GOV/10019/Domstolasyslan/JusticePortal-v1 | ||
| XROAD_HUNTING_LICENSE_PATH: IS-DEV/GOV/10009/Umhverfisstofnun-Protected/api | ||
| XROAD_FIREARM_LICENSE_PATH: IS-DEV/GOV/10005/Logreglan-Protected/island-api-v1 | ||
| XROAD_DISABILITY_LICENSE_PATH: IS-DEV/GOV/10008/TR-Protected/oryrki-v1 | ||
| XROAD_BASE_PATH: http://localhost:9388 | ||
| XROAD_BASE_PATH_WITH_ENV: http://localhost:9388/r1/IS-DEV | ||
| XROAD_TLS_BASE_PATH: https://securityserver.dev01.devland.is | ||
| XROAD_TLS_BASE_PATH_WITH_ENV: https://securityserver.dev01.devland.is/r1/IS-DEV | ||
| XROAD_CLIENT_ID: IS-DEV/GOV/10000/island-is-client | ||
| XROAD_HEALTH_DIRECTORATE_PATH: IS-DEV/GOV/10015/EmbaettiLandlaeknis-Protected/landlaeknir | ||
| XROAD_HEALTH_INSURANCE_WSDLURL: https://test-huld.sjukra.is/islandrg?wsdl | ||
| XROAD_HEALTH_INSURANCE_ID: IS-DEV/GOV/10007/SJUKRA-Protected | ||
| XROAD_HEALTH_INSURANCE_MY_PAGES_PATH: IS-DEV/GOV/10007/SJUKRA-Protected/minarsidur | ||
| XROAD_INTELLECTUAL_PROPERTIES_PATH: IS-DEV/GOV/10030/WebAPI-Public/HUG-webAPI | ||
| XROAD_INNA_PATH: IS-DEV/GOV/10066/MMS-Protected/inna-v1 | ||
| XROAD_VMST_API_PATH: /VMST-ParentalLeave-Protected/ParentalLeaveApplication-v1 | ||
| XROAD_VMST_MEMBER_CODE: 10003 | ||
| XROAD_DRIVING_LICENSE_PATH: r1/IS-DEV/GOV/10005/Logreglan-Protected/RafraentOkuskirteini-v1 | ||
| XROAD_DRIVING_LICENSE_V2_PATH: r1/IS-DEV/GOV/10005/Logreglan-Protected/RafraentOkuskirteini-v2 | ||
| XROAD_DRIVING_LICENSE_V4_PATH: r1/IS-DEV/GOV/10005/Logreglan-Protected/Okuskirteini-v4 | ||
| XROAD_DRIVING_LICENSE_V5_PATH: r1/IS-DEV/GOV/10005/Logreglan-Protected/Okuskirteini-v5 | ||
| XROAD_PAYMENT_PROVIDER_ID: IS-DEV/GOV/10021/FJS-Public | ||
| XROAD_PAYMENT_BASE_CALLBACK_URL: XROAD:/IS-DEV/GOV/10000/island-is/application-callback-v2/application-payment/ | ||
| XROAD_PAYMENT_ADDITION_CALLBACK_URL: / | ||
| XROAD_DISTRICT_COMMISSIONERS_P_CARD_PATH: IS-DEV/GOV/10016/Syslumenn-Protected/IslandMinarSidur | ||
| XROAD_DISTRICT_COMMISSIONERS_LICENSES_PATH: IS-DEV/GOV/10016/Syslumenn-Protected/RettindiIslandis | ||
| XROAD_FINANCES_PATH: IS-DEV/GOV/10021/FJS-Public/financeIsland | ||
| XROAD_FINANCES_V2_PATH: IS-DEV/GOV/10021/FJS-Public/financeServicesFJS_v2 | ||
| XROAD_HMS_LOANS_PATH: IS-DEV/GOV/10033/HMS-Protected/libra-v1 | ||
| XROAD_HMS_HOUSING_BENEFITS_PATH: IS-DEV/GOV/10033/HMS-Protected/husbot-v1 | ||
| XROAD_MMS_LICENSE_SERVICE_ID: IS-DEV/GOV/10066/MMS-Protected/license-api-v1 | ||
| XROAD_MMS_GRADE_SERVICE_ID: IS-DEV/GOV/10066/MMS-Protected/grade-api-v1 | ||
| XROAD_NATIONAL_REGISTRY_SERVICE_PATH: IS-DEV/GOV/10001/SKRA-Protected/Einstaklingar-v1 | ||
| XROAD_TJODSKRA_API_PATH: /SKRA-Protected/Einstaklingar-v1 | ||
| XROAD_TJODSKRA_MEMBER_CODE: 10001 | ||
| XROAD_PROPERTIES_SERVICE_V2_PATH: IS-DEV/GOV/10033/HMS-Protected/Fasteignir-v1 | ||
| XROAD_PAYMENT_SCHEDULE_PATH: IS-DEV/GOV/10021/FJS-Public/paymentSchedule_v1 | ||
| XROAD_CRIMINAL_RECORD_PATH: r1/IS-DEV/GOV/10005/Logreglan-Protected/Sakavottord-PDF-v2 | ||
| COMPANY_REGISTRY_XROAD_PROVIDER_ID: IS-DEV/GOV/10006/Skatturinn/ft-v1 | ||
| FISHING_LICENSE_XROAD_PROVIDER_ID: IS-DEV/GOV/10012/Fiskistofa-Protected/veidileyfi-v1 | ||
| XROAD_FINANCIAL_AID_BACKEND_PATH: IS-DEV/MUN/10023/samband-sveitarfelaga/financial-aid-backend | ||
| XROAD_VEHICLES_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Mitt-Svaedi-V1 | ||
| XROAD_VEHICLES_MILEAGE_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-Mileagereading-V1 | ||
| XROAD_PASSPORT_LICENSE_PATH: IS-DEV/GOV/10001/SKRA-Cloud-Protected/Forskraning-V1 | ||
| XROAD_VEHICLE_SERVICE_FJS_V1_PATH: IS-DEV/GOV/10021/FJS-Public/VehicleServiceFJS_v1 | ||
| XROAD_VEHICLE_CODETABLES_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-Codetables-V1 | ||
| XROAD_VEHICLE_INFOLOCKS_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-Infolocks-V1 | ||
| XROAD_VEHICLE_OPERATORS_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-Operators-V3 | ||
| XROAD_VEHICLE_OWNER_CHANGE_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-Ownerchange-V2 | ||
| XROAD_VEHICLE_PLATE_ORDERING_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-PlateOrdering-V1 | ||
| XROAD_VEHICLE_PLATE_RENEWAL_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-PlateOwnership-V1 | ||
| XROAD_VEHICLE_PRINTING_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Vehicle-Printing-V1 | ||
| XROAD_DIGITAL_TACHOGRAPH_DRIVERS_CARD_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Okuritar-V1 | ||
| XROAD_CHARGE_FJS_V2_PATH: IS-DEV/GOV/10021/FJS-Public/chargeFJS_v2 | ||
| XROAD_ENERGY_FUNDS_PATH: IS-DEV/GOV/10021/FJS-Public/ElectricCarSubSidyService_v1 | ||
| XROAD_UNIVERSITY_OF_AKUREYRI_PATH: IS-DEV/EDU/10054/UNAK-Protected/brautskraning-v1 | ||
| XROAD_AGRICULTURAL_UNIVERSITY_OF_ICELAND_PATH: IS-DEV/EDU/10056/LBHI-Protected/brautskraning-v1 | ||
| XROAD_BIFROST_UNIVERSITY_PATH: IS-DEV/EDU/10057/Bifrost-Protected/brautskraning-v1 | ||
| XROAD_HOLAR_UNIVERSITY_PATH: IS-DEV/EDU/10055/Holar-Protected/brautskraning-v1 | ||
| XROAD_ICELAND_UNIVERSITY_OF_THE_ARTS_PATH: IS-DEV/EDU/10049/LHI-Protected/brautskraning-v1 | ||
| XROAD_UNIVERSITY_OF_ICELAND_PATH: IS-DEV/EDU/10010/HI-Protected/brautskraning-v1 | ||
| XROAD_WORK_MACHINE_LICENSE_PATH: IS-DEV/GOV/10013/Vinnueftirlitid-Protected/vinnuvelar-token | ||
| XROAD_ICELANDIC_GOVERNMENT_INSTITUTION_VACANCIES_PATH: IS-DEV/GOV/10021/FJS-Protected/recruitment-v1 | ||
| XROAD_RSK_PROCURING_PATH: IS-DEV/GOV/10006/Skatturinn/relationships-v1 | ||
| NATIONAL_REGISTRY_B2C_CLIENT_ID: b464afdd-056b-406d-b650-6d41733cfeb7 | ||
| NATIONAL_REGISTRY_B2C_ENDPOINT: https://skraidentitydev.b2clogin.com/skraidentitydev.onmicrosoft.com/b2c_1_midlun_flow/oauth2/v2.0/token | ||
| NATIONAL_REGISTRY_B2C_SCOPE: https://skraidentitydev.onmicrosoft.com/midlun/.default | ||
| NATIONAL_REGISTRY_B2C_PATH: IS-DEV/GOV/10001/SKRA-Cloud-Protected/Midlun-v1 | ||
| XROAD_AIRCRAFT_REGISTRY_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/Loftfaraskra-V1 | ||
| XROAD_HOUSING_BENEFIT_CALCULATOR_PATH: IS-DEV/GOV/10033/HMS-Protected/calc-v1 | ||
| XROAD_SHIP_REGISTRY_PATH: IS-DEV/GOV/10017/Samgongustofa-Protected/skipaskra-V1 | ||
| XROAD_DIRECTORATE_OF_IMMIGRATION_PATH: IS-DEV/GOV/10011/UTL-Protected/Utl-Umsokn-v1 | ||
| XROAD_SIGNATURE_COLLECTION_PATH: IS-DEV/GOV/10001/SKRA-Cloud-Protected/Medmaeli-v1 | ||
| XROAD_TR_PATH: IS-DEV/GOV/10008/TR-Protected/external-v1 | ||
| XROAD_OFFICIAL_JOURNAL_PATH: IS-DEV/GOV/10014/DMR-Protected/official-journal | ||
| XROAD_JUDICIAL_SYSTEM_SP_PATH: IS-DEV/GOV/10014/Rettarvorslugatt-Private/judicial-system-mailbox-api | ||
| XROAD_OFFICIAL_JOURNAL_APPLICATION_PATH: IS-DEV/GOV/10014/DMR-Protected/official-journal-application | ||
| XROAD_MMS_FRIGG_PATH: IS-DEV/GOV/10066/MMS-Protected/frigg-form-service | ||
| XROAD_HEALTH_DIRECTORATE_ORGAN_DONATION_PATH: IS-DEV/GOV/10015/EmbaettiLandlaeknis-Protected/organ-donation-v1 | ||
| XROAD_HEALTH_DIRECTORATE_VACCINATION_PATH: IS-DEV/GOV/10015/EmbaettiLandlaeknis-Protected/vaccination-v1 | ||
| XROAD_WORK_ACCIDENT_PATH: IS-DEV/GOV/10013/Vinnueftirlitid-Protected/slysaskraning-token | ||
| PROD_MODE: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using environment files for sensitive configuration
The service contains a large number of environment variables with sensitive URLs and paths. Consider moving these to a .env file or using Docker secrets for better security and maintainability.
api:
environment:
- SERVERSIDE_FEATURES_ON:
- APPLICATION_SYSTEM_API_URL: http://localhost:9613
- # ... other env vars
+ env_file:
+ - ./config/api.envCommittable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17008 +/- ##
=======================================
Coverage 35.76% 35.76%
=======================================
Files 6931 6931
Lines 147949 147977 +28
Branches 42167 42192 +25
=======================================
+ Hits 52918 52931 +13
- Misses 95031 95046 +15 Flags with carried forward coverage won't be shown. Click here to find out more. see 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/pullrequest.yml (2)
Line range hint
230-268: Consider updating the actions/cache versionThe e2e-ci job configuration looks good, but there's a recommended update for the cache action.
Update the cache action version in the e2e-ci job:
- uses: actions/cache@v3 + uses: actions/cache@v4
269-319: Consider consolidating e2e test jobsThe workflow now has two separate jobs for e2e tests (
e2eande2e-ci). Consider consolidating them to reduce duplication and maintenance overhead.Potential improvements:
- Merge the jobs using conditional steps based on the test type
- Share common configuration and setup steps
- Use job matrices to handle different test scenarios
🧰 Tools
🪛 actionlint (1.7.4)
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
304-304: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pullrequest.yml(6 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pullrequest.yml
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
304-304: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (3)
.github/workflows/pullrequest.yml (3)
37-41: LGTM: New outputs are properly configured
The new outputs E2E_CI_CHUNKS and E2E_CI_BUILD_ID are correctly defined and follow the existing pattern for other similar outputs.
146-157: LGTM: E2E CI targets preparation is well implemented
The implementation follows best practices:
- Uses proper error handling with
set -euo pipefail - Generates unique build IDs using
uuidgen - Sets appropriate chunk size for e2e-ci tests
478-486: Verify the runner label configuration
The workflow uses the ec2-runners label which isn't in the default GitHub Actions runner list. Ensure this custom runner is properly configured in your self-hosted environment.
✅ Verification successful
The ec2-runners label is properly configured and used consistently
The workflow file shows that ec2-runners is the intended runner label used across multiple jobs in the pipeline. The jobs prepare, tests, e2e-ci, e2e, linting-workspace, linting, build, and success all use this runner label with a custom container image from ECR (public.ecr.aws/m3u4c4h9/island-is/actions-runner-public:latest).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and configuration of ec2-runners label
# Check if the runner label is defined in actionlint config
if [ -f ".actionlint.yaml" ]; then
echo "Checking .actionlint.yaml for runner labels..."
cat .actionlint.yaml
fi
# Check for any runner-related configuration files
fd -t f -e yaml -e yml . | xargs grep -l "ec2-runners"
Length of output: 84
Script:
#!/bin/bash
# Let's check the workflow file directly for runner configuration
cat .github/workflows/pullrequest.yml
# Also check for any other workflow files that might reference runners
fd -t f -e yml -e yaml .github/workflows/ | xargs grep -l "runs-on"
Length of output: 17097
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pullrequest.yml (1)
146-157: Consider adding error handling for chunk generationWhile the implementation is correct, it could benefit from additional error handling.
run: | set -euo pipefail CHUNKS="$(./scripts/ci/generate-chunks.sh e2e-ci)" + if [ $? -ne 0 ]; then + echo "Error: Failed to generate e2e-ci chunks" >&2 + exit 1 + fi if [[ "$CHUNKS" != "[]" ]]; then echo "CHUNKS={\"projects\":$CHUNKS}" >> "$GITHUB_OUTPUT" fi echo BUILD_ID="$GITHUB_RUN_ID-$GITHUB_RUN_NUMBER-$(uuidgen)" >> "$GITHUB_OUTPUT"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pullrequest.yml(6 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pullrequest.yml
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (4)
.github/workflows/pullrequest.yml (4)
37-41: LGTM: New outputs properly integrated
The new outputs E2E_CI_CHUNKS and E2E_CI_BUILD_ID are correctly added to the prepare job's outputs, maintaining consistency with existing output patterns.
461-469: LGTM: Success job properly updated
The success job correctly includes the new e2e-ci job in its dependencies and result checking.
273-273: Verify the runner label configuration
The static analysis tool indicates that "ec2-runners" is an unknown label. If this is a custom self-hosted runner, ensure it's properly configured in your GitHub organization settings.
✅ Verification successful
Custom runner label 'ec2-runners' appears to be consistently used across workflows
The label 'ec2-runners' is used consistently across multiple workflow files (.github/workflows/):
- pullrequest.yml
- push.yml
- pullrequest-lint.yml
- pullrequest-close.yml
- external-checks.yml
- config-values.yaml
- revert-pr.yaml
All jobs using this runner label are also configured with a specific container image from AWS ECR: public.ecr.aws/m3u4c4h9/island-is/actions-runner-public:latest, indicating this is a properly configured setup for self-hosted runners.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check runner label usage across workflows
rg -B 2 -A 2 'runs-on:\s*ec2-runners' .github/workflows/
Length of output: 9099
🧰 Tools
🪛 actionlint (1.7.4)
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
Line range hint 230-268: Verify the timeout configuration
The e2e-ci job's timeout is set to 45 minutes, which is higher than the e2e job's 35 minutes. Please confirm if this extended timeout is intentional.
Also, consider adding retry mechanism for flaky tests:
- name: Running e2e tests
+ uses: nick-fields/retry@v2
+ with:
+ timeout_minutes: 40
+ max_attempts: 2
+ command: ./scripts/ci/40_e2e.sh "${AFFECTED_PROJECT}"
- run: ./scripts/ci/40_e2e.sh "${AFFECTED_PROJECT}"✅ Verification successful
The 45-minute timeout for e2e-ci job is intentionally different from e2e job
The longer timeout for the e2e-ci job is justified because:
- e2e-ci includes additional configurations like Cypress recording (CYPRESS_RECORD_KEY and CYPRESS_PROJECT_ID)
- e2e-ci uses API mocks (API_MOCKS: 'true')
- e2e-ci runs a different test script (40_e2e.sh) compared to the e2e job which uses 'yarn e2e'
These differences in configuration and execution method warrant the extended timeout. The suggestion to add retry mechanism for flaky tests is still valid as a good-to-have improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check timeout settings across e2e jobs in other workflows
rg -A 1 'timeout-minutes:' .github/workflows/
Length of output: 1687
Script:
#!/bin/bash
# Let's check the job names and their configurations to understand the context better
rg -B 5 -A 2 'e2e-ci:|e2e:' .github/workflows/pullrequest.yml
Length of output: 552
Script:
#!/bin/bash
# Let's examine the full configuration of both e2e and e2e-ci jobs to understand their differences
rg -B 10 -A 30 '\s+e2e-ci:|^\s+e2e:' .github/workflows/pullrequest.yml
Length of output: 2558
.github/workflows/pullrequest.yml
Outdated
| e2e: | ||
| needs: | ||
| - prepare | ||
| if: needs.prepare.outputs.E2E_CHUNKS | ||
| runs-on: ec2-runners | ||
| container: | ||
| image: public.ecr.aws/m3u4c4h9/island-is/actions-runner-public:latest | ||
| timeout-minutes: 35 | ||
| env: | ||
| AFFECTED_PROJECT: ${{ matrix.projects }} | ||
| NODE_OPTIONS: --max-old-space-size=4096 | ||
| E2E_BUILD_ID: '${{ needs.prepare.outputs.E2E_BUILD_ID }}-${{ github.run_attempt }}' | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: ${{ fromJson(needs.prepare.outputs.E2E_CHUNKS) }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: 'package.json' | ||
|
|
||
| - name: Setup yarn | ||
| run: corepack enable | ||
|
|
||
| - name: Install Dependencies | ||
| run: yarn install --immutable | ||
|
|
||
| - name: Running e2e tests | ||
| run: yarn e2e "${AFFECTED_PROJECT}" | ||
|
|
||
| - name: Stop and Clean Docker Compose | ||
| run: docker compose down | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating e2e job configurations
The e2e job configuration appears to be a duplicate of e2e-ci with slight differences. Consider creating a reusable workflow to reduce duplication.
Create a new file .github/workflows/reusable-e2e.yml:
name: Reusable E2E Workflow
on:
workflow_call:
inputs:
timeout:
type: number
required: true
command:
type: string
required: true
secrets:
GITHUB_TOKEN:
required: true
jobs:
e2e:
runs-on: ec2-runners
timeout-minutes: ${{ inputs.timeout }}
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version-file: 'package.json'
- name: Setup yarn
run: corepack enable
- name: Running e2e tests
run: ${{ inputs.command }}🧰 Tools
🪛 actionlint (1.7.4)
273-273: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pullrequest.yml (1)
37-40: Consider adding inline documentation for the new e2e-ci configurationThe new e2e-ci related outputs and prepare step would benefit from inline comments explaining their purpose and relationship to the existing e2e configuration.
+ # Output for e2e-ci test chunks and build ID E2E_CI_CHUNKS: ${{ steps.e2e_ci_projects.outputs.CHUNKS }} E2E_CI_BUILD_ID: ${{ steps.e2e_ci_projects.outputs.BUILD_ID }} + # Prepare e2e-ci targets with chunk size 1 for individual test execution - name: Prepare e2e-ci targets id: e2e_ci_projectsAlso applies to: 146-157
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pullrequest.yml(7 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pullrequest.yml
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
238-238: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (3)
.github/workflows/pullrequest.yml (3)
298-309: LGTM! Good improvements to the e2e job reliability
The addition of explicit dependency installation steps and docker compose cleanup improves the reliability and cleanliness of the e2e job execution.
469-469: LGTM! Proper integration of e2e-ci in success criteria
The e2e-ci job is correctly integrated into the success criteria checks.
Also applies to: 476-477
Line range hint 234-271: Document custom runner label configuration
The job uses a custom runner label ec2-runners which isn't recognized by actionlint. Consider adding the custom runner labels to actionlint.yaml configuration file to suppress these warnings.
🧰 Tools
🪛 actionlint (1.7.4)
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
238-238: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
| if: false | ||
| runs-on: ec2-runners | ||
| steps: | ||
| - run: echo "This step won't run" | ||
| # needs: | ||
| # - prepare | ||
| # if: needs.prepare.outputs.TEST_CHUNKS | ||
| # runs-on: ec2-runners | ||
| # container: | ||
| # image: public.ecr.aws/m3u4c4h9/island-is/actions-runner-public:latest | ||
| # timeout-minutes: 45 | ||
| # env: | ||
| # AFFECTED_PROJECTS: ${{ matrix.projects }} | ||
| # MAX_JOBS: 1 | ||
| # strategy: | ||
| # fail-fast: false | ||
| # matrix: ${{ fromJson(needs.prepare.outputs.TEST_CHUNKS) }} | ||
| # steps: | ||
| # - uses: actions/checkout@v4 | ||
| # with: | ||
| # fetch-depth: 0 | ||
|
|
||
| # - uses: actions/setup-node@v4 | ||
| # with: | ||
| # node-version-file: 'package.json' | ||
|
|
||
| # - name: Setup yarn | ||
| # run: corepack enable | ||
|
|
||
| # - name: Get cache | ||
| # id: get-cache | ||
| # uses: ./.github/actions/get-cache | ||
| # with: | ||
| # github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| # keys: ${{ needs.prepare.outputs.CACHE_KEYS }} | ||
| # enable-cache: 'node_modules,cypress,generated-files' | ||
|
|
||
| # - uses: ./.github/actions/unit-test | ||
| # with: | ||
| # dd-api-key: '${{ secrets.DD_API_KEY }}' | ||
| # codecov-token: ${{ secrets.CODECOV_TOKEN }} | ||
| # aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| # aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| # docker-registry: 821090935708.dkr.ecr.eu-west-1.amazonaws.com/ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code
Instead of keeping the commented-out test job configuration, consider removing it entirely since it's tracked in version control. If this code needs to be referenced later, it can be found in the git history.
🧰 Tools
🪛 actionlint (1.7.4)
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
.github/workflows/pullrequest.yml (2)
Line range hint
234-271: Consider documenting the e2e-ci job's purposeThe new e2e-ci job looks well-structured, but it would benefit from a comment explaining its specific purpose and how it differs from the regular e2e job.
+ # Runs end-to-end tests for consultation-portal in CI environment e2e-ci: needs: - prepare🧰 Tools
🪛 actionlint (1.7.4)
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
238-238: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
Line range hint
1-502: Configure custom runner labels in actionlint.yamlThe static analysis tool reports unknown runner label "ec2-runners". Consider adding custom runner labels to actionlint.yaml configuration file.
# .github/actionlint.yaml +runner-labels: + - "ec2-runners"🧰 Tools
🪛 actionlint (1.7.4)
403-403: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
439-439: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
474-474: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
infra/src/cli/generate-docker-compose.ts (3)
90-91: Use Consistent Logging Instead of Console StatementsThe function
getAppPathsusesconsole.logfor logging messages. For consistency and better log management, replaceconsole.logwithlogger.info.Apply this diff to replace
console.logstatements:- console.log(`Fetching project paths for: ${appName}`) + logger.info(`Fetching project paths for: ${appName}`) ... - console.log(`Raw output from command:\n${rawOutput}`) + logger.debug(`Raw output from command:\n${rawOutput}`)Also applies to: 100-100
315-358: Leverage Existing Libraries for YAML GenerationThe custom
generateYAMLfunction may not handle all edge cases of YAML serialization. Consider using a standard library likejs-yamlfor robustness and maintainability.Apply this diff to use
js-yamlfor YAML conversion:+ import yaml from 'js-yaml'; ... - // Convert the config object to a YAML string - const yamlContent = generateYAML(config) + // Use js-yaml to convert the config object + const yamlContent = yaml.dump(config) ... - const generateYAML = (obj: any, indentLevel = 0): string => { - // Custom YAML generation logic - }Remember to add
js-yamlto your project dependencies if not already included.
153-167: Refactor Duplicate Docker Compose ConfigurationsThe Docker Compose configurations for the main service and dependencies have duplicated code. Extract common configurations to reduce redundancy.
Consider creating a helper function to generate the common parts:
function createServiceConfig(name: string, service: LocalrunService, appHome: string, appDistHome: string, command: string) { return { build: { context: './', dockerfile: 'scripts/ci/Dockerfile', target: 'src', args: { NODE_IMAGE_TAG: '20.15.0-alpine3.20', APP: name, APP_HOME: appHome, APP_DIST_HOME: appDistHome, }, }, container_name: name, env_file: [`.env.${name}`], environment: { ...service.env, IP: '0.0.0.0', }, ports: [`${service.port}:${service.port}`], command, working_dir: '/app', volumes: [ './apps:/app/apps', './libs:/app/libs', './.yarn:/app/.yarn', './yarn.lock:/app/yarn.lock', './package.json:/app/package.json', ], networks: ['local'], privileged: true, ...(service.port ? addHealthCheck(name, service.port) : {}), }; }Then use this function for both main service and dependencies.
Also applies to: 176-210, 215-273
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pullrequest.yml(8 hunks).gitignore(1 hunks)infra/src/cli/generate-docker-compose.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pullrequest.yml
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
238-238: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
403-403: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
439-439: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (6)
.github/workflows/pullrequest.yml (6)
37-41: LGTM: New outputs for e2e-ci configuration
The new outputs E2E_CI_CHUNKS and E2E_CI_BUILD_ID are properly integrated with the existing output structure.
146-157: LGTM: Well-structured e2e-ci preparation step
The preparation step correctly:
- Uses a chunk size of 1 for granular test distribution
- Generates a unique build ID using UUID
- Properly handles empty chunks case
486-494: LGTM: Proper integration of e2e-ci in success checks
The success job correctly includes the new e2e-ci job in its dependencies and result checking.
402-471: 🛠️ Refactor suggestion
Remove commented-out jobs
Similar to the tests job, the commented-out linting and build jobs should be removed as they're preserved in version control history.
🧰 Tools
🪛 actionlint (1.7.4)
403-403: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
439-439: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
189-233: 🛠️ Refactor suggestion
Remove commented-out code
Instead of keeping the commented-out test job configuration, consider removing it entirely since it's tracked in version control history.
- if: false
- runs-on: ec2-runners
- steps:
- - run: echo "This step won't run"
- # needs:
- # - prepare
- # if: needs.prepare.outputs.TEST_CHUNKS
- # runs-on: ec2-runners
- # container:
- [... rest of commented code ...]Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
298-318: Verify Docker Compose cleanup reliability
The new Docker Compose cleanup step might fail silently if the compose file is not found or if the command fails.
| const relevantService = Object.entries(renderedServices) | ||
| .filter(([name]) => services[0] === name) | ||
| .map(([name, service]) => ({ name, service }))[0] |
There was a problem hiding this comment.
Add Checks for Empty Service Arrays to Prevent Errors
Accessing services[0] without checking if the array is empty could lead to runtime errors. Ensure the services array has the required elements before accessing.
Apply this diff to add a safeguard:
+ if (!services || services.length === 0) {
+ throw new Error('No services specified for Docker Compose generation.')
+ }
const relevantService = Object.entries(renderedServices)
.filter(([name]) => services[0] === name)
.map(([name, service]) => ({ name, service }))[0]📝 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 relevantService = Object.entries(renderedServices) | |
| .filter(([name]) => services[0] === name) | |
| .map(([name, service]) => ({ name, service }))[0] | |
| if (!services || services.length === 0) { | |
| throw new Error('No services specified for Docker Compose generation.') | |
| } | |
| const relevantService = Object.entries(renderedServices) | |
| .filter(([name]) => services[0] === name) | |
| .map(([name, service]) => ({ name, service }))[0] |
| if (error instanceof Error) { | ||
| console.error( | ||
| `Error fetching project paths for '${appName}': ${error.message}`, | ||
| ) | ||
| } else { | ||
| console.error(`Error fetching project paths for '${appName}': ${error}`) | ||
| } | ||
| console.error(`Command attempted: yarn nx show project ${appName} --json`) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Logger for Error Handling and Avoid process.exit(1)
Instead of using console.error and process.exit(1), use logger.error and throw an error to allow upstream error handling.
Apply this diff to improve error handling:
- console.error(
+ logger.error(
`Error fetching project paths for '${appName}': ${error.message}`,
)
...
- console.error(`Command attempted: yarn nx show project ${appName} --json`)
- process.exit(1)
+ logger.error(`Command attempted: yarn nx show project ${appName} --json`)
+ throw errorCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pullrequest.yml (3)
37-41: Consider using a shared build ID formatThe build IDs for both e2e and e2e-ci jobs follow the same pattern. Consider extracting this to a reusable format to maintain consistency and ease future updates.
- E2E_CI_BUILD_ID: ${{ steps.e2e_ci_projects.outputs.BUILD_ID }} - E2E_BUILD_ID: ${{ steps.e2e_projects.outputs.BUILD_ID }} + E2E_BUILD_FORMAT: "$GITHUB_RUN_ID-$GITHUB_RUN_NUMBER-$(uuidgen)"
481-489: Consider consolidating success checksThe success job has multiple similar checks that could be consolidated into a reusable function.
+ - name: Check job results + run: | + check_job() { + [[ $1 != "failure" ]] || exit 1 + } + check_job "${{ needs.prepare.result }}" + check_job "${{ needs.tests.result }}" + check_job "${{ needs.e2e-ci.result }}" + check_job "${{ needs.e2e.result }}" + check_job "${{ needs.linting.result }}" + check_job "${{ needs.run-shellcheck.result }}" + check_job "${{ needs.formatting.result }}" + check_job "${{ needs.build.result }}"
Line range hint
1-1: Configure custom runner labels in actionlint.yamlThe static analysis tool reports unknown runner label "ec2-runners". If this is a custom self-hosted runner, configure it in actionlint.yaml to suppress these warnings.
Create or update
.github/actionlint.yaml:self-hosted-runner: - ec2-runners
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pullrequest.yml(8 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/pullrequest.yml
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
238-238: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
398-398: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
434-434: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (4)
.github/workflows/pullrequest.yml (4)
146-157: LGTM: e2e-ci preparation step
The preparation step for e2e-ci targets is well structured with appropriate error handling and unique build ID generation.
189-233: Remove commented-out code
Instead of keeping the commented-out test job configuration, consider removing it entirely since it's tracked in version control. If this code needs to be referenced later, it can be found in the git history.
🧰 Tools
🪛 actionlint (1.7.4)
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
397-466: Remove commented-out linting and build jobs
Similar to the tests job, these commented-out configurations should be removed as they are tracked in version control.
🧰 Tools
🪛 actionlint (1.7.4)
398-398: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
434-434: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
Line range hint 234-271: Verify Cypress configuration for e2e-ci job
The e2e-ci job configuration looks good, but there are a few points to verify:
- The CYPRESS_PROJECT_ID is hardcoded
- API_MOCKS is set to 'true' which might not be suitable for all test scenarios
✅ Verification successful
Let me gather more information about the Cypress configuration to make a more informed decision.
Let me check the nx configuration and e2e test setup to understand how these values are being used.
E2E testing configuration appears to be properly configured
The hardcoded CYPRESS_PROJECT_ID and API_MOCKS=true settings in the workflow are intentional:
- The project ID is specific to this project's Cypress Cloud integration
- API_MOCKS=true is a deliberate choice for CI environment, supported by multiple mock implementations across the codebase (found in libs/shared/mocking, libs/api/mocks, and various app-specific mock directories)
- The e2e-ci configuration is consistently implemented across the project structure using nx workspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Cypress project ID is configured in any other location
rg -l "CYPRESS_PROJECT_ID.*4q7jz8"
# Check for other API_MOCKS configurations
rg -l "API_MOCKS.*true"
Length of output: 572
Script:
#!/bin/bash
# Check for Cypress configuration files
fd -e json -e js cypress.config
# Check for environment configurations that might override these settings
rg -l "CYPRESS_PROJECT_ID|API_MOCKS" .env
# Look for any documentation about e2e testing setup
fd -g "*e2e*.md"
fd -g "*cypress*.md"
# Check the e2e test script content
cat ./scripts/ci/40_e2e.sh
Length of output: 249
Script:
#!/bin/bash
# Check nx configuration for e2e testing
fd -e json project.json -x grep -l "e2e-ci"
# Look for e2e test configurations in package.json
rg "e2e-ci" package.json
# Check for any cypress related configurations in the codebase
rg -l "cypress\.config"
# Check the content of a sample project.json that has e2e-ci configuration
fd -e json project.json -x grep -l "e2e-ci" | head -n1 | xargs cat
Length of output: 1322
🧰 Tools
🪛 actionlint (1.7.4)
190-190: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
238-238: label "ec2-runners" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
| - name: Building NodeJS dependencies | ||
| working-directory: infra | ||
| run: yarn install --immutable | ||
|
|
||
| - name: Running e2e tests | ||
| run: ./scripts/ci/40_e2e.sh "${AFFECTED_PROJECT}" | ||
| run: yarn e2e "${AFFECTED_PROJECT}" | ||
|
|
||
| - name: Stop and Clean Docker Compose | ||
| run: docker compose down |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for Docker Compose cleanup
The Docker Compose cleanup step should handle potential failures gracefully.
- run: docker compose down
+ run: docker compose down || 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.
| - name: Building NodeJS dependencies | |
| working-directory: infra | |
| run: yarn install --immutable | |
| - name: Running e2e tests | |
| run: ./scripts/ci/40_e2e.sh "${AFFECTED_PROJECT}" | |
| run: yarn e2e "${AFFECTED_PROJECT}" | |
| - name: Stop and Clean Docker Compose | |
| run: docker compose down | |
| - name: Building NodeJS dependencies | |
| working-directory: infra | |
| run: yarn install --immutable | |
| - name: Running e2e tests | |
| run: yarn e2e "${AFFECTED_PROJECT}" | |
| - name: Stop and Clean Docker Compose | |
| run: docker compose down || true |
Summary by CodeRabbit
New Features
Bug Fixes
.dockerignoreand.gitignoreto exclude unnecessary files.Documentation
Chores