Revert "Umutuzgur/sc 23256/pwt native code package simple" #1064
Conversation
This reverts commit c4176cb.
WalkthroughThis change removes all Playwright-specific code, constructs, configuration handling, bundling utilities, and related test fixtures from the CLI project. It eliminates PlaywrightCheck, PlaywrightConfig, related dependencies, and all associated logic from commands, services, utilities, and tests. The parser and config loader are refactored to remove Playwright awareness, simplifying their interfaces and implementations. Changes
Possibly related PRs
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
packages/cli/src/services/checkly-config-loader.ts (2)
10-10:⚠️ Potential issueRemove orphaned
PlaywrightConfigimport – the module was deleted
playwright-config.tsand thePlaywrightConfigtype were removed in this PR, yet the import statement is still present.
This will produce a hard-failure at compile time:Cannot find module '../constructs/playwright-config'Please delete the stale import.
-import { PlaywrightConfig } from '../constructs/playwright-config'
42-60:⚠️ Potential issueDelete obsolete
playwrightConfigfield fromChecklyConfigThe
ChecklyConfiginterface still exposes an optionalplaywrightConfig?: PlaywrightConfigproperty and accompanying JSDoc, butPlaywrightConfigno longer exists.
Keeping this reference also breaks the type-checker once the dangling import above is removed.- playwrightConfig?: PlaywrightConfig,Make sure to prune the surrounding JSDoc block as well.
If you plan to re-introduce Playwright support later, re-add the field then.packages/cli/src/commands/test.ts (1)
345-347:⚠️ Potential issueBroken template-literal concatenation results in incorrect error message & potential runtime crash
The three concatenated literals mix single quotes and back-ticks.
- The back-ticked fragment on L346 is never closed, so the code will not compile.
- Even if it did, the two single-quoted fragments would not interpolate
${…}placeholders, printing them verbatim.-throw new Error('Both runLocation and privateRunLocation fields were set in your Checkly config file.' + - ` Please only specify one run location. The configured locations were' + - ' "${configOptions.runLocation}" and "${configOptions.privateRunLocation}"`) +throw new Error( + `Both runLocation and privateRunLocation fields were set in your Checkly config file. ` + + `Please only specify one run location. The configured locations were ` + + `"${configOptions.runLocation}" and "${configOptions.privateRunLocation}".`, +)
🧹 Nitpick comments (11)
packages/cli/src/services/check-parser/package-files/resolver.ts (1)
1-1: Avoid disabling the no-labels rule globally
The file‐level/* eslint-disable no-labels */silences all label warnings, but you only have one labeled statement (resolve:) in this file. It's better to remove the global disable and instead scope it to that single occurrence to minimize lint suppression.Proposed diff:
-/* eslint-disable no-labels */ // ... other code ... - resolve: + // eslint-disable-next-line no-labels + resolve:.github/workflows/release-canary.yml (2)
22-30: Simplify canary tag extraction with step-levelifand GitHub context
You can eliminate the shell workaround and directly use Actions expressions to check for thecanary:prefix and strip it in one go.- - name: Extract dynamic value from canary label - id: extract-canary - run: | - if [[ "${GITHUB_EVENT_LABEL_NAME}" == canary:* ]]; then - CANARY_TAG="${GITHUB_EVENT_LABEL_NAME#canary:}" - echo "CANARY_TAG=${CANARY_TAG}" >> $GITHUB_ENV - fi - env: - GITHUB_EVENT_LABEL_NAME: ${{ github.event.label.name }} + - name: Extract canary tag + if: ${{ startsWith(github.event.label.name, 'canary:') }} + run: echo "CANARY_TAG=${{ github.event.label.name | replace('canary:', '') }}" >> $GITHUB_ENVThis reduces complexity and leverages built-in expression functions.
39-41: Decoupledist-tagaddition into its own step using step-levelif
Splitting thenpm dist-tag addinto a separate step with its ownifcondition improves readability and makes the intent clearer:- - run: | - npm publish --workspace packages/cli --tag experimental - if [[ -n "${{ env.CANARY_TAG }}" ]]; then - npm dist-tag add checkly@${{ env.PR_VERSION }} ${{ env.CANARY_TAG }} - fi + - run: npm publish --workspace packages/cli --tag experimental + + - name: Add canary dist-tag + if: ${{ env.CANARY_TAG != '' }} + run: npm dist-tag add checkly@${{ env.PR_VERSION }} ${{ env.CANARY_TAG }}This isolates the conditional logic and leverages Actions’ built-in
if.packages/cli/src/reporters/list.ts (2)
45-53: Consider colouring each URL individually to preserve readabilityThe new implementation applies
chalk.underline.cyanto the whole comma-separated string:chalk.underline.cyan(links.testTraceLinks.join(', '))When several links are printed this turns the entire line blue, which can be hard to scan and makes it impossible to ⌘-click a single URL in some terminals.
The previousmap(link => chalk.underline.cyan(link)).join(', ')approach kept the benefits of colour & underline while preserving distinct URLs.-printLn(indentString('View trace : ' + chalk.underline.cyan(links.testTraceLinks.join(', ')), 4)) +printLn( + indentString( + 'View trace : ' + links.testTraceLinks.map(l => chalk.underline.cyan(l)).join(', '), + 4, + ), +)Same applies to
videoLinksbelow.
If you decide to keep the current style, please remove the superfluous template literal wrappers (\${links.testResultLink}`` etc.) as they do not add value.
79-87: Duplication – extract helper to avoid divergenceThe three print blocks for
result,trace, andvideoare duplicated in bothonCheckAttemptResultandonCheckEnd. Any future tweak will have to be done in two places and risks drifting.A tiny helper would remove the repetition and guarantee identical formatting:
function printLinks (links: TestResultsShortLinks) { printLn(indentString(`View result: ${chalk.underline.cyan(links.testResultLink)}`, 4)) if (links.testTraceLinks?.length) printLn(indentString(`View trace : ${links.testTraceLinks.map(l => chalk.underline.cyan(l)).join(', ')}`, 4)) if (links.videoLinks?.length) printLn(indentString(`View video : ${links.videoLinks.map(l => chalk.underline.cyan(l)).join(', ')}`, 4)) printLn('') }This keeps both call-sites lean and guarantees consistency.
packages/cli/src/services/project-parser.ts (2)
121-128: Avoid “unused variable” lint noise when instantiating checks
browserCheckis created only for its side-effects (the constructor registers itself with the currentProject).
Assigning it to a constant that is never used will trigger@typescript-eslint/no-unused-varsin many setups.-const browserCheck = new BrowserCheck(pathToPosix(relPath), { - name: path.basename(checkFile), - code: { entrypoint: checkFile }, -}) +new BrowserCheck(pathToPosix(relPath), { + name: path.basename(checkFile), + code: { entrypoint: checkFile }, +})Do the same for
multistepChecklater in the file.
153-158: Same “unused variable” issue as aboveReplicate the in-place instantiation pattern for
MultiStepCheckto keep the linter quiet.packages/cli/src/services/check-parser/parser.ts (1)
182-202: Implementation refactored to handle direct file paths.The refactored implementation effectively handles file paths, directories, and glob patterns without Playwright-specific logic. It's more straightforward and focused.
However, consider adding more explicit error handling for invalid paths that aren't glob patterns.
private async getFilesFromPaths (paths: string[]): Promise<string[]> { const files = paths.map(async (currPath) => { const normalizedPath = pathToPosix(currPath) try { const stats = await fsAsync.lstat(normalizedPath) if (stats.isDirectory()) { return findFilesWithPattern(normalizedPath, '**/*.{js,ts,mjs}', []) } return [normalizedPath] } catch (err) { if (normalizedPath.includes('*') || normalizedPath.includes('?') || normalizedPath.includes('{')) { return findFilesWithPattern(process.cwd(), normalizedPath, []) - } else { + } else { + console.warn(`Path not found: ${normalizedPath}`) return [] } } }) const filesArray = await Promise.all(files) return filesArray.flat() }packages/cli/src/commands/test.ts (2)
200-209: Shadowed variable name hampers readabilityInside the
.filter()chain you reuse the identifiertagsfor the lambda parameter, shadowing the outertagsarray you build.
Consider renaming the callback parameter tocheckTags(or similar) to avoid confusion and aid debugging.
226-231: Snapshot upload happens serially – consider batchingUploading snapshots one check at a time can be slow for large projects.
IfuploadSnapshotssupports bulk input, gather allrawSnapshotsfirst and make a single call, then distribute results to each check.packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts (1)
35-48: DRY violation – identicaltoAbsolutePath()helpersEvery test re-declares the same helper. Extract a shared utility (e.g.,
fixturesPath()in asetup.ts) or declare it once in the outerdescribeblock.This will shorten tests and avoid accidental divergence when paths change.
Also applies to: 72-85, 91-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts-snapshots/Google-test-1-Mobile-Chrome-linux.pngis excluded by!**/*.png
📒 Files selected for processing (22)
.github/workflows/release-canary.yml(2 hunks)packages/cli/package.json(0 hunks)packages/cli/src/commands/deploy.ts(1 hunks)packages/cli/src/commands/test.ts(1 hunks)packages/cli/src/constructs/index.ts(0 hunks)packages/cli/src/constructs/playwright-check.ts(0 hunks)packages/cli/src/reporters/list.ts(2 hunks)packages/cli/src/rest/checkly-storage.ts(0 hunks)packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config-no-browsers.ts(0 hunks)packages/cli/src/services/__tests__/fixtures/playwright-configs/simple-config.ts(0 hunks)packages/cli/src/services/__tests__/playwright-config.spec.ts(0 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts(0 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts(0 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/playwright.config.ts(0 hunks)packages/cli/src/services/check-parser/__tests__/check-parser-fixtures/playwright-project/tests/example.spec.ts(0 hunks)packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts(1 hunks)packages/cli/src/services/check-parser/package-files/resolver.ts(1 hunks)packages/cli/src/services/check-parser/parser.ts(5 hunks)packages/cli/src/services/checkly-config-loader.ts(2 hunks)packages/cli/src/services/playwright-config.ts(0 hunks)packages/cli/src/services/project-parser.ts(2 hunks)packages/cli/src/services/util.ts(1 hunks)
💤 Files with no reviewable changes (12)
- packages/cli/src/rest/checkly-storage.ts
- packages/cli/src/services/check-parser/tests/check-parser-fixtures/playwright-project-snapshots/tests/example.spec.ts
- packages/cli/src/services/tests/playwright-config.spec.ts
- packages/cli/package.json
- packages/cli/src/services/check-parser/tests/check-parser-fixtures/playwright-project/playwright.config.ts
- packages/cli/src/services/check-parser/tests/check-parser-fixtures/playwright-project-snapshots/playwright.config.ts
- packages/cli/src/services/tests/fixtures/playwright-configs/simple-config-no-browsers.ts
- packages/cli/src/services/tests/fixtures/playwright-configs/simple-config.ts
- packages/cli/src/services/check-parser/tests/check-parser-fixtures/playwright-project/tests/example.spec.ts
- packages/cli/src/constructs/index.ts
- packages/cli/src/constructs/playwright-check.ts
- packages/cli/src/services/playwright-config.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/cli/src/reporters/list.ts (1)
packages/cli/src/reporters/util.ts (1)
printLn(459-461)
packages/cli/src/services/checkly-config-loader.ts (4)
packages/cli/src/services/__tests__/fixtures/configs/no-export-config.js (1)
config(1-4)packages/cli/src/services/__tests__/fixtures/configs/no-logical-id-config.js (1)
config(1-3)packages/cli/src/constructs/project.ts (1)
Session(146-221)packages/cli/src/services/util.ts (1)
loadFile(55-71)
packages/cli/src/services/check-parser/parser.ts (2)
packages/cli/src/services/check-parser/errors.ts (1)
DependencyParseError(3-50)packages/cli/src/services/util.ts (2)
pathToPosix(158-163)findFilesWithPattern(267-280)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test - windows-latest
🔇 Additional comments (6)
packages/cli/src/commands/deploy.ts (1)
14-14: Import list looks good – just confirm tree-shaking
BrowserCheckis now the only construct imported from../constructs. That matches the remaining snapshot-upload logic below, so the change is correct.
No further action required.packages/cli/src/services/project-parser.ts (1)
2-2: 👍 Cross-platform pathsAdding
pathToPosixto the util import set is a solid move; it guarantees POSIX-style slashes in logical IDs regardless of host OS.packages/cli/src/services/util.ts (1)
267-271: LGTM - Signature consistent with function implementation.The
ignorePatternparameter is properly used in theglobfunction call, making the signature consistent with its implementation.packages/cli/src/services/check-parser/parser.ts (3)
132-133: API signature simplified by removing Playwright dependency.The method now accepts a simple array of file paths instead of a Playwright configuration object, making it more generic and reusable.
141-145: File filtering logic simplified.The file filtering logic has been simplified to only skip JSON files, removing special handling for Playwright-specific files like snapshots, type declarations, etc.
177-177: Error message updated to match new function signature.The error message now references the array of paths instead of a Playwright config entrypoint, maintaining consistency with the new function signature.
| it('should parse the cli in less than 400ms', async () => { | ||
| const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, '../../../', ...filepath) | ||
| const startTimestamp = Date.now().valueOf() | ||
| const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('/index.ts')]) | ||
| const endTimestamp = Date.now().valueOf() | ||
| expect(res.files).not.toHaveLength(0) | ||
| expect(res.errors).toHaveLength(0) | ||
| const isCI = process.env.CI === 'true' | ||
| expect(endTimestamp - startTimestamp).toBeLessThan(isCI ? 2000 : 400) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded 400 ms threshold is flaky
Build servers often show higher IO latency. The current fallback of 2 s for CI may still fail on slow runners or when the repo grows.
Recommend either:
• Mocking FS reads and removing the time assertion altogether, or
• Deriving the threshold from file count (e.g., files.length * 5 ms) and adding a generous ceiling.
🤖 Prompt for AI Agents
In packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts around
lines 61 to 70, the test uses a hard-coded 400 ms threshold for parsing time
which is flaky on slower CI environments. To fix this, remove the fixed time
assertion and instead mock the filesystem reads to make the test deterministic,
or calculate the threshold dynamically based on the number of files processed
(e.g., files.length * 5 ms) with an upper limit to allow for variability in
slower environments.
| it('should parse the cli in less than 400ms', async () => { | ||
| const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, '../../../', ...filepath) | ||
| const startTimestamp = Date.now().valueOf() | ||
| const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('/index.ts')]) | ||
| const endTimestamp = Date.now().valueOf() | ||
| expect(res.files).not.toHaveLength(0) |
There was a problem hiding this comment.
Path helper swallows intended base path due to leading “/”
Passing '/index.ts' to path.join() resets the accumulated segments, so the resolved path becomes /<index.ts> instead of the CLI root.
That defeats the purpose of the performance test and may hide regressions.
-const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('/index.ts')])
+const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('index.ts')])Alternatively, use path.resolve(repoRoot, 'index.ts') to be explicit.
📝 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.
| it('should parse the cli in less than 400ms', async () => { | |
| const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, '../../../', ...filepath) | |
| const startTimestamp = Date.now().valueOf() | |
| const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('/index.ts')]) | |
| const endTimestamp = Date.now().valueOf() | |
| expect(res.files).not.toHaveLength(0) | |
| it('should parse the cli in less than 400ms', async () => { | |
| const toAbsolutePath = (...filepath: string[]) => path.join(__dirname, '../../../', ...filepath) | |
| const startTimestamp = Date.now().valueOf() | |
| - const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('/index.ts')]) | |
| + const res = await new Parser({}).getFilesAndDependencies([toAbsolutePath('index.ts')]) | |
| const endTimestamp = Date.now().valueOf() | |
| expect(res.files).not.toHaveLength(0) |
🤖 Prompt for AI Agents
In packages/cli/src/services/check-parser/__tests__/parse-files.spec.ts around
lines 61 to 66, the path helper function uses a leading slash in '/index.ts'
which causes path.join to ignore the base path and resolve to an absolute path
from root, breaking the intended test setup. Fix this by removing the leading
slash from the file path argument or replace the path.join call with
path.resolve using the repo root and 'index.ts' to correctly resolve the file
path relative to the CLI root.
umutuzgur
deleted the
revert-1042-umutuzgur/sc-23256/pwt-native-code-package-simple
branch
Reverts #1042