feat: test config initial runtime values setup#177
Conversation
…low 'safe' credential passing
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce environment variable injection for tests via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (4)
packages/cli/src/commands/generate/implementation/repo.ts (1)
119-144: Consider usinglog.errorfor write failures.While the per-file error handling is well-implemented, line 140-141 uses
log.warnfor write failures. Since these failures prevent generated files from being written, consider usinglog.errorinstead for clearer severity indication.Apply this diff:
if (failedWrites.length) { - log.warn(`Some files failed to write (${failedWrites.length}):`); - failedWrites.forEach((r) => log.warn(` - ${r.path}: ${r.error}`)); + log.error(`Some files failed to write (${failedWrites.length}):`); + failedWrites.forEach((r) => log.error(` - ${r.path}: ${r.error}`)); } else { log.info('All files written successfully.'); }packages/core/src/index.ts (1)
335-366: Consider makinginitialRuntimeValuesoptional for backward compatibility.The new
initialRuntimeValuesparameter is currently required, which could break existing callers of the public API. Since the testRunner already handles this with a fallback (initialRuntimeValues ?? {}), consider making it optional here as well.Apply this diff:
async runTests({ context, groups, testConfig, - initialRuntimeValues, + initialRuntimeValues = {}, }: { context: Context; groups: ApiGroupConfig[]; testConfig: any; - initialRuntimeValues: Record<string, any>; + initialRuntimeValues?: Record<string, any>; }): Promise<packages/cli/src/commands/test/implementation/test.ts (2)
12-32: Clarify misleading comment about key format.The comment at line 15 states "returns flat object with ENVIRONMENT.* keys", but the function actually returns keys as-is (e.g.,
XANO_PASSWORD). While the implementation is correct for use with the{{ENVIRONMENT.KEY}}template pattern in test configs, the comment could be clearer.Apply this diff:
/** * * @param cliEnvVars - object of CLI provided env vars (e.g., {DEMO_ADMIN_PWD: 'xyz'}) - * @returns flat object with ENVIRONMENT.* keys + * @returns flat object with keys as-is, to be used with {{ENVIRONMENT.KEY}} template pattern */
27-31: Simplify unnecessary loop.Lines 27-31 create a new object by copying all entries from
mergedwithout any transformation. You can directly returnmergedinstead.Apply this diff:
- const result = {}; - for (const [k, v] of Object.entries(merged)) { - result[`${k}`] = v; - } - return result; + return merged;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/release.yml(1 hunks)packages/cli/src/commands/generate/implementation/repo.ts(2 hunks)packages/cli/src/commands/test/implementation/test.ts(6 hunks)packages/cli/src/commands/test/index.ts(2 hunks)packages/core/src/features/testing/index.ts(4 hunks)packages/core/src/implementations/run-tests.ts(3 hunks)packages/core/src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/implementations/run-tests.ts (1)
packages/core/src/features/testing/index.ts (1)
testRunner(264-264)
packages/cli/src/commands/test/index.ts (1)
packages/cli/src/utils/methods/with-error-handler.ts (1)
withErrorHandler(24-43)
🔇 Additional comments (7)
.github/workflows/release.yml (1)
10-12: LGTM: Appropriate permissions for automated releases.The
contents: writepermission enables the changesets action to commit release changes back to the repository, which is standard practice for automated release workflows.packages/cli/src/commands/test/index.ts (1)
35-43: LGTM: Robust parsing of environment variables.The parsing logic correctly handles edge cases including:
- Values containing
=characters (line 40 joins with=)- Empty keys (line 39 validates
keyexists)- Missing values (line 39 checks
rest.length > 0)packages/core/src/implementations/run-tests.ts (1)
5-41: LGTM: Clean parameter threading.The
initialRuntimeValuesparameter is correctly added to the function signature, type annotation, and forwarded totestRunnerwithout introducing any behavioral changes.packages/core/src/features/testing/index.ts (1)
64-114: LGTM: Proper initialization of runtime values.The initialization logic correctly uses nullish coalescing to provide a fallback empty object, and subsequent test execution can augment these values (lines 230-233) as expected.
packages/cli/src/commands/test/implementation/test.ts (2)
47-130: LGTM: Well-structured test summary formatting.The summary table provides comprehensive test results with appropriate detail levels. The dynamic column width calculation ensures clean formatting regardless of content length.
170-230: LGTM: Clean integration of runtime values collection.The function correctly collects initial runtime values from both environment variables and CLI arguments, then passes them to the test runner at the appropriate point in the execution flow.
packages/cli/src/commands/generate/implementation/repo.ts (1)
59-74: The review comment's concern is unfounded—the code is safe as written.The dummy configs are created only when
input && !fetchis true (line 61). Whenfetchis true, the condition evaluates to false andresolveConfigsis called instead, which provides all required fields includingurl. The subsequentif (fetch)block at line 91 that accessesinstanceConfig.urlis only reached whenresolveConfigsprovides complete configs. The two paths are mutually exclusive—there is no scenario where dummy configs (which lackurl) would be used with code that accessesurl.The dummy configs contain all fields they actually need for their intended use case:
instanceConfig.name,process.output✓ (used at lines 81, 79—both accessible via either path)workspaceConfig.name,id✓ (used at lines 82, 95—line 95 only inif(fetch)block)branchConfig.label✓ (used at lines 83, 96—line 96 only inif(fetch)block)No validation changes are needed.
1 participant
Summary by CodeRabbit
New Features
--test-envCLI option to inject environment variables into test runs.Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.