Conversation
- Add a function to check if a port is available before starting Appium. - Modify appiumIsReady to provide detailed error messages and troubleshooting tips. - Update driverStart to use the hostname determined by appiumIsReady for better compatibility. - Refactor runSpecs and getRunner to handle the new appiumHost parameter. - Implement a test to ensure getRunner initializes within 60 seconds across platforms. - Validate that descriptive errors are thrown if Appium fails to start within the timeout.
…mensions - Add viewport dimension retrieval for clamping - Implement clamping logic to prevent negative values and overflow - Ensure width and height are at least 1 pixel - Introduce screenshot cleanup helper in core tests - Clean up screenshot files before and after running tests
WalkthroughAdds port probing and host-aware Appium readiness (multi-host checks, diagnostics, timeouts, appiumHost propagation), exposes Changes
Sequence DiagramsequenceDiagram
participant Runner as Test Runner
participant PortCheck as checkPortAvailable
participant AppCheck as appiumIsReady
participant Appium as Appium Server
participant Driver as WebDriver
Runner->>AppCheck: appiumIsReady(timeoutMs)
AppCheck->>PortCheck: checkPortAvailable(4723, "127.0.0.1")
PortCheck->>Appium: probe port
alt open on 127.0.0.1
Appium-->>PortCheck: respond
PortCheck-->>AppCheck: success ("127.0.0.1")
AppCheck-->>Runner: return host "127.0.0.1"
else probe failed
AppCheck->>PortCheck: checkPortAvailable(4723, "localhost")
PortCheck->>Appium: probe port
alt open on localhost
Appium-->>PortCheck: respond
PortCheck-->>AppCheck: success ("localhost")
AppCheck-->>Runner: return host "localhost"
else timeout/error
AppCheck-->>Runner: throw timeout/error (with diagnostics)
Runner->>Appium: kill(appium.pid)
end
end
Runner->>Driver: driverStart(capabilities, appiumHost)
Driver->>Appium: connect to appiumHost
Appium-->>Driver: ready
Driver-->>Runner: driver instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
📝 Documentation updates detected! New suggestion: Document screenshot padding viewport clamping behavior |
![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
🤖 Fix all issues with AI agents
In `@test/core.test.js`:
- Around line 859-894: Remove the explicit Mocha timeout call
(this.timeout(70000)) and keep the suite-level this.timeout(0); instead wrap the
getRunner() call in an explicit timeout via Promise.race (or a helper like
withTimeout) so the test enforces a 60s startup limit without changing Mocha's
timeout policy; call getRunner() inside the timed promise, preserve assigning
cleanup = result.cleanup and the finally cleanup invocation, and make the timed
rejection produce a descriptive error if the timeout elapses so assertions still
communicate that getRunner() exceeded 60000ms.
🧹 Nitpick comments (1)
src/tests.js (1)
957-971: Treat permission/binding errors as “not available.”
Right now, any non-EADDRINUSEerror resolves as available, which can mislead diagnostics (e.g.,EACCES,EADDRNOTAVAIL). Consider marking those as unavailable too.🔧 Suggested tweak
- server.once("error", (err) => { - if (err.code === "EADDRINUSE") { - resolve(false); // Port is in use - } else { - resolve(true); // Other error, assume available - } - }); + server.once("error", (err) => { + if (["EADDRINUSE", "EACCES", "EADDRNOTAVAIL"].includes(err.code)) { + resolve(false); // Port is not available + } else { + resolve(true); // Unknown error, assume available + } + });
|
@copilot identify and fix the |
|
@hawkeyexl I've opened a new pull request, #425, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Initial investigation - identified bug in getDriverCapabilities call Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com> * Fix test failures - update DuckDuckGo selectors and getDriverCapabilities bug Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com> * Update DuckDuckGo selectors to input[name="q"] for compatibility Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests.js (1)
419-442: Add port availability pre-check before spawning Appium to fail fast.Appium is spawned at line 424 without checking if port 4723 is already bound. If the port is in use, the code waits the full
appiumIsReady()timeout before discovering the problem. UsecheckPortAvailable()before spawning to exit immediately if the port is unavailable.Example implementation
if (appiumRequired) { + const portFree = await checkPortAvailable(4723, "127.0.0.1"); + if (!portFree) { + const message = "Appium port 4723 is already in use. Stop the process using it or set a different port."; + log(config, "error", message); + throw new Error(message); + } // Set Appium home directory setAppiumHome(); // Start Appium server appium = spawn("npx", ["appium"], {
🤖 Fix all issues with AI agents
In `@src/tests.js`:
- Around line 957-1033: The checkPortAvailable function incorrectly treats
non-EADDRINUSE errors as "available" and appiumIsReady only queries port status
for 127.0.0.1; update checkPortAvailable to treat any server "error" event as
port-unavailable (resolve false) except when the error is specifically a
transient connection refusal that truly indicates free (or better: return both
available boolean and error info), and include the actual error.code/message in
its result; then modify appiumIsReady to call checkPortAvailable for both hosts
in hosts = ["127.0.0.1","localhost"], collect per-host port statuses and last
errors (e.g., lastErrorPerHost), and include those detailed per-host diagnostics
(host, available/in-use, and last connection error) in the final errorMsg before
throwing.
- Implement a test to navigate to the local echo server. - Set a 60s timeout for the navigation test. - Verify the navigation status and page title.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/core.test.js`:
- Around line 888-894: The test currently only asserts that checkPortAvailable
is exported but must exercise the timeout path and verify the descriptive error;
update the spec to stub or mock the readiness check (e.g., appiumIsReady or
checkPortAvailable) to always fail/timeout using sinon/proxyquire or by passing
a deliberately non-responsive config, then call the higher-level startup
function under test and assert that it rejects/throws and that the error message
includes the expected troubleshooting details (mentioning
checkPortAvailable/appiumIsReady and timeout guidance) so the new timeout
messaging is actually validated.
♻️ Duplicate comments (1)
test/core.test.js (1)
861-883: Avoid per‑test Mocha timeouts; enforce 60s via explicit race instead.Line 862 and Line 897 override the suite’s
this.timeout(0)policy. Prefer keeping Mocha indefinite and timing the async work explicitly (also avoids slow-runner flakiness). Apply the same pattern to both tests.As per coding guidelines, keep `test/core.test.js` on `this.timeout(0)`.🛠️ Suggested adjustment (apply similarly to the runStep test)
- it("should initialize within 60 seconds on all platforms", async function () { - this.timeout(70000); // 70s test timeout (allows 60s startup + 10s buffer) - - const startTime = Date.now(); - let cleanup; - - try { - const result = await getRunner(); + it("should initialize within 60 seconds on all platforms", async function () { + const maxStartupMs = 60000; + const startTime = Date.now(); + let cleanup; + + try { + const result = await Promise.race([ + getRunner(), + new Promise((_, reject) => + setTimeout( + () => reject(new Error(`getRunner() timed out after ${maxStartupMs}ms`)), + maxStartupMs + ) + ), + ]); cleanup = result.cleanup; - const elapsed = Date.now() - startTime; - assert.ok(elapsed < 60000, `getRunner() took ${elapsed}ms, should complete within 60000ms`); + const elapsed = Date.now() - startTime; + assert.ok( + elapsed < maxStartupMs, + `getRunner() took ${elapsed}ms, should complete within ${maxStartupMs}ms` + );Also applies to: 896-919
test/core.test.js
Outdated
There was a problem hiding this comment.
Test name says “descriptive error,” but assertion only checks export.
This doesn’t validate the new timeout error messaging. Consider forcing a timeout (e.g., stub appiumIsReady/checkPortAvailable via proxyquire/sinon or pass a deliberately failing config) and asserting the thrown error contains the troubleshooting details.
🤖 Prompt for AI Agents
In `@test/core.test.js` around lines 888 - 894, The test currently only asserts
that checkPortAvailable is exported but must exercise the timeout path and
verify the descriptive error; update the spec to stub or mock the readiness
check (e.g., appiumIsReady or checkPortAvailable) to always fail/timeout using
sinon/proxyquire or by passing a deliberately non-responsive config, then call
the higher-level startup function under test and assert that it rejects/throws
and that the error message includes the expected troubleshooting details
(mentioning checkPortAvailable/appiumIsReady and timeout guidance) so the new
timeout messaging is actually validated.
- Update checkPortAvailable to return detailed availability status and error information. - Modify appiumIsReady to track connection errors per host and provide diagnostics for each host's port status. - Improve error message formatting for better clarity on port availability and connection issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/tests.js`:
- Around line 982-1054: The loop in appiumIsReady can overshoot timeoutMs
because the fixed 1s sleep and per-host axios calls (with 5s timeouts) may
exceed the remaining budget, and post-loop diagnostics via checkPortAvailable
run unbounded; fix by computing a deadline = Date.now() + timeoutMs and before
each await calculate remaining = deadline - Date.now(), then clamp/slice waits
and request timeouts to Math.max(0, remaining) (skip further attempts if
remaining <= 0), pass the clamped timeout to axios.get and to the 1s sleep (use
Math.min(1000, remaining)), and for the post-timeout port diagnostics either
skip them if no remaining time or call checkPortAvailable with a very small
clamped timeout per host so these diagnostics cannot push past the original
deadline.
This reverts commit c60dbd9.
…Appium pre-check, fix timeout budgets, and enhance test timeouts - Treat EACCES and EADDRNOTAVAIL errors as port unavailable in checkPortAvailable - Add port 4723 availability pre-check before spawning Appium server - Fix timeout budget in appiumIsReady loop to prevent exceeding timeoutMs - Clamp axios request timeouts and sleep intervals to remaining deadline - Track per-host connection errors and include in diagnostics - Remove per-test Mocha timeout calls and use Promise.race for explicit timing - Enhance checkPortAvailable test to verify boolean return value - Improve error message formatting and diagnostics for Appium startup failures
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/tests.js`:
- Around line 1009-1012: The axios timeout clamp currently uses Math.min(5000,
Math.max(100, hostRemaining)) which forces a 100ms minimum and can overshoot the
overall deadline; update the try block where axios.get is called so it uses a
timeout of Math.min(5000, hostRemaining) and before making the request
skip/return if hostRemaining <= 0 (or otherwise treat very small remaining
budgets as a skip) to avoid exceeding the deadline; locate and modify the
axios.get call in src/tests.js (the try block that constructs the timeout) to
implement this change.
- Around line 419-427: The pre-check currently calls checkPortAvailable only for
"127.0.0.1" which can cause false negatives; update the warm-up logic (the
appiumRequired branch that calls checkPortAvailable for port 4723) to probe both
"127.0.0.1" and "localhost" (reuse checkPortAvailable) and only treat the port
as in-use if both checks report unavailable, and include diagnostic details
(which host(s) failed and any error messages) in the log via log(config,
"error", ...) so callers can see whether 127.0.0.1 failed while localhost
succeeded; ensure this behavior matches appiumIsReady’s dual-host probing.
In `@test/core.test.js`:
- Around line 866-875: The Promise.race usage that races getRunner() against a
setTimeout creates a timer that isn't cleared when getRunner() resolves; capture
the timeout ID (e.g., const timeoutId = setTimeout(...)) for each Promise.race,
wrap the await in try/finally, and call clearTimeout(timeoutId) in finally to
prevent dangling timers (apply the same pattern to both Promise.race usages
around getRunner(), the timeout promise, and maxStartupMs).
| try { | ||
| const resp = await axios.get(`http://${host}:4723/status`, { | ||
| timeout: Math.min(5000, Math.max(100, hostRemaining)), // Clamp to remaining time | ||
| }); |
There was a problem hiding this comment.
Clamp axios timeout to the remaining budget (avoid minimum 100ms).
Math.max(100, hostRemaining) can overshoot the deadline, which the new tests try to enforce strictly. If the remaining budget is small, prefer using it as-is (or skip the request).
🛠️ Suggested fix
- const resp = await axios.get(`http://${host}:4723/status`, {
- timeout: Math.min(5000, Math.max(100, hostRemaining)), // Clamp to remaining time
- });
+ const resp = await axios.get(`http://${host}:4723/status`, {
+ timeout: Math.min(5000, hostRemaining), // Clamp to remaining time
+ });🤖 Prompt for AI Agents
In `@src/tests.js` around lines 1009 - 1012, The axios timeout clamp currently
uses Math.min(5000, Math.max(100, hostRemaining)) which forces a 100ms minimum
and can overshoot the overall deadline; update the try block where axios.get is
called so it uses a timeout of Math.min(5000, hostRemaining) and before making
the request skip/return if hostRemaining <= 0 (or otherwise treat very small
remaining budgets as a skip) to avoid exceeding the deadline; locate and modify
the axios.get call in src/tests.js (the try block that constructs the timeout)
to implement this change.
…, and dangling timers - Remove Math.max(100) from axios timeout to prevent exceeding deadline - Update axios timeout clamp to use Math.min(5000, hostRemaining) - Check both 127.0.0.1 and localhost for port availability before spawning Appium - Include per-host diagnostic details in pre-check error message - Clear setTimeout timers from Promise.race to prevent dangling timers - Apply timeout cleanup to both getRunner tests
There was a problem hiding this comment.
Pull request overview
This pull request enhances Appium integration by implementing robust port availability checks, cross-platform hostname detection (127.0.0.1 with localhost fallback), improved error messages with platform-specific troubleshooting, and proper cleanup on failures. It also introduces viewport-aware screenshot coordinate clamping to prevent cropping errors, and adds comprehensive test utilities for screenshot file cleanup.
Changes:
- Added port availability checking before Appium startup with detailed diagnostics
- Implemented cross-platform hostname detection and retry logic with enhanced timeout handling
- Added viewport coordinate clamping for screenshot crops to prevent out-of-bounds errors
- Introduced test cleanup helpers and cross-platform compatibility tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests.js | Added checkPortAvailable function, enhanced appiumIsReady with timeout/hostname detection/error messages, updated driverStart to accept hostname parameter, added Appium cleanup on failures, reduced connection timeouts from 10 minutes to 60 seconds |
| src/tests/saveScreenshot.js | Implemented viewport coordinate clamping for screenshot crops to prevent negative coordinates and overflow, ensuring minimum 1-pixel dimensions |
| test/core.test.js | Added screenshot cleanup helper, wrapped screenshot tests in try-finally blocks for cleanup, added cross-platform timeout tests and navigation tests |
| test/artifacts/screenshot.spec.json | Updated selectors from #searchbox_input to input[name="q"] for better element targeting |
| test/artifacts/find_rightClick.spec.json | Updated selector from #searchbox_input to input[name="q"] for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| while (Date.now() < deadline) { | ||
| const remaining = deadline - Date.now(); | ||
| if (remaining <= 0) break; | ||
|
|
||
| // Retry delay - clamp to remaining time | ||
| const sleepMs = Math.min(1000, remaining); | ||
| await new Promise((resolve) => setTimeout(resolve, sleepMs)); |
There was a problem hiding this comment.
The retry loop sleeps before the first connection attempt (line 1018), adding an unnecessary 1-second delay at startup. This means the first connection attempt won't happen until 1 second after the function is called. Consider restructuring to attempt connection first, then sleep only before retries. This would make the function more responsive and reduce the minimum startup time by 1 second.
| while (Date.now() < deadline) { | |
| const remaining = deadline - Date.now(); | |
| if (remaining <= 0) break; | |
| // Retry delay - clamp to remaining time | |
| const sleepMs = Math.min(1000, remaining); | |
| await new Promise((resolve) => setTimeout(resolve, sleepMs)); | |
| let firstAttempt = true; | |
| while (Date.now() < deadline) { | |
| const remaining = deadline - Date.now(); | |
| if (remaining <= 0) break; | |
| // Retry delay - clamp to remaining time (skip before first attempt) | |
| if (!firstAttempt) { | |
| const sleepMs = Math.min(1000, remaining); | |
| await new Promise((resolve) => setTimeout(resolve, sleepMs)); | |
| } | |
| firstAttempt = false; |
| it("should throw descriptive error if Appium fails to start within timeout", async function () { | ||
| // This test validates that timeout mechanism works and provides helpful error messages | ||
| const { checkPortAvailable } = require("../src/tests"); | ||
| assert.ok(typeof checkPortAvailable === "function", "checkPortAvailable should be exported"); | ||
|
|
||
| // Verify the function works - it should return a boolean | ||
| const available = await checkPortAvailable(65432, "127.0.0.1"); // Use unlikely port | ||
| assert.ok(typeof available === "boolean", "checkPortAvailable should return a boolean"); | ||
| }); |
There was a problem hiding this comment.
The test name "should throw descriptive error if Appium fails to start within timeout" is misleading. The test only verifies that checkPortAvailable is exported and returns a boolean. It doesn't actually test the Appium timeout scenario or validate that descriptive error messages are thrown. Consider renaming the test to better reflect what it actually tests, such as "should export checkPortAvailable function that returns boolean", or expand the test to actually validate the timeout behavior.
|
|
||
| try { | ||
| const resp = await axios.get(`http://${host}:4723/status`, { | ||
| timeout: Math.min(5000, hostRemaining), // Clamp to remaining time, no minimum |
There was a problem hiding this comment.
The comment "Clamp to remaining time, no minimum" is slightly unclear. The code uses Math.min(5000, hostRemaining) which clamps the timeout to a maximum of 5 seconds OR the remaining time (whichever is smaller). The phrase "no minimum" might be confusing since there's effectively a minimum of 0 enforced by the remaining time check. Consider clarifying to "Clamp to minimum of 5 seconds and remaining time" or "Use 5 seconds or remaining time, whichever is smaller".
| timeout: Math.min(5000, hostRemaining), // Clamp to remaining time, no minimum | |
| timeout: Math.min(5000, hostRemaining), // Use 5 seconds or remaining time, whichever is smaller |
| portAvailable = await Promise.race([ | ||
| checkPortAvailable(4723), | ||
| new Promise((_, reject) => | ||
| setTimeout(() => reject(new Error("Port check timeout")), checkDeadline - Date.now()) |
There was a problem hiding this comment.
The setTimeout delay calculation checkDeadline - Date.now() can result in a negative or zero value if time elapses between line 1051 and line 1056. A negative timeout will cause setTimeout to fire immediately, which may not cause an error but defeats the purpose of the timeout. Consider using Math.max(0, checkDeadline - Date.now()) to ensure a non-negative timeout value.
| setTimeout(() => reject(new Error("Port check timeout")), checkDeadline - Date.now()) | |
| setTimeout( | |
| () => reject(new Error("Port check timeout")), | |
| Math.max(0, checkDeadline - Date.now()) | |
| ) |
| server.once("error", (err) => { | ||
| if (["EADDRINUSE", "EACCES", "EADDRNOTAVAIL"].includes(err.code)) { | ||
| resolve(false); // Port is not available | ||
| } else { |
There was a problem hiding this comment.
The checkPortAvailable function has a potential resource leak. If an unknown error occurs (line 986), the function resolves with true but never closes the server, which could leave a socket open. Consider adding server.close() before resolving in the error handler for unknown errors, or handle server cleanup more systematically.
| } else { | |
| } else { | |
| server.close(); |
| if (rect.y + rect.height > viewport.height) { | ||
| rect.height = viewport.height - rect.y; | ||
| } | ||
|
|
There was a problem hiding this comment.
The clamping logic may produce invalid dimensions in edge cases. If an element with padding extends significantly beyond the viewport bounds on multiple sides, the sequential clamping operations (lines 247-260) could result in rect.width or rect.height becoming zero or negative before the Math.max(1, ...) calls at lines 263-264. For example, if rect.x = -100, rect.width = 50, after line 248, rect.width = -50. While lines 263-264 ensure minimum 1 pixel, this may not represent the intended crop area. Consider validating that the element is at least partially visible in the viewport before attempting to crop.
| // After clamping, ensure the element is at least partially visible and the rect is valid | |
| const isNonPositiveSize = rect.width <= 0 || rect.height <= 0; | |
| const isCompletelyOutsideViewport = | |
| rect.x >= viewport.width || | |
| rect.y >= viewport.height || | |
| rect.x + rect.width <= 0 || | |
| rect.y + rect.height <= 0; | |
| if (isNonPositiveSize || isCompletelyOutsideViewport) { | |
| throw new Error( | |
| "Cannot capture screenshot: target element is not sufficiently visible within the viewport." | |
| ); | |
| } |
Improve Appium integration by adding port availability checks, enhancing error messages, and ensuring compatibility across platforms. Implement clamping for screenshot coordinates to viewport bounds and introduce a cleanup helper for screenshot files in tests.
Summary by CodeRabbit
New Features
Bug Fixes
Enhancements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.