Skip to content

Enhance Appium integration and screenshot handling #424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hawkeyexl wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Enhance Appium integration and screenshot handling #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
858e55b
Enhance Appium integration and testing
hawkeyexl Jan 25, 2026
2ea99e9
Clamp screenshot coordinates to viewport bounds and ensure minimum di…
hawkeyexl Jan 25, 2026
9451ff5
Fix test failures from incorrect parameter and outdated selectors (#425)
Copilot Jan 26, 2026
e007ce6
Add navigation test for local server using runStep
hawkeyexl Jan 26, 2026
c60dbd9
Enhance port availability checks and error reporting
hawkeyexl Jan 26, 2026
c779ea7
Revert "Enhance port availability checks and error reporting"
hawkeyexl Jan 26, 2026
39be475
Address CodeRabbit review comments: improve port error handling, add …
hawkeyexl Jan 26, 2026
6ad9590
Fix CodeRabbit comments: axios timeout overshoot, dual-host pre-check…
hawkeyexl Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 203 additions & 46 deletions src/tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const kill = require("tree-kill");
const wdio = require("webdriverio");
const os = require("os");
const net = require("net");
const { log, replaceEnvs } = require("./utils");
const axios = require("axios");
const { instantiateCursor } = require("./tests/moveTo");
Expand Down Expand Up @@ -31,6 +32,7 @@ const { uploadChangedFiles } = require("./integrations");
exports.runSpecs = runSpecs;
exports.runViaApi = runViaApi;
exports.getRunner = getRunner;
exports.checkPortAvailable = checkPortAvailable;
// exports.appiumStart = appiumStart;
// exports.appiumIsReady = appiumIsReady;
// exports.driverStart = driverStart;
Expand Down Expand Up @@ -380,6 +382,7 @@ async function runSpecs({ resolvedTests }) {
const availableApps = runnerDetails.availableApps;
const metaValues = { specs: {} };
let appium;
let appiumHost;
const report = {
summary: {
specs: {
Expand Down Expand Up @@ -413,24 +416,52 @@ async function runSpecs({ resolvedTests }) {
// Determine which apps are required
const appiumRequired = isAppiumRequired(specs);

// Warm up Appium
if (appiumRequired) {
// Set Appium home directory
setAppiumHome();
// Start Appium server
appium = spawn("npx", ["appium"], {
shell: true,
windowsHide: true,
cwd: path.join(__dirname, ".."),
});
// Warm up Appium
if (appiumRequired) {
// Check port availability before spawning (probe both hosts like appiumIsReady does)
const hosts = ["127.0.0.1", "localhost"];
let portAvailableOnAnyHost = false;
const hostStatuses = {};

for (const host of hosts) {
const isAvailable = await checkPortAvailable(4723, host);
hostStatuses[host] = isAvailable;
if (isAvailable) {
portAvailableOnAnyHost = true;
}
}

if (!portAvailableOnAnyHost) {
let message = "Appium port 4723 is already in use. Stop the process using it or set a different port.\n";
message += "Port availability check results:\n";
for (const host of hosts) {
message += ` ${host}: ${hostStatuses[host] ? "available" : "unavailable"}\n`;
}
log(config, "error", message);
throw new Error(message);
}
// Set Appium home directory
setAppiumHome();
// Start Appium server
appium = spawn("npx", ["appium"], {
shell: true,
windowsHide: true,
cwd: path.join(__dirname, ".."),
});
appium.stdout.on("data", (data) => {
// console.log(`stdout: ${data}`);
});
appium.stderr.on("data", (data) => {
// console.error(`stderr: ${data}`);
});
await appiumIsReady();
log(config, "debug", "Appium is ready.");
try {
appiumHost = await appiumIsReady();
log(config, "debug", `Appium is ready on ${appiumHost}.`);
} catch (error) {
// Clean up Appium process on timeout/failure
kill(appium.pid);
throw error;
}
}

// Iterate specs
Expand Down Expand Up @@ -541,7 +572,7 @@ async function runSpecs({ resolvedTests }) {

// Instantiate driver
try {
driver = await driverStart(caps);
driver = await driverStart(caps, appiumHost);
} catch (error) {
try {
// If driver fails to start, try again as headless
Expand All @@ -552,15 +583,15 @@ async function runSpecs({ resolvedTests }) {
);
context.browser.headless = true;
caps = getDriverCapabilities({
config: config,
runnerDetails: runnerDetails,
name: context.browser.name,
options: {
width: context.browser?.window?.width || 1200,
height: context.browser?.window?.height || 800,
headless: context.browser?.headless !== false,
},
});
driver = await driverStart(caps);
driver = await driverStart(caps, appiumHost);
} catch (error) {
let errorMessage = `Failed to start context '${context.browser?.name}' on '${platform}'.`;
if (context.browser?.name === "safari")
Expand Down Expand Up @@ -939,36 +970,155 @@ async function runStep({
return actionResult;
}

// Delay execution until Appium server is available.
async function appiumIsReady() {
let isReady = false;
while (!isReady) {
// Retry delay
// TODO: Add configurable retry delay
// TODO: Add configurable timeout duration
await new Promise((resolve) => setTimeout(resolve, 1000));
/**
* Check if a port is available (not in use).
* @param {number} port - Port number to check
* @param {string} host - Host to check on (default: 127.0.0.1)
* @returns {Promise<boolean>} - True if port is available, false if in use
*/
async function checkPortAvailable(port, host = "127.0.0.1") {
return new Promise((resolve) => {
const server = net.createServer();
server.once("error", (err) => {
if (["EADDRINUSE", "EACCES", "EADDRNOTAVAIL"].includes(err.code)) {
resolve(false); // Port is not available
} else {
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
} else {
} else {
server.close();

Copilot uses AI. Check for mistakes.
resolve(true); // Unknown error, assume available
}
});
server.once("listening", () => {
server.close();
resolve(true); // Port is available
});
server.listen(port, host);
});
}

/**
* Delay execution until Appium server is available.
* Uses 127.0.0.1 with localhost fallback for cross-platform compatibility.
* @param {number} timeoutMs - Maximum time to wait in milliseconds (default: 60000)
* @returns {Promise<string>} - The hostname that worked (127.0.0.1 or localhost)
* @throws {Error} - If Appium fails to start within timeout
*/
async function appiumIsReady(timeoutMs = 60000) {
const startTime = Date.now();
const deadline = startTime + timeoutMs;
const hosts = ["127.0.0.1", "localhost"];
let lastError = null;
let successHost = null;
const lastErrorPerHost = {};

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));
Comment on lines +1011 to +1018
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;

Copilot uses AI. Check for mistakes.

for (const host of hosts) {
const hostRemaining = deadline - Date.now();
if (hostRemaining <= 0) break;

try {
const resp = await axios.get(`http://${host}:4723/status`, {
timeout: Math.min(5000, hostRemaining), // Clamp to remaining time, no minimum
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Suggested change
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

Copilot uses AI. Check for mistakes.
});
Comment on lines 1024 to 1027
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

if (resp.status === 200) {
successHost = host;
return successHost;
}
} catch (err) {
lastError = err;
lastErrorPerHost[host] = err;
// Continue to next host or retry
}
}
}

// Timeout reached - build descriptive error message
const elapsed = Date.now() - startTime;
const platform = process.platform;
const remaining = Math.max(0, deadline - Date.now());

let errorMsg = `Appium failed to start within ${Math.round(elapsed / 1000)} seconds.\n`;
errorMsg += `Platform: ${platform}\n`;

// Check port availability with clamped timeout if time remains
let portAvailable = null;
if (remaining > 0) {
const checkDeadline = Date.now() + Math.min(1000, remaining);
try {
let resp = await axios.get("http://0.0.0.0:4723/status");
if (resp.status === 200) isReady = true;
} catch {}
portAvailable = await Promise.race([
checkPortAvailable(4723),
new Promise((_, reject) =>
setTimeout(() => reject(new Error("Port check timeout")), checkDeadline - Date.now())
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
setTimeout(() => reject(new Error("Port check timeout")), checkDeadline - Date.now())
setTimeout(
() => reject(new Error("Port check timeout")),
Math.max(0, checkDeadline - Date.now())
)

Copilot uses AI. Check for mistakes.
),
]);
} catch {
portAvailable = null; // Couldn't determine port status
}
}

if (portAvailable !== null) {
errorMsg += `Port 4723 status: ${portAvailable ? "available (not bound)" : "in use (bound)"}\n`;
}

// Include per-host diagnostics
for (const host of hosts) {
if (lastErrorPerHost[host]) {
errorMsg += `${host} connection error: ${lastErrorPerHost[host].message}\n`;
}
}

if (lastError) {
errorMsg += `Last connection error: ${lastError.message}\n`;
}

if (platform === "win32") {
errorMsg += "\nWindows troubleshooting:\n";
errorMsg += "- Check Windows Firewall settings for port 4723\n";
errorMsg += "- Temporarily disable antivirus software\n";
errorMsg += "- Run as Administrator if port binding fails\n";
} else if (platform === "darwin") {
errorMsg += "\nmacOS troubleshooting:\n";
errorMsg += "- Check System Preferences > Security & Privacy > Firewall\n";
errorMsg += "- Ensure no VPN is blocking localhost connections\n";
}
return isReady;

throw new Error(errorMsg);
}

// Start the Appium driver specified in `capabilities`.
async function driverStart(capabilities) {
const driver = await wdio.remote({
protocol: "http",
hostname: "0.0.0.0",
port: 4723,
path: "/",
logLevel: "error",
capabilities,
connectionRetryTimeout: 600000, // 10 minutes
waitforTimeout: 600000, // 10 minutes
});
driver.state = { url: "", x: null, y: null };
return driver;
// Uses hostname determined by appiumIsReady() for cross-platform compatibility.
async function driverStart(capabilities, hostname = "127.0.0.1") {
const hosts = hostname ? [hostname] : ["127.0.0.1", "localhost"];
let lastError = null;

for (const host of hosts) {
try {
const driver = await wdio.remote({
protocol: "http",
hostname: host,
port: 4723,
path: "/",
logLevel: "error",
capabilities,
connectionRetryTimeout: 60000, // 60 seconds (reduced from 10 minutes)
waitforTimeout: 60000, // 60 seconds (reduced from 10 minutes)
});
driver.state = { url: "", x: null, y: null };
return driver;
} catch (err) {
lastError = err;
// Try next hostname
}
}

throw new Error(
`Failed to connect to Appium driver on hosts [${hosts.join(", ")}]: ${lastError?.message || "Unknown error"}`
);
}

/**
Expand Down Expand Up @@ -1029,9 +1179,16 @@ async function getRunner(options = {}) {
cwd: path.join(__dirname, ".."),
});

// Wait for Appium to be ready
await appiumIsReady();
log(config, "debug", "Appium is ready for external driver.");
// Wait for Appium to be ready and get the working hostname
let appiumHost;
try {
appiumHost = await appiumIsReady();
log(config, "debug", `Appium is ready on ${appiumHost} for external driver.`);
} catch (error) {
// Clean up Appium process on timeout/failure
kill(appium.pid);
throw error;
}

// Get Chrome driver capabilities
const caps = getDriverCapabilities({
Expand All @@ -1044,10 +1201,10 @@ async function getRunner(options = {}) {
},
});

// Start the runner
// Start the runner using the hostname that worked for Appium status check
let runner;
try {
runner = await driverStart(caps);
runner = await driverStart(caps, appiumHost);
} catch (error) {
// If runner fails, attempt to set headless and retry
try {
Expand All @@ -1057,7 +1214,7 @@ async function getRunner(options = {}) {
"Failed to start Chrome runner. Retrying as headless."
);
caps["goog:chromeOptions"].args.push("--headless", "--disable-gpu");
runner = await driverStart(caps);
runner = await driverStart(caps, appiumHost);
} catch (error) {
// If runner fails, clean up Appium and rethrow
kill(appium.pid);
Expand Down
26 changes: 26 additions & 0 deletions src/tests/saveScreenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,32 @@ async function saveScreenshot({ config, step, driver }) {
rect.width += padding.left + padding.right;
rect.height += padding.top + padding.bottom;

// Get viewport dimensions for clamping
const viewport = await driver.execute(() => ({
width: window.innerWidth,
height: window.innerHeight,
}));

// Clamp coordinates to viewport bounds (prevent negative values and overflow)
if (rect.x < 0) {
rect.width += rect.x; // Reduce width by the amount we're clamping
rect.x = 0;
}
if (rect.y < 0) {
rect.height += rect.y; // Reduce height by the amount we're clamping
rect.y = 0;
}
if (rect.x + rect.width > viewport.width) {
rect.width = viewport.width - rect.x;
}
if (rect.y + rect.height > viewport.height) {
rect.height = viewport.height - rect.y;
}

Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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."
);
}

Copilot uses AI. Check for mistakes.
// Ensure width and height are at least 1 pixel
rect.width = Math.max(1, rect.width);
rect.height = Math.max(1, rect.height);

// Scale the values based on the pixel density
rect.x *= pixelDensity;
rect.y *= pixelDensity;
Expand Down
2 changes: 1 addition & 1 deletion test/artifacts/find_rightClick.spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
},
{
"action": "find",
"selector": "#searchbox_input",
"selector": "input[name=\"q\"]",
"click": {
"button": "right"
}
Expand Down
Loading
Loading