feat: create pw-test command #1092
Conversation
WalkthroughThis change introduces a new Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
ferrandiaz
force-pushed
the
ferran/sc-0/pwt-test-command-flags
branch
from
a5beca7 to
7c7e151
Compare
This comment has been minimized.
This comment has been minimized.
ferrandiaz
added
build
This comment has been minimized.
This comment has been minimized.
ferrandiaz
force-pushed
the
ferran/sc-0/pwt-test-command-flags
branch
from
2727e21 to
ed656b5
Compare
ferrandiaz
force-pushed
the
ferran/sc-0/pwt-test-command-flags
branch
from
ed656b5 to
29ac4ae
Compare
ferrandiaz
added
build
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/cli/src/helpers/test-helper.ts (2)
12-12: Consider adding proper type annotations for the API parameter.The
apiparameter usesanytype, which reduces type safety and IDE support.Consider defining a proper interface or importing the actual API type:
- api: any, + api: ApiClient, // or whatever the actual type is
38-38: Consider adding proper type annotations for the API parameter.Same issue as above - the
apiparameter should have a proper type instead ofany.packages/cli/e2e/__tests__/pw-test.spec.ts (1)
16-25: Consider the necessity of the extended timeout.The test has a very long timeout (130 seconds) which may indicate slow operations. While this might be necessary for Playwright test execution, consider if this can be optimized or if the timeout can be reduced.
packages/cli/src/commands/pw-test.ts (3)
219-220: Missing retry strategy implementation.The retry strategy feature is not yet implemented for Playwright tests.
Would you like me to create an issue to track the implementation of retry strategy support for the pw-test command?
369-379: Fix static analysis warnings in package manager detection.The catch block has an unused variable and unnecessary continue statement.
try { await fs.access(lockFilePath); return pm.name; - } catch (error) { - continue; + } catch { + // Try next package manager }
77-79: Fix misleading flag description.The
skip-recordflag description incorrectly states it will record results when it actually skips recording.'skip-record': Flags.boolean({ - description: 'Record test results in Checkly as a test session with full logs, traces and videos.', + description: 'Skip recording test results in Checkly as a test session.', default: false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/cli/e2e/__tests__/fixtures/test-pwt-native/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
packages/cli/e2e/__tests__/fixtures/test-pwt-native/checkly.config.original.ts(1 hunks)packages/cli/e2e/__tests__/fixtures/test-pwt-native/checkly.config.ts(1 hunks)packages/cli/e2e/__tests__/fixtures/test-pwt-native/package.json(1 hunks)packages/cli/e2e/__tests__/fixtures/test-pwt-native/playwright.config.ts(1 hunks)packages/cli/e2e/__tests__/fixtures/test-pwt-native/test-project/pw-test-project.test.ts(1 hunks)packages/cli/e2e/__tests__/fixtures/test-pwt-native/test/pw-test-example.test.ts(1 hunks)packages/cli/e2e/__tests__/help.spec.ts(1 hunks)packages/cli/e2e/__tests__/pw-test.spec.ts(1 hunks)packages/cli/src/commands/pw-test.ts(1 hunks)packages/cli/src/commands/sync-playwright.ts(2 hunks)packages/cli/src/commands/test.ts(2 hunks)packages/cli/src/helpers/test-helper.ts(1 hunks)packages/cli/src/helpers/write-config-helpers.ts(1 hunks)packages/cli/src/services/checkly-config-loader.ts(3 hunks)packages/cli/src/services/util.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/cli/src/services/util.ts (1)
packages/cli/src/services/checkly-config-loader.ts (2)
PlaywrightSlimmedProp(18-21)ChecklyConfig(23-94)
packages/cli/src/commands/test.ts (4)
packages/cli/src/helpers/test-helper.ts (2)
prepareRunLocation(9-36)prepareReportersTypes(52-57)packages/cli/src/commands/trigger.ts (2)
prepareRunLocation(188-213)prepareReportersTypes(233-238)packages/cli/src/config.ts (1)
Region(8-31)packages/cli/src/reporters/reporter.ts (1)
ReporterType(19-19)
🪛 Biome (1.9.4)
packages/cli/src/commands/pw-test.ts
[error] 375-375: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🪛 GitHub Check: lint
packages/cli/src/commands/pw-test.ts
[warning] 374-374:
'error' is defined but never used
🔇 Additional comments (16)
packages/cli/src/helpers/test-helper.ts (1)
9-36: Good separation of concerns and clear precedence logic.The function properly handles the precedence of CLI flags over config options and includes comprehensive error handling for various edge cases.
packages/cli/e2e/__tests__/help.spec.ts (1)
47-47: LGTM! Correct addition of the new pw-test command to help output verification.The test properly verifies that the new
pw-testcommand appears in the core commands section with the correct description.packages/cli/e2e/__tests__/fixtures/test-pwt-native/package.json (1)
5-5: Verify the Playwright version for security and compatibility.Since this is June 2025 and the specified version is from 2024, please ensure this version is still supported and free from known vulnerabilities.
What is the latest stable version of @playwright/test and are there any known security vulnerabilities in version 1.53.1?packages/cli/e2e/__tests__/fixtures/test-pwt-native/test-project/pw-test-project.test.ts (1)
5-7: Test fixture serves its purpose for E2E testing.While the test logic is trivial, this appears to be intentional for a test fixture used to validate the
pw-testCLI command functionality.packages/cli/e2e/__tests__/fixtures/test-pwt-native/test/pw-test-example.test.ts (1)
4-10: Test fixture appropriately structured for CLI testing.The tests are intentionally simple and serve their purpose as fixtures for validating the
pw-testcommand's tag filtering and execution capabilities.packages/cli/e2e/__tests__/fixtures/test-pwt-native/checkly.config.original.ts (1)
1-23: LGTM! Well-structured test fixture configuration.The Checkly configuration is properly structured with appropriate test settings. The comment about using
.test.tssuffix to avoid Jest transpilation errors is a valuable insight for maintaining test compatibility.packages/cli/e2e/__tests__/fixtures/test-pwt-native/playwright.config.ts (1)
3-17: Configuration looks good with a note on timeout value.The Playwright configuration is well-structured for testing multi-project setups. The specific timeout of 1234ms appears intentional for testing purposes, but verify this is the desired value rather than a placeholder.
packages/cli/e2e/__tests__/fixtures/test-pwt-native/checkly.config.ts (1)
1-23: LGTM! Working copy of test fixture configuration.This configuration mirrors the original version and serves as the working copy for e2e tests that modify the config file. The identical structure and content are appropriate for this testing pattern.
packages/cli/src/services/checkly-config-loader.ts (3)
132-132: Good addition of optional parameter for config writing control.The new
writeChecklyConfigparameter with a default value oftruemaintains backward compatibility while providing flexibility for scenarios where config file writing should be controlled.
147-147: Parameter properly passed through the call chain.The parameter is correctly passed from the main function to the helper function.
160-171: Conditional config writing implemented correctly.The conditional logic properly controls when the config file is written based on the
shouldWriteConfigparameter, while maintaining the existing error handling flow.packages/cli/e2e/__tests__/pw-test.spec.ts (2)
10-14: Good test cleanup pattern implemented.The
afterEachhook properly restores the original config file after each test, ensuring test isolation. This is essential for reliable e2e testing.
27-43: Comprehensive assertions for config modification test.The test thoroughly verifies that the
--create-checkflag properly adds Playwright configuration to the Checkly config file, including checking the structure, path, and specific check properties.Verify that the test fixtures contain the expected
@TAG-Btagged tests that these e2e tests depend on:#!/bin/bash # Description: Verify that @TAG-B tagged tests exist in the fixture directory # Expected: Find test files with @TAG-B annotations fd -e .ts . packages/cli/e2e/__tests__/fixtures/test-pwt-native/ --exec grep -l "@TAG-B" {}packages/cli/src/services/util.ts (1)
294-306: LGTM! Good enhancement for Playwright check configuration.The function now properly supports optional Playwright check configuration while maintaining backward compatibility with sensible defaults.
packages/cli/src/commands/sync-playwright.ts (1)
8-43: Clean refactoring to use shared AST manipulation helpers.Good extraction of common AST manipulation logic into reusable helpers, improving code maintainability and following DRY principles.
packages/cli/src/commands/test.ts (1)
26-163: Good refactoring to use shared test helper functions.The extraction of run location and reporter preparation logic into reusable helpers improves code organization and maintainability.
packages/cli/e2e/__tests__/fixtures/test-pwt-native/test-project/pw-test-project.test.ts
Show resolved
Hide resolved
packages/cli/e2e/__tests__/fixtures/test-pwt-native/test/pw-test-example.test.ts
Show resolved
Hide resolved
ferrandiaz
added
build
This comment has been minimized.
This comment has been minimized.
ferrandiaz
added
build
|
🎉 Experimental release successfully published on npm |
|
Also, the Windows tests are flaky because the GH Windows runner is ultra slow - rerun after implementing the changes above if it still doesn't succeed. |
| const parseArgs = args.map(arg => { | ||
| if (arg.includes(' ')) { | ||
| arg = `"${arg}"` | ||
| } | ||
| return arg | ||
| }) | ||
| const input = parseArgs.join(' ') || '' |
There was a problem hiding this comment.
It would be nice to not do this here, because the unsafeDisplayCommand (used in getTestCommand) quotes args for you. Instead, the input args should just be passed to execCommand() as array items. However, that has some implications for the test name below which also uses the manually quoted input string.
| import { parseProject } from '../services/project-parser' | ||
| import type { Runtime } from '../rest/runtimes' | ||
| import { Diagnostics, Session } from '../constructs' | ||
| import { Flags, ux } from '@oclif/core' |
There was a problem hiding this comment.
You can remove the unused ux import
Affected Components
Notes for the Reviewer
pw-test--are related to checkly, the ones after the separator are related to playwrightnpx checkly pw-test { checkly flags } -- { playwright flags }checkly.config.tsfile nor the monitor defined--create-checkflag, if this flag is passed no test/check will run, it will just add the monitor to the config.This will create something that will look like: