Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new public API getRunner(options) that starts an Appium server, creates a Chrome WebDriver session with resolved capabilities and window sizing, exposes runStep and cleanup, adds comprehensive tests for getRunner, and bumps a few dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Test / Caller
participant Appium as Appium (process)
participant Caps as Capability Resolver
participant Driver as Chrome WebDriver
participant Cleanup as Cleanup Routine
Caller->>Appium: spawn/start Appium (env/options)
Note right of Appium: process startup & readiness probe
Appium-->>Caller: ready
Caller->>Caps: resolve Chrome capabilities (platform mapping + options)
Caps-->>Caller: capabilities
Caller->>Driver: init WebDriver with capabilities (driverStart)
alt driverStart fails
Caller->>Driver: retry driverStart with headless capability
end
Driver-->>Caller: runner instance
Caller->>Driver: set window size (width × height)
Driver-->>Caller: window size applied
Caller-->>Caller: return { runner, appium, runStep, cleanup }
alt caller finishes
Caller->>Cleanup: call cleanup()
Cleanup->>Driver: deleteSession (if open)
Cleanup->>Appium: stop Appium process
Cleanup-->>Caller: cleanup complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)test/**/*.test.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/**/*.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/tests.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/**/*.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
📚 Learning: 2025-11-26T19:31:27.042ZApplied to files:
🧬 Code graph analysis (1)test/core.test.js (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (7)
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 |
![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 (2)
src/index.js (1)
5-9: getRunner re-export from index.js is wired correctlyImporting
getRunnerfrom./testsand exporting it alongsiderunTestscleanly exposes the new API while keepingrunTestsas the primary entry point.Optionally, consider adding a small test that imports
{ getRunner }from"../src"to exercise the public re-export, since current tests pull it from"../src/tests".test/core.test.js (1)
419-830: Strong getRunner coverage; align timeouts and cleanup patterns, and consider testing the public exportThe new
describe("getRunner() function", ...)block does a thorough job of exercising the new API: default vs. non‑headless, custom and default dimensions, config handling, cleanup behavior (including idempotence and already‑closed sessions), sequential runners, basic navigation, and the presence/type ofrunStep. That’s excellent coverage.A few targeted tweaks will make this suite more robust and better aligned with project conventions:
- Timeout policy
- Here you use
this.timeout(60000), whereas other suites in this file follow the guideline ofthis.timeout(0)for long‑running/browser tests.- Given Appium + browser startup can easily exceed 60s on slower CI (especially Windows), strongly consider switching this to
this.timeout(0)to avoid flakiness, consistent with the documentedtest/**/*.test.jsguideline. As per coding guidelines, tests should use an indefinite timeout.
- Ensure cleanup is always called via
try/finallyMost tests correctly pattern:
let cleanup; try { const result = await getRunner(); cleanup = result.cleanup; // ...assertions... } finally { if (cleanup) await cleanup(); }But a few use
getRunnerwithout guardingcleanupin afinallyblock (e.g., “cleanup function should properly close runner session”, “cleanup function should terminate Appium process”, “cleanup should be idempotent”, “should handle multiple sequential runners”, “cleanup should handle errors gracefully when session already closed”). If an assertion or navigation fails beforecleanup()is reached, Appium may be left running and hang the test process.I’d standardize all
getRunnerusages in this describe to the sametry/finallypattern used in other tests so driver/Appium cleanup is guaranteed even on failure.
- Optionally test the public export
Right now you import
getRunnerfrom"../src/tests". Sincesrc/index.jsnow re‑exportsgetRunner, consider either:
- Importing from
"../src"in this suite, or- Adding a tiny separate test that asserts
require("../src").getRunneris a function.That will help catch accidental regressions in the public API surface.
📜 Review details
Configuration used: Path: .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 (3)
src/index.js(1 hunks)src/tests.js(4 hunks)test/core.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
test/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Tests must use mocha with
this.timeout(0)for indefinite timeout to avoid test failures on long-running operations
Files:
test/core.test.js
test/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use
fs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup
Files:
test/core.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for logging where level is 'debug'|'info'|'warning'|'error', passing config object as first parameter
UsegetAvailableApps()to detect installed browsers rather than hardcoding browser paths, accounting for platform-specific locations
Files:
src/index.jssrc/tests.js
src/index.js
📄 CodeRabbit inference engine (AGENTS.md)
Entry point
src/index.jsmust exposerunTests()function as the primary API for test execution
Files:
src/index.js
src/tests.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests.js: Register new step types insrc/tests.js: add action todriverActionsarray if requires browser, add case inrunStep()switch statement
Configure Appium drivers (chromium,gecko,safarion macOS) with default 10-minute timeout vianewCommandTimeout: 600
Core test runnersrc/tests.jsorchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Files:
src/tests.js
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Core test runner `src/tests.js` orchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Register new step types in `src/tests.js`: add action to `driverActions` array if requires browser, add case in `runStep()` switch statement
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Configure Appium drivers (`chromium`, `gecko`, `safari` on macOS) with default 10-minute timeout via `newCommandTimeout: 600`
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Browser automation via Appium server manages WebDriver sessions for Chrome/Firefox/Safari; auto-started if needed
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/index.js : Entry point `src/index.js` must expose `runTests()` function as the primary API for test execution
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Always handle driver cleanup in try/finally blocks to ensure WebDriver sessions are properly terminated
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Core test runner `src/tests.js` orchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Applied to files:
test/core.test.jssrc/index.jssrc/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Register new step types in `src/tests.js`: add action to `driverActions` array if requires browser, add case in `runStep()` switch statement
Applied to files:
test/core.test.jssrc/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/index.js : Entry point `src/index.js` must expose `runTests()` function as the primary API for test execution
Applied to files:
test/core.test.jssrc/index.jssrc/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Configure Appium drivers (`chromium`, `gecko`, `safari` on macOS) with default 10-minute timeout via `newCommandTimeout: 600`
Applied to files:
test/core.test.jssrc/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Tests follow nested hierarchy: spec (file) → test → context (browser/platform combo) → step (action), with contexts running serially and skipping if unsupported
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/*.js : Create new step type handlers in `src/tests/[actionName].js` exporting async functions that validate schema, resolve to object, set defaults, and return `{ status: 'PASS'|'FAIL'|'WARNING', description: string, outputs: {} }`
Applied to files:
test/core.test.jssrc/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Always handle driver cleanup in try/finally blocks to ensure WebDriver sessions are properly terminated
Applied to files:
test/core.test.jssrc/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Use `setViewportSize()` to set inner viewport dimensions, which calculates delta from window size rather than setting window size directly
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Each step type has dedicated handler module in `src/tests/` (e.g., `httpRequest.js`, `runShell.js`) implementing specific action logic
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to test/**/*.test.js : Tests must use mocha with `this.timeout(0)` for indefinite timeout to avoid test failures on long-running operations
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Set step variables via `step.variables = { MY_VAR: '$$response.body.token' }` to store runtime values as environment variables for subsequent steps
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Input resolution via `doc-detective-resolver` detects tests from docs/specs and resolves to executable format as first step in test execution flow
Applied to files:
src/index.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Run local test suite with `npm test` (mocha), use `node dev` for development/manual testing, run `npm run depcheck` to check unused dependencies
Applied to files:
src/index.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Test & Publish workflow runs cross-platform tests (Ubuntu/Windows/macOS × Node 18/20/22/24) before publishing; triggers downstream `doc-detective` update via repository dispatch
Applied to files:
src/index.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/**/*.js : Use `getAvailableApps()` to detect installed browsers rather than hardcoding browser paths, accounting for platform-specific locations
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Browser automation via Appium server manages WebDriver sessions for Chrome/Firefox/Safari; auto-started if needed
Applied to files:
src/tests.js
🧬 Code graph analysis (1)
test/core.test.js (1)
src/tests.js (6)
cleanup(1039-1054)width(972-972)height(973-973)runner(1018-1018)appium(373-373)appium(996-1000)
🔇 Additional comments (1)
src/tests.js (1)
795-803: runStep defaults and recording guard improve robustnessDefaulting
config,context, andmetaValuesplus switching toconfig?.recordingmakesrunStepsafer to call from external contexts (like the newgetRunnerAPI) without requiring callers to always construct full objects. This helps avoid runtime errors whenconfigormetaValuesare omitted, while preserving existing behavior.No changes requested here.
Also applies to: 881-886
|
📝 Documentation updates detected! New suggestion: Document getRunner() programmatic API |
- Use getEnvironment() to build an environment object and include it in config and runnerDetails instead of recreating platform manually - Update JSDoc and example to refer to "runner" (WebDriver) instead of "driver" - Clarify appium termination guidance and add runStep to the documented returned properties
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/tests.js (2)
795-906: runStep defaults and recording hook look good; consider guarding driver accessThe new defaults for
config,context, andmetaValuesmakerunStepsafer to call from external code, and the recording block that instantiates the cursor on URL change matches the existing runner behaviour.For extra robustness with external callers, you could defensively guard against missing or foreign drivers and ensure
driver.stateexists before use:- if (config?.recording) { - const currentUrl = await driver.getUrl(); - if (currentUrl !== driver.state.url) { - driver.state.url = currentUrl; - await instantiateCursor(driver); - } - } + if (config?.recording && driver) { + if (!driver.state) { + driver.state = { url: "", x: null, y: null }; + } + const currentUrl = await driver.getUrl(); + if (currentUrl !== driver.state.url) { + driver.state.url = currentUrl; + await instantiateCursor(driver); + } + }This keeps current behaviour intact while avoiding surprises if
runStepis used with a non‑Doc‑Detective driver instance or without a driver.
32-32: getRunner lifecycle and API look solid; consider returning a pre-bound step runner helperThe new
getRunnerexport cleanly reuses existing primitives (getEnvironment(),getAvailableApps(),setAppiumHome(),appiumIsReady(),driverStart()) and:
- Validates that Chrome is available up front.
- Starts Appium, waits until it’s ready, and logs readiness.
- Starts a Chrome WebDriver with the same capability conventions as
runSpecs.- Applies window sizing with a best-effort
setWindowSize.- Exposes a
cleanupthat tears down both the driver session and Appium and is effectively idempotent.One ergonomic improvement you might consider for external consumers:
- Return a pre-bound step runner that already wires in
configandrunner, so callers don’t have to repeatedly pass them torunStep:// Create cleanup function const cleanup = async () => { @@ if (appium) { kill(appium.pid); } }; - return { runner, appium, cleanup, runStep }; + const run = (args) => + runStep({ driver: runner, config, metaValues: {}, ...args }); + + return { runner, appium, cleanup, runStep, run };This keeps the low-level
runStepavailable while giving most users a simpler, less error-prone entry point tied to therunnercreated bygetRunner.Also applies to: 940-1059
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tests.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/tests.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests.js: Register new step types insrc/tests.js: add action todriverActionsarray if requires browser, add case inrunStep()switch statement
Configure Appium drivers (chromium,gecko,safarion macOS) with default 10-minute timeout vianewCommandTimeout: 600
Core test runnersrc/tests.jsorchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Files:
src/tests.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for logging where level is 'debug'|'info'|'warning'|'error', passing config object as first parameter
UsegetAvailableApps()to detect installed browsers rather than hardcoding browser paths, accounting for platform-specific locations
Files:
src/tests.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Core test runner `src/tests.js` orchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Configure Appium drivers (`chromium`, `gecko`, `safari` on macOS) with default 10-minute timeout via `newCommandTimeout: 600`
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Browser automation via Appium server manages WebDriver sessions for Chrome/Firefox/Safari; auto-started if needed
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Register new step types in `src/tests.js`: add action to `driverActions` array if requires browser, add case in `runStep()` switch statement
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/index.js : Entry point `src/index.js` must expose `runTests()` function as the primary API for test execution
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Core test runner `src/tests.js` orchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Configure Appium drivers (`chromium`, `gecko`, `safari` on macOS) with default 10-minute timeout via `newCommandTimeout: 600`
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Register new step types in `src/tests.js`: add action to `driverActions` array if requires browser, add case in `runStep()` switch statement
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/index.js : Entry point `src/index.js` must expose `runTests()` function as the primary API for test execution
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Always handle driver cleanup in try/finally blocks to ensure WebDriver sessions are properly terminated
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/*.js : Create new step type handlers in `src/tests/[actionName].js` exporting async functions that validate schema, resolve to object, set defaults, and return `{ status: 'PASS'|'FAIL'|'WARNING', description: string, outputs: {} }`
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Browser automation via Appium server manages WebDriver sessions for Chrome/Firefox/Safari; auto-started if needed
Applied to files:
src/tests.js
🧬 Code graph analysis (1)
src/tests.js (2)
src/config.js (4)
environment(296-296)chrome(320-322)path(5-5)getAvailableApps(305-378)src/appium.js (2)
path(1-1)setAppiumHome(6-23)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/tests.js (2)
940-944: Consider removing duplicateplatformMapand reusinggetEnvironment().This
platformMapduplicates the one insrc/config.js(lines 15-19). SincegetRunneralready callsgetEnvironment()at line 972, the map is unused—environmentis derived fromgetEnvironment()directly.The
platformMapdeclaration can be safely removed as it's not referenced anywhere ingetRunner():-const platformMap = { - win32: "windows", - darwin: "mac", - linux: "linux", -}; -
1040-1056: Consider makingcleanupidempotent by nullifying references.The cleanup function may attempt
runner.deleteSession()andkill(appium.pid)multiple times if called repeatedly. Whiletree-killtypically handles already-dead PIDs gracefully, the tests at lines 775-793 rely on idempotent behavior. Nullifying references after cleanup would make this explicit and prevent redundant operations.const cleanup = async () => { + if (!runner && !appium) return; // Already cleaned up try { if (runner) { await runner.deleteSession(); + runner = null; } } catch (error) { log( config, "error", `Failed to delete runner session: ${error.message}` ); + runner = null; // Mark as cleaned even on error } if (appium) { kill(appium.pid); + appium = null; } };Note: This requires changing
consttoletforrunnerandappiumdeclarations.
📜 Review details
Configuration used: Path: .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 (3)
package.json(2 hunks)src/tests.js(4 hunks)test/core.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/tests.js
📄 CodeRabbit inference engine (AGENTS.md)
src/tests.js: Register new step types insrc/tests.js: add action todriverActionsarray if requires browser, add case inrunStep()switch statement
Configure Appium drivers (chromium,gecko,safarion macOS) with default 10-minute timeout vianewCommandTimeout: 600
Core test runnersrc/tests.jsorchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Files:
src/tests.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Uselog(config, level, message)for logging where level is 'debug'|'info'|'warning'|'error', passing config object as first parameter
UsegetAvailableApps()to detect installed browsers rather than hardcoding browser paths, accounting for platform-specific locations
Files:
src/tests.js
test/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Tests must use mocha with
this.timeout(0)for indefinite timeout to avoid test failures on long-running operations
Files:
test/core.test.js
test/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use
fs.writeFileSync()+fs.unlinkSync()in try/finally blocks for temporary test files to ensure cleanup
Files:
test/core.test.js
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Core test runner `src/tests.js` orchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Browser automation via Appium server manages WebDriver sessions for Chrome/Firefox/Safari; auto-started if needed
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Register new step types in `src/tests.js`: add action to `driverActions` array if requires browser, add case in `runStep()` switch statement
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Configure Appium drivers (`chromium`, `gecko`, `safari` on macOS) with default 10-minute timeout via `newCommandTimeout: 600`
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/config.js : Validate configuration objects via `doc-detective-common` schemas using `validate({ schemaKey: 'config_v3', object })`
Applied to files:
package.json
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Run local test suite with `npm test` (mocha), use `node dev` for development/manual testing, run `npm run depcheck` to check unused dependencies
Applied to files:
package.json
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Core test runner `src/tests.js` orchestrates Appium/WebDriver sessions and manages spec → test → context → step hierarchy (600+ LOC orchestrator)
Applied to files:
src/tests.jstest/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Register new step types in `src/tests.js`: add action to `driverActions` array if requires browser, add case in `runStep()` switch statement
Applied to files:
src/tests.jstest/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests.js : Configure Appium drivers (`chromium`, `gecko`, `safari` on macOS) with default 10-minute timeout via `newCommandTimeout: 600`
Applied to files:
src/tests.jstest/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/index.js : Entry point `src/index.js` must expose `runTests()` function as the primary API for test execution
Applied to files:
src/tests.jstest/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Always handle driver cleanup in try/finally blocks to ensure WebDriver sessions are properly terminated
Applied to files:
src/tests.jstest/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/*.js : Create new step type handlers in `src/tests/[actionName].js` exporting async functions that validate schema, resolve to object, set defaults, and return `{ status: 'PASS'|'FAIL'|'WARNING', description: string, outputs: {} }`
Applied to files:
src/tests.jstest/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Browser automation via Appium server manages WebDriver sessions for Chrome/Firefox/Safari; auto-started if needed
Applied to files:
src/tests.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Tests follow nested hierarchy: spec (file) → test → context (browser/platform combo) → step (action), with contexts running serially and skipping if unsupported
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Each step type has dedicated handler module in `src/tests/` (e.g., `httpRequest.js`, `runShell.js`) implementing specific action logic
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to test/**/*.test.js : Tests must use mocha with `this.timeout(0)` for indefinite timeout to avoid test failures on long-running operations
Applied to files:
test/core.test.js
📚 Learning: 2025-11-26T19:31:27.042Z
Learnt from: CR
Repo: doc-detective/core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T19:31:27.042Z
Learning: Applies to src/tests/**/*.js : Set step variables via `step.variables = { MY_VAR: '$$response.body.token' }` to store runtime values as environment variables for subsequent steps
Applied to files:
test/core.test.js
🧬 Code graph analysis (2)
src/tests.js (1)
src/config.js (3)
platformMap(16-20)environment(296-296)chrome(320-322)
test/core.test.js (1)
src/tests.js (5)
width(974-974)height(975-975)runner(1020-1020)appium(373-373)appium(998-1002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Agent
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (windows-latest, 24)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (ubuntu-latest, 24)
- GitHub Check: test (ubuntu-latest, 20)
- GitHub Check: test (ubuntu-latest, 22)
🔇 Additional comments (6)
package.json (1)
30-30: LGTM!Routine dependency version bumps (patch/minor) that align with the new
getRunner()functionality. Thewebdriveriominor update may include relevant improvements for the browser automation features.Also applies to: 47-47, 49-49
src/tests.js (3)
796-803: LGTM on default parameter additions.Adding default values for
config,context, andmetaValuesenablesrunStepto be safely called from the publicgetRunner()API without requiring full internal context. The optional chaining onconfig?.recordingat line 880 complements this well.
946-970: LGTM on JSDoc and return structure.The JSDoc accurately documents the return shape
{ runner, appium, cleanup, runStep }with a correct usage example. The note about preferringcleanup()over directly killingappiumis helpful for API consumers.Also applies to: 1058-1058
32-32: LGTM!The export follows the existing pattern used for
runSpecsandrunViaApi.test/core.test.js (2)
424-449: LGTM on test structure and cleanup handling.The tests correctly use
try/finallyblocks to ensure cleanup even on failures, following the coding guideline to "handle driver cleanup in try/finally blocks." Tests that omittry/finally(e.g., lines 552-569) are intentionally testing cleanup behavior itself.
419-825: Comprehensive test coverage forgetRunner()API.The test suite thoroughly covers the API surface including:
- Default options and headless modes
- Custom window dimensions with tolerance checks
- Config object handling
- Cleanup robustness and idempotency
- Sequential runner usage
- State object initialization
The skipped tests for mocking scenarios (lines 545-550, 647-652) are appropriately marked, acknowledging the need for mocking infrastructure.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a public getRunner() function that allows external libraries to programmatically create and manage Chrome WebDriver instances with integrated Appium support. The function provides automated setup of the browser environment, configurable window sizing, and proper cleanup mechanisms for resource management.
Key Changes
- Exposed
getRunner()as a public API function that creates pre-configured Chrome WebDriver instances - Added default parameter values to
runStep()function for safer external usage - Comprehensive test suite covering various configurations, edge cases, and cleanup behavior
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests.js | Implements getRunner() function with Chrome driver initialization, Appium server management, and cleanup; adds default parameters to runStep() for safer external usage; includes unused platformMap constant |
| src/index.js | Exports getRunner() function to make it available as part of the public API |
| test/core.test.js | Adds comprehensive test suite for getRunner() covering default options, headless/non-headless modes, custom dimensions, cleanup behavior, navigation, and edge cases; contains one spelling error ("compelte") |
| package.json | Updates dependency versions: @apidevtools/json-schema-ref-parser (15.1.2→15.1.3), posthog-node (5.14.0→5.14.1), and webdriverio (9.20.1→9.21.0) |
Comments suppressed due to low confidence (1)
src/tests.js:940
- Unused variable platformMap.
const platformMap = {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Introduce the
getRunner()function for creating a Chrome WebDriver instance with an Appium server, enhancing automation capabilities. Additionally, establish unit tests to ensure functionality and reliability.Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.