chore(tests): Support more options E2E Playwright config #16982
Conversation
WalkthroughThe pull request introduces changes to the E2E Testing Library, primarily updating the Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (5)
libs/testing/e2e/src/lib/config/playwright-config.ts (3)
24-26: Enhance JSDoc parameter descriptionsThe new parameters need more detailed descriptions explaining their purpose and expected values.
Apply this diff to improve the documentation:
* @param {number} config.timeoutMs - Timeout in milliseconds for server startup. - * @param {string} config.app - Application to run. - * @param {string} config.dependencies - Space-separated list of dependencies. - * @param {boolean} config.proxies - Whether to use proxies. + * @param {string} config.app - Name of the application to run in the local environment. + * @param {string[]} config.dependencies - List of dependent services required to run the application. + * @param {boolean} config.proxies - Whether to enable proxy configuration for the local environment.
40-51: Consider refactoring command string constructionThe current string concatenation approach could be improved for maintainability and safety.
Consider using an array to build the command parts:
- command = `yarn infra run-local-env ${app} --print --no-secrets` - if (dependencies.length > 0) { - command += ` --dependencies ${dependencies}` - } - if (proxies) { - command += ' --proxies' - } + const commandParts = [ + 'yarn infra run-local-env', + app, + '--print', + '--no-secrets', + ...(dependencies.length > 0 ? ['--dependencies', dependencies.join(' ')] : []), + ...(proxies ? ['--proxies'] : []) + ] + command = commandParts.join(' ')
Line range hint
1-79: Well-structured configuration module designThe module follows good architectural principles:
- Provides a flexible and type-safe configuration interface
- Maintains backward compatibility while adding new features
- Exports types for reuse across different NextJS applications
libs/testing/e2e/README.md (2)
Line range hint
25-35: Consider enhancing the configuration exampleWhile the current example is good, it would be helpful to showcase more configuration options that were recently added to
PlaywrightConfigParams, such asapp,dependencies, andproxies.Consider expanding the example like this:
import { createPlaywrightConfig } from '@island.is/testing/e2e' const playwrightConfig = createPlaywrightConfig({ webServerUrl: '<web-server-url>', command: '<command>', // Optional app: '<app-name>', // Optional: Specify the app being tested dependencies: ['<dependent-app>'], // Optional: List dependent apps proxies: { // Optional: Configure proxies '/api': 'http://localhost:3000' } }) export default playwrightConfig🧰 Tools
🪛 LanguageTool
[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e --skip-nx-cache ``` - Run Tests with Tags: Use tags to include ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
61-71: Consider varying sentence structure in the "Run Tests with Tags" sectionTo improve readability and flow, consider rewording the examples to avoid repetitive sentence beginnings.
Here's a suggested revision:
- **Run Tests with Tags**: Filter tests using tags: ```bash # Execute tests with @fast tag only yarn nx e2e <app-name> --grep @fast # Skip tests marked with @fast yarn nx e2e <app-name> --grep-invert @fast # Include tests tagged as either @fast or @slow yarn nx e2e <app-name> --grep "@fast|@slow"<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...e <app-name> --skip-nx-cache ``` - **Run Tests with Tags**: Use tags to include ... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between be1824e1bb9c09b5cf61988f996c2acd7eeac2ec and 0e87b4477d3a443fba847194a9e555ce23907656. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `libs/testing/e2e/README.md` (2 hunks) * `libs/testing/e2e/src/lib/config/playwright-config.ts` (4 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (2)</summary> <details> <summary>libs/testing/e2e/README.md (1)</summary> Pattern `libs/**/*`: "Confirm that the code adheres to the following: - Reusability of components and hooks across different NextJS apps. - TypeScript usage for defining props and exporting types. - Effective tree-shaking and bundling practices." </details> <details> <summary>libs/testing/e2e/src/lib/config/playwright-config.ts (1)</summary> Pattern `libs/**/*`: "Confirm that the code adheres to the following: - Reusability of components and hooks across different NextJS apps. - TypeScript usage for defining props and exporting types. - Effective tree-shaking and bundling practices." </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>libs/testing/e2e/README.md</summary> [style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...e <app-name> --skip-nx-cache ``` - **Run Tests with Tags**: Use tags to include ... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) </details> </details> </details> <details> <summary>🔇 Additional comments (4)</summary> <details> <summary>libs/testing/e2e/src/lib/config/playwright-config.ts (2)</summary> `6-9`: **LGTM: Interface changes are well-typed and flexible** The new optional properties enhance configuration flexibility while maintaining type safety. --- `36-39`: **LGTM: Parameter defaults are well-chosen** The default values provide sensible fallbacks while maintaining type safety. </details> <details> <summary>libs/testing/e2e/README.md (2)</summary> `44-44`: **Command usage updates look good!** The migration from `yarn e2e` to `yarn nx e2e` is consistently applied across all command examples, properly reflecting the new command structure for the nx workspace. Also applies to: 52-52, 58-58, 65-65, 68-68, 71-71, 83-83, 89-89 --- Line range hint `1-24`: **Documentation structure is well-organized!** The README follows documentation best practices with clear sections, proper hierarchy, and comprehensive coverage of the E2E testing library usage. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...e <app-name> --skip-nx-cache ``` - **Run Tests with Tags**: Use tags to include ... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) </details> </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
24-26: Enhance return type documentationThe JSDoc could be more specific about the return type structure and its configuration options.
Add more details to the return type documentation:
- * @returns A configuration object for Playwright E2E tests. + * @returns {import('@playwright/test').PlaywrightTestConfig} A configuration object + * containing test directory settings, parallel execution options, retry logic, + * reporter configuration, and web server settings.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/testing/e2e/src/lib/config/playwright-config.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (4)
libs/testing/e2e/src/lib/config/playwright-config.ts (4)
6-9: LGTM: Interface changes follow TypeScript best practices
The interface updates properly define optional properties with clear types, maintaining good TypeScript practices.
36-39: LGTM: Well-structured parameter defaults
The new parameters are properly defined with sensible default values.
Line range hint 54-80: LGTM: Well-structured Playwright configuration
The configuration follows Playwright best practices with appropriate settings for different environments (CI vs local).
44-52:
Critical: Command Injection Vulnerability
The command construction is vulnerable to command injection attacks through unsanitized input in app and dependencies. An attacker could inject malicious shell commands through these parameters.
Add input validation before command construction:
if (!command) {
+ // Validate app name (alphanumeric, hyphens, and underscores only)
+ if (!/^[\w-]+$/.test(app)) {
+ throw new Error('Invalid app name. Only alphanumeric characters, hyphens, and underscores are allowed.');
+ }
+ // Validate each dependency name
+ if (!dependencies.every(dep => /^[\w-]+$/.test(dep))) {
+ throw new Error('Invalid dependency name. Only alphanumeric characters, hyphens, and underscores are allowed.');
+ }
command = `yarn infra run-local-env ${app} --print --no-secrets`
if (dependencies.length > 0) {
command += ` --dependencies ${dependencies.join(' ')}`
}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/testing/e2e/src/lib/config/playwright-config.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/testing/e2e/src/lib/config/playwright-config.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (4)
libs/testing/e2e/src/lib/config/playwright-config.ts (4)
6-9: LGTM: Interface changes are well-structured
The new optional properties are properly typed and follow TypeScript best practices.
24-26: LGTM: Documentation is comprehensive
The JSDoc comments are well-maintained with clear descriptions for the new parameters.
36-39: LGTM: Parameter defaults are well-chosen
The new parameters have appropriate default values that maintain backward compatibility.
Line range hint 54-80: LGTM: Configuration is well-structured
The Playwright configuration is comprehensive and properly handles both CI and local development environments.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16982 +/- ##
=======================================
Coverage 35.62% 35.62%
=======================================
Files 6914 6914
Lines 145990 145990
Branches 41446 41446
=======================================
Hits 52011 52011
Misses 93979 93979
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
* chore(tests): Support more options E2E Playwright config * Fix nested if statement * Small correction --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Summary by CodeRabbit
Documentation
yarn e2etoyarn nx e2e.New Features