Add runCode functionality and enhance screenshot capabilities #271
Conversation
Add runCode function to execute temporary scripts in various languages
Enhance click functionality in findElement to support button option
…images for consistency
Implement viewport resizing and enhance driver capabilities
…improve image comparison logic
Enhance screenshot functionality for visibility and cropping
…ove viewport dimensions from Firefox options
2.19.0: - Added runCode action - Added custom viewport sizing - Added right and middle button clicking - Added dynamic screenshot resizing to account for pixel density differences between devices when doing pixel diffs - Fixed element visibility issues when attempting to screenshot an element
…rs and doc-detective-common
Bump dependencies
dosubot
bot
added
the
size:XL
WalkthroughThis pull request updates the CI workflow to include Node.js 22 and upgrades several GitHub Actions to v4. Test specifications have been modified: an HTTP request test was replaced by a screenshot test, and a new test for right-click actions was added. The test runner configuration now uses Firefox instead of Chrome, and test files have been updated accordingly. New functionalities include the introduction of a runCode module for executing code snippets, enhanced screenshot cropping with retries and flexible click actions, and extensive dependency updates in package.json. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant RC as runCode
participant VF as Validator
participant TS as TempScript Manager
participant SC as Shell Executor
participant FS as File System
TR->>RC: runCode(step)
RC->>VF: validate(step)
VF-->>RC: validated step (or error)
RC->>TS: createTempScript(code, language)
TS-->>RC: temp file path
RC->>RC: Determine execution command
RC->>SC: Verify command availability (spawnCommand)
alt Command available
RC->>SC: Execute script (runShell)
SC-->>RC: Return stdout, stderr, exit code
else Command missing
RC-->>TR: Return failure status
end
RC->>FS: Delete temporary script file
FS-->>RC: File cleanup status
RC-->>TR: Return execution result
sequenceDiagram
participant TS as Test Step
participant SS as saveScreenshot
participant FE as Element Finder
participant IG as Image Generator
TS->>SS: Initiate screenshot action
alt Crop parameter exists
SS->>FE: Get element using selector
FE-->>SS: Return element's bounding rectangle
SS->>SS: Calculate crop dimensions with padding
SS->>IG: Write cropped screenshot file (with retry)
IG-->>SS: Return file save status
else
SS->>IG: Capture full screenshot
IG-->>SS: Return file save status
end
SS-->>TS: Return screenshot action result
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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
🧹 Nitpick comments (8)
src/tests/runCode.js (3)
14-41: Consider concurrency and ephemeral usage for temp script.
Creating unique scripts in the system temp directory could lead to clutter in long-running or parallel processes. Consider using ephemeral or more robust concurrency-safe approaches (e.g., usingmktemputilities or random unique identifiers) to ensure no collisions and simpler cleanup.
110-113: Avoid using thedeleteoperator for performance reasons.
Static analysis flags thedeleteoperator as detrimental to performance. Instead, set the property toundefinedornullto clean up references while preserving object shape.- if (step.code) delete shellStep.code; - if (step.language) delete shellStep.language; - if (step.file) delete shellStep.file; - if (step.group) delete shellStep.group; + if (step.code) shellStep.code = undefined; + if (step.language) shellStep.language = undefined; + if (step.file) shellStep.file = undefined; + if (step.group) shellStep.group = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
140-158: Ensure code execution is properly sandboxed.
runCodeexecutes arbitrary user code. Ifstep.codecomes from untrusted sources, it can lead to potential security risks. Consider sandboxing or restricting commands.src/tests/findElement.js (1)
92-95: Enhanced click action with custom mouse buttons.
Allowing for an optionalbuttonproperty expands interactivity beyond left-click. There is aTODOsuggesting a separate action for configurable click parameters—happy to help if you’d like assistance implementing that feature.test/artifacts/find_rightClick.spec.json (1)
1-21: Add validation step after right-click.The test case successfully implements right-click functionality but lacks verification of the action's result.
Consider adding a verification step:
{ "id": "find_right click", "tests": [ { "steps": [ { "action": "goTo", "url": "https://www.duckduckgo.com" }, { "action": "find", "selector": "#searchbox_input", "click": { "button": "right" } }, + { + "action": "find", + "selector": ".context-menu", + "exists": true + } ] } ] }test/artifacts/saveScreenshot_pixeldiff.spec.json (1)
24-26: Update Screenshot Overwrite Setting:
The"overwrite"property is now set to"byVariation"instead of"true". This change introduces a threshold-based conditional overwrite behavior (in combination with the"maxVariation"parameter). Ensure that the screenshot logic in the implementation correctly interprets the"byVariation"flag, and consider adding inline documentation to clarify this behavior for future maintainers.test/artifacts/runCode.spec.json (1)
1-41: IntroducerunCodeAction Tests:
This new test specification clearly defines test cases for therunCodeaction across JavaScript, Python, and Bash. The structure is logical and verifies that executing code snippets produces the expected output. As the feature evolves, consider adding tests for edge cases (e.g., error handling or multi-line output scenarios) to further strengthen the test coverage..github/workflows/npm-test.yaml (1)
20-28: Upgrade Node Version and GitHub Actions; Remove Trailing Spaces:
The workflow now includes Node.js version 22 and upgrades several GitHub Actions (e.g.,checkout,setup-node, andcache) to v4. These updates should help improve compatibility and performance. However, YAMLlint has flagged trailing spaces (notably on lines 24 and 27).Please remove the extraneous whitespace to comply with YAML standards. For example:
- - 22 + - 22(Ensure no trailing spaces remain on the affected lines.)
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 27-27: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/npm-test.yaml(1 hunks)dev/dev.spec.json(1 hunks)dev/index.js(2 hunks)package.json(3 hunks)src/tests.js(7 hunks)src/tests/findElement.js(1 hunks)src/tests/runCode.js(1 hunks)src/tests/saveScreenshot.js(3 hunks)test/artifacts/config.json(1 hunks)test/artifacts/find_rightClick.spec.json(1 hunks)test/artifacts/runCode.spec.json(1 hunks)test/artifacts/saveScreenshot_pixeldiff.spec.json(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/npm-test.yaml
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 27-27: trailing spaces
(trailing-spaces)
🪛 Biome (1.9.4)
src/tests/runCode.js
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 18)
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (windows-latest, 18)
- GitHub Check: test (ubuntu-latest, 22)
- GitHub Check: test (ubuntu-latest, 20)
- GitHub Check: test (ubuntu-latest, 18)
🔇 Additional comments (13)
src/tests/saveScreenshot.js (3)
57-95: Element cropping logic looks solid.
Verifying that the element plus padding fits in the viewport before scrolling is well-structured. This helps avoid partial or hidden captures.
202-206: File overwrite logic is implemented correctly.
Replacing the existing file with the new screenshot whenoverwrite == "true"follows the intended behavior.
215-242: Aspect ratio check and resizing approach.
The aspect ratio verification and resized comparison logic is neatly done and should help reduce false mismatches due to small dimensional differences.dev/index.js (2)
36-36: LGTM! Input path updated for test specifications.The path has been correctly updated to use the new test specification file.
46-51: LGTM! Browser configuration updated to Firefox.The browser configuration has been properly updated with appropriate options.
src/tests.js (4)
17-17: LGTM! Added runCode module import.The new runCode functionality has been properly imported.
213-243: LGTM! Enhanced viewport size handling.The new setViewportSize function properly handles viewport dimensions by:
- Calculating the difference between desired and current viewport size
- Adjusting window size to accommodate the desired viewport
- Confirming the final viewport size
651-653: LGTM! Added runCode action handler.The runCode action has been properly integrated into the step execution switch statement.
57-57: Verify WebDriver classic enforcement impact.The addition of
"wdio:enforceWebDriverClassic": truefor Firefox, Safari, and Chrome/Edge might affect browser automation behavior.Also applies to: 81-81, 109-109
✅ Verification successful
WebDriver Classic Enforcement Verification
The grep results confirm that"wdio:enforceWebDriverClassic": trueis consistently set in src/tests.js at lines 57, 81, and 109. No additional issues or discussions regarding adverse effects on Firefox, Safari, or Chrome/Edge were found in the repository. Please ensure that your runtime browser automation tests reflect the expected behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any known issues or discussions about WebDriver classic mode # Test: Search for related issues in the repository rg -i "webdriver.*classic|classic.*webdriver"Length of output: 228
dev/dev.spec.json (2)
13-16: LGTM! Screenshot cropping configuration.The crop configuration properly specifies both selector and padding parameters.
17-18: Verify screenshot path handling.Ensure the test artifacts directory exists and is properly handled in the CI environment.
✅ Verification successful
Screenshot Directory Verified
The test artifacts directory (
test/artifacts) is correctly created and writable, so screenshot path handling appears to be properly supported in the CI environment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the test artifacts directory exists and is writable # Test: Verify directory existence and permissions mkdir -p test/artifacts if [ ! -w "test/artifacts" ]; then echo "Warning: test/artifacts directory is not writable" exit 1 fiLength of output: 57
test/artifacts/config.json (1)
16-25: Update Testing Context Configuration:
The testing contexts have been updated from Chrome to Firefox—this change aligns with the overall enhancement in the testing environment. The new configuration appears consistent with the updated driver capabilities and the subsequent alterations in related files.package.json (1)
2-53: Dependency Updates and Version Bump:
The package version has been bumped to"2.19.0", and multiple dependencies as well as optional dependencies have been updated. These changes modernize the project and help align it with the updated testing configurations and browser capabilities.Please ensure thorough regression testing is performed to catch any compatibility issues that might arise from these dependency updates.
| sharp(filePath) | ||
| .extract({ | ||
| left: rect.x, | ||
| top: rect.y, | ||
| width: rect.width, | ||
| height: rect.height, | ||
| }) | ||
| .toFile(croppedPath); | ||
|
|
||
| // Wait for the file to be written | ||
| let retryLimit = 50; | ||
| while (!fs.existsSync(croppedPath)) { | ||
| if (--retryLimit === 0) { | ||
| result.status = "FAIL"; | ||
| result.description = `Couldn't crop image. ${err}`; | ||
| result.description = `Couldn't write cropped image to file.`; | ||
| return result; | ||
| } | ||
| }); | ||
|
|
||
| // Wait for the file to be written | ||
| while (!fs.existsSync(croppedPath)) { | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| } | ||
|
|
||
| // Replace the original file with the cropped file | ||
| fs.renameSync(croppedPath, filePath); | ||
| // Replace the original file with the cropped file | ||
| fs.renameSync(croppedPath, filePath); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await async calls instead of spin-waiting.
Currently, sharp(...).toFile(...) is not awaited, leading to a manual spin wait. This can introduce race conditions and reduce clarity.
- sharp(filePath)
- .extract({
- left: rect.x,
- top: rect.y,
- width: rect.width,
- height: rect.height,
- })
- .toFile(croppedPath);
-
- let retryLimit = 50;
- while (!fs.existsSync(croppedPath)) {
- if (--retryLimit === 0) {
- result.status = "FAIL";
- result.description = `Couldn't write cropped image to file.`;
- return result;
- }
- await new Promise((resolve) => setTimeout(resolve, 500));
- }
+ await sharp(filePath)
+ .extract({
+ left: rect.x,
+ top: rect.y,
+ width: rect.width,
+ height: rect.height,
+ })
+ .toFile(croppedPath);📝 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.
| sharp(filePath) | |
| .extract({ | |
| left: rect.x, | |
| top: rect.y, | |
| width: rect.width, | |
| height: rect.height, | |
| }) | |
| .toFile(croppedPath); | |
| // Wait for the file to be written | |
| let retryLimit = 50; | |
| while (!fs.existsSync(croppedPath)) { | |
| if (--retryLimit === 0) { | |
| result.status = "FAIL"; | |
| result.description = `Couldn't crop image. ${err}`; | |
| result.description = `Couldn't write cropped image to file.`; | |
| return result; | |
| } | |
| }); | |
| // Wait for the file to be written | |
| while (!fs.existsSync(croppedPath)) { | |
| await new Promise((resolve) => setTimeout(resolve, 100)); | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| } | |
| // Replace the original file with the cropped file | |
| fs.renameSync(croppedPath, filePath); | |
| // Replace the original file with the cropped file | |
| fs.renameSync(croppedPath, filePath); | |
| } catch (error) { | |
| await sharp(filePath) | |
| .extract({ | |
| left: rect.x, | |
| top: rect.y, | |
| width: rect.width, | |
| height: rect.height, | |
| }) | |
| .toFile(croppedPath); | |
| // Replace the original file with the cropped file | |
| fs.renameSync(croppedPath, filePath); | |
| } catch (error) { |
Introduce the runCode function for executing temporary scripts in multiple languages. Improve screenshot functionality by ensuring target elements are visible and refining image comparison logic. Update dependencies and enhance driver capabilities for better compatibility.
Summary by CodeRabbit
New Features
Tests
Chores