Add API client and runner functionality#365
Conversation
hawkeyexl
left a comment
hawkeyexl
left a comment
There was a problem hiding this comment.
@coderabbitai review
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to run tests via an external API as an alternative to local execution. When an API key is configured, tests are submitted to a remote service for execution instead of running locally.
Key changes:
- Added
runViaApifunction to handle API-based test execution with polling for results - Modified
runTeststo conditionally route to API or local execution based on configuration - Added API key configuration support through environment variables
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/tests.js | Implements runViaApi function for remote test execution via API with polling mechanism |
| src/index.js | Updates runTests to route execution to API when API key is present in config |
| dev/index.js | Adds API key configuration example using environment variable |
Comments suppressed due to low confidence (1)
src/tests.js:1
- The
configparameter is not passed torunViaApi, but the function signature accepts it asconfig = {}. This means API timeout configuration (config.apiMaxWaitTime) won't be accessible. Pass the config object:results = await runViaApi({ resolvedTests, apiKey: config.integrations.docDetectiveApi.apiKey, config })
const kill = require("tree-kill");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 config wiring for a DocDetective API key, modifies the test runner to accept pre-resolved tests and choose between local or API execution, and implements an API-driven execution path that creates, starts, polls, and retrieves run reports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant R as runTests
participant DR as detectAndResolveTests
participant L as runSpecs (local)
participant A as runViaApi
U->>R: runTests(config, options)
alt options.resolvedTests provided
R->>R: use options.resolvedTests\noverride config with resolvedTests.config
else
R->>DR: detectAndResolveTests(config)
DR-->>R: resolvedTests or none
R->>R: error if none found
end
alt config.integrations.docDetectiveApi.apiKey present
R->>A: runViaApi({resolvedTests, apiKey, config})
A-->>R: results/report or error
else
R->>L: runSpecs(resolvedTests, config)
L-->>R: results/report
end
R->>R: telemetry, logging, cleanup
R-->>U: final results
sequenceDiagram
autonumber
participant A as runViaApi
participant S as DocDetective API
participant P as Poller
A->>S: POST /runs (resolvedTests) {apiKey}
S-->>A: 201 Created {runId}
A->>S: POST /runs/{runId}/start
S-->>A: 202 Accepted
loop Poll until complete or timeout
A->>S: GET /runs/{runId}/status
S-->>A: {status: running|completed|failed}
Note over A,P: randomized interval (jitter), enforce maxWaitTime
end
alt completed
A->>S: GET /runs/{runId}/report
S-->>A: Final report
A-->>Caller: {status: success, report}
else failed/timeout/error
A-->>Caller: {status: error, error}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (12)
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
♻️ Duplicate comments (4)
src/tests.js (4)
260-270: Potential null reference error when accessingerror.response.If
error.responseis undefined (which occurs during network errors like ECONNREFUSED or timeouts before receiving a response), accessingerror.response.statusanderror.response.data.errorwill throw a TypeError.Apply this diff to add defensive checks:
} catch (error) { - return { status: error.response.status, error: error.response.data.error }; + return error.response + ? { status: error.response.status, error: error.response.data?.error || error.message } + : { status: "NETWORK_ERROR", error: error.message }; }
274-287: Potential null reference error when accessingerror.response.If
error.responseis undefined (which occurs during network errors like ECONNREFUSED or timeouts before receiving a response), accessingerror.response.statusanderror.response.data.errorwill throw a TypeError.Apply this diff to add defensive checks:
} catch (error) { - return { status: error.response.status, error: error.response.data.error }; + return error.response + ? { status: error.response.status, error: error.response.data?.error || error.message } + : { status: "NETWORK_ERROR", error: error.message }; }
289-305: Status type inconsistency: mixing string and numeric status codes.The timeout returns
status: "TIMEOUT"(string) while other error paths return numeric HTTP status codes. This inconsistency could cause issues for consumers expecting uniform status types.Consider using a consistent format. Options include:
- Use a numeric code for timeout (e.g.,
408for request timeout)- Wrap all statuses in a structured object with separate
typeandcodefields- Document the mixed types clearly if intentional
307-319: Potential null reference error when accessingerror.response.If
error.responseis undefined (which occurs during network errors like ECONNREFUSED or timeouts before receiving a response), accessingerror.response.statusanderror.response.data.errorwill throw a TypeError.Apply this diff to add defensive checks:
} catch (error) { - return { - status: error.response.status, - error: error.response.data.error, - }; + return error.response + ? { + status: error.response.status, + error: error.response.data?.error || error.message, + } + : { + status: "NETWORK_ERROR", + error: error.message, + }; }
🧹 Nitpick comments (1)
dev/index.js (1)
20-25: Consider adding validation for the API key.The
apiKeyis sourced directly fromprocess.env.KEYwithout validation. If the environment variable is not set,undefinedwill be passed to the configuration, which may lead to unclear error messages downstream.Consider adding a check or fallback:
}], integrations: { docDetectiveApi: { - apiKey: process.env.KEY + apiKey: process.env.KEY // Set KEY environment variable to test API integration } } };Alternatively, add validation before calling
runTests:if (!process.env.KEY) { console.warn("Warning: KEY environment variable not set. API integration will be disabled."); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dev/index.js(1 hunks)src/index.js(2 hunks)src/tests.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
dev/index.js (1)
src/config.js (1)
setConfig(168-258)
src/tests.js (2)
src/tests/httpRequest.js (4)
require(1-1)require(6-6)require(7-7)axios(2-2)src/utils.js (1)
axios(4-4)
src/index.js (1)
src/tests.js (2)
config(357-357)results(335-335)
⏰ 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)
- GitHub Check: test (macos-latest, 20)
- GitHub Check: test (macos-latest, 24)
- GitHub Check: test (macos-latest, 22)
- GitHub Check: test (macos-latest, 18)
- GitHub Check: test (windows-latest, 24)
- GitHub Check: test (windows-latest, 18)
- GitHub Check: test (windows-latest, 22)
- GitHub Check: test (windows-latest, 20)
- GitHub Check: test (ubuntu-latest, 22)
🔇 Additional comments (9)
src/tests.js (5)
5-5: LGTM!The axios import is correctly placed and necessary for the new API integration functionality.
31-31: LGTM!The
runViaApiexport properly exposes the new API-based test execution pathway for external use.
242-258: LGTM!The function setup correctly configures the base URL with environment variable fallback and properly configures axios with authentication headers and connection settings. The explicit
keepAlive: falsesetting prevents connection reuse issues that could occur in batch operations.
326-331: Verify the polling variance calculation produces expected results.The variance calculation uses
Math.random() * pollIntervalVariance * 2 - pollIntervalVariance, which produces values in the range[-pollIntervalVariance, +pollIntervalVariance]. WithpollIntervalVariance = 2000, the actual wait time ranges from 3 to 7 seconds. Ensure this randomization aligns with the intended load distribution on the API.
242-338: Consider whether the fullconfigobject should be passed to the API.Currently,
runViaApireceivesresolvedTestsandapiKey, but only usesconfig.apiMaxWaitTimefrom the config. The API endpoint receives the resolved tests but not the original configuration settings (e.g.,logLevel,recording,allowUnsafeSteps). Verify whether the API should receive additional config parameters to match local execution behavior.If the API needs access to config settings, consider modifying the function signature:
async function runViaApi({ resolvedTests, apiKey, config = {} }) { // Include relevant config in the API request body const payload = { ...resolvedTests, settings: { logLevel: config.logLevel, recording: config.recording, // other relevant settings } }; // ... }src/index.js (4)
1-5: LGTM!The import updates correctly reflect the new dependencies:
detectAndResolveTestsfrom the resolver package andrunViaApifrom the local tests module. The changes support the new API-based execution pathway.
19-26: LGTM!The signature change to accept an
optionsparameter is backward compatible (defaults to{}), and the logic to handle pre-resolved tests viaoptions.resolvedTestsis clean. OverridingconfigwithresolvedTests.configensures consistency when tests are pre-resolved.
31-37: LGTM!The conditional test resolution correctly skips detection when
resolvedTestsare already provided, avoiding unnecessary work and improving performance when tests are pre-resolved externally.
39-49: Add defensive check for nested property access.Line 40 accesses deeply nested properties (
config.integrations.docDetectiveApi.apiKey) without checking if intermediate properties exist. Ifconfig.integrationsorconfig.integrations.docDetectiveApiis undefined, this will throw a TypeError.Apply this diff to add optional chaining:
- if (config.integrations && config.integrations.docDetectiveApi && config.integrations.docDetectiveApi.apiKey) { + if (config.integrations?.docDetectiveApi?.apiKey) { // Run test specs via API results = await runViaApi({ resolvedTests, apiKey: config.integrations.docDetectiveApi.apiKey, });Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
📝 Documentation updates detected! New suggestion: Document API client and runner functionality |
|
@copilot Write tests for the functionality introduced in this PR. |
Summary by CodeRabbit