test(orchestrator): further unit test coverage#784
Open
frostebite wants to merge 4 commits intomainfrom
Open
Conversation
Adds 64 new mock-based unit tests covering orchestrator services that previously had zero test coverage: - TaskParameterSerializer: env var format conversion, round-trip, uniqBy deduplication, blocked params, default secrets - FollowLogStreamService: build output message parsing — end of transmission, build success/failure detection, error accumulation, Library rebuild detection - OrchestratorNamespace (guid): GUID generation format, platform name normalization, nanoid uniqueness - OrchestratorFolders: path computation for all folder getters, ToLinuxFolder conversion, repo URL generation, purge flag detection All tests are pure mock-based and run without any external infrastructure (no LocalStack, K8s, Docker, or AWS). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds four new Jest test suites covering orchestrator folder/path logic, GUID generation, log-stream processing, and task parameter serialization, plus a CI workflow step to run targeted fast unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Adds a fast-fail unit test step at the top of orchestrator-integrity, right after yarn install and before any infrastructure setup (k3d, LocalStack). Runs 113 mock-based orchestrator tests in ~5 seconds. If serialization, path computation, log parsing, or provider loading is broken, the workflow fails immediately instead of spending 30+ minutes setting up LocalStack and k3d clusters. Tests included: orchestrator-guid, orchestrator-folders, task-parameter-serializer, follow-log-stream-service, runner-availability-service, provider-url-parser, provider-loader, provider-git-manager, orchestrator-image, orchestrator-hooks, orchestrator-github-checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/model/orchestrator/options/orchestrator-folders.test.ts (1)
146-161: Usetry/finallyaroundPURGE_REMOTE_BUILDER_CACHEmutations.These tests restore env manually, but not in a failure-safe way.
try/finallyavoids cross-test contamination when assertions fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/options/orchestrator-folders.test.ts` around lines 146 - 161, The two tests that mutate process.env.PURGE_REMOTE_BUILDER_CACHE should be wrapped in try/finally to guarantee restoration on failure: in the 'returns false when env var is not set' and 'returns true when env var is set' specs, capture the original value in a local variable, perform the delete/set and call expect(OrchestratorFolders.purgeRemoteCaching) inside a try block, and then restore process.env.PURGE_REMOTE_BUILDER_CACHE to the original value (or delete it if originally undefined) in the finally block so the environment is always reset even if assertions throw.src/model/orchestrator/services/core/task-parameter-serializer.test.ts (1)
156-163: Expand assertions to cover all declared blocked/default-secret keys.Current checks miss keys declared by implementation (
CACHE_UNITY_INSTALLATION_ON_MAC,RUNNER_TEMP_PATH,NAME) and additional default secrets (UNITY_EMAIL,UNITY_PASSWORD,GIT_PRIVATE_TOKEN) fromsrc/model/orchestrator/services/core/task-parameter-serializer.ts(Line [11]-[21], Line [169]-[179]). Adding those assertions will better protect against regressions.Also applies to: 172-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/services/core/task-parameter-serializer.test.ts` around lines 156 - 163, The test only asserts some blocked keys; update the test in TaskParameterSerializer.unit tests to assert all keys declared on TaskParameterSerializer.blockedParameterNames and TaskParameterSerializer.defaultSecretParameterNames: add expect(...has('CACHE_UNITY_INSTALLATION_ON_MAC')), expect(...has('RUNNER_TEMP_PATH')), expect(...has('NAME')), and for default secrets add expects for 'UNITY_EMAIL', 'UNITY_PASSWORD', 'GIT_PRIVATE_TOKEN' (referencing TaskParameterSerializer.blockedParameterNames and TaskParameterSerializer.defaultSecretParameterNames to find where to add the assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/options/orchestrator-folders.test.ts`:
- Line 2: Remove the unused import statement "import path from 'node:path';"
from the test file (it isn't referenced anywhere), update any related imports if
you expected to use path elsewhere, and then run the linter/tests to confirm the
unused-import lint error is resolved.
In `@src/model/orchestrator/services/core/task-parameter-serializer.test.ts`:
- Around line 178-192: The test that mutates process.env.UNITY_SERIAL should
guard the mutation with try/finally: store the original value in originalSerial,
set process.env.UNITY_SERIAL = 'test-serial', run the assertions that call
TaskParameterSerializer.readDefaultSecrets() and inspect serialSecret, and then
restore or delete process.env.UNITY_SERIAL in a finally block to ensure cleanup
even on assertion failure; apply the same try/finally pattern to the other
related test(s) that mutate process.env (the block around the second test at
lines noted in the review) so no env leak occurs between tests.
---
Nitpick comments:
In `@src/model/orchestrator/options/orchestrator-folders.test.ts`:
- Around line 146-161: The two tests that mutate
process.env.PURGE_REMOTE_BUILDER_CACHE should be wrapped in try/finally to
guarantee restoration on failure: in the 'returns false when env var is not set'
and 'returns true when env var is set' specs, capture the original value in a
local variable, perform the delete/set and call
expect(OrchestratorFolders.purgeRemoteCaching) inside a try block, and then
restore process.env.PURGE_REMOTE_BUILDER_CACHE to the original value (or delete
it if originally undefined) in the finally block so the environment is always
reset even if assertions throw.
In `@src/model/orchestrator/services/core/task-parameter-serializer.test.ts`:
- Around line 156-163: The test only asserts some blocked keys; update the test
in TaskParameterSerializer.unit tests to assert all keys declared on
TaskParameterSerializer.blockedParameterNames and
TaskParameterSerializer.defaultSecretParameterNames: add
expect(...has('CACHE_UNITY_INSTALLATION_ON_MAC')),
expect(...has('RUNNER_TEMP_PATH')), expect(...has('NAME')), and for default
secrets add expects for 'UNITY_EMAIL', 'UNITY_PASSWORD', 'GIT_PRIVATE_TOKEN'
(referencing TaskParameterSerializer.blockedParameterNames and
TaskParameterSerializer.defaultSecretParameterNames to find where to add the
assertions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 857b6179-ac5c-472d-986d-168b507ceeae
📒 Files selected for processing (4)
src/model/orchestrator/options/orchestrator-folders.test.tssrc/model/orchestrator/options/orchestrator-guid.test.tssrc/model/orchestrator/services/core/follow-log-stream-service.test.tssrc/model/orchestrator/services/core/task-parameter-serializer.test.ts
Comment on lines
+178
to
+192
| it('includes secrets from environment when present', () => { | ||
| const originalSerial = process.env.UNITY_SERIAL; | ||
| process.env.UNITY_SERIAL = 'test-serial'; | ||
|
|
||
| const secrets = TaskParameterSerializer.readDefaultSecrets(); | ||
| const serialSecret = secrets.find((s) => s.ParameterKey === 'UNITY_SERIAL'); | ||
| expect(serialSecret).toBeDefined(); | ||
| expect(serialSecret?.ParameterValue).toBe('test-serial'); | ||
|
|
||
| if (originalSerial !== undefined) { | ||
| process.env.UNITY_SERIAL = originalSerial; | ||
| } else { | ||
| delete process.env.UNITY_SERIAL; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Guard process.env mutations with try/finally to prevent test bleed.
If an assertion fails before cleanup, UNITY_SERIAL leaks into later tests and creates order-dependent failures. Wrap mutation and assertions in try/finally.
Proposed fix
it('includes secrets from environment when present', () => {
const originalSerial = process.env.UNITY_SERIAL;
- process.env.UNITY_SERIAL = 'test-serial';
-
- const secrets = TaskParameterSerializer.readDefaultSecrets();
- const serialSecret = secrets.find((s) => s.ParameterKey === 'UNITY_SERIAL');
- expect(serialSecret).toBeDefined();
- expect(serialSecret?.ParameterValue).toBe('test-serial');
-
- if (originalSerial !== undefined) {
- process.env.UNITY_SERIAL = originalSerial;
- } else {
- delete process.env.UNITY_SERIAL;
- }
+ try {
+ process.env.UNITY_SERIAL = 'test-serial';
+ const secrets = TaskParameterSerializer.readDefaultSecrets();
+ const serialSecret = secrets.find((s) => s.ParameterKey === 'UNITY_SERIAL');
+ expect(serialSecret).toBeDefined();
+ expect(serialSecret?.ParameterValue).toBe('test-serial');
+ } finally {
+ if (originalSerial !== undefined) {
+ process.env.UNITY_SERIAL = originalSerial;
+ } else {
+ delete process.env.UNITY_SERIAL;
+ }
+ }
});
it('excludes secrets not in environment', () => {
const originalSerial = process.env.UNITY_SERIAL;
- delete process.env.UNITY_SERIAL;
-
- const secrets = TaskParameterSerializer.readDefaultSecrets();
- const serialSecret = secrets.find((s) => s.ParameterKey === 'UNITY_SERIAL');
- expect(serialSecret).toBeUndefined();
-
- if (originalSerial !== undefined) {
- process.env.UNITY_SERIAL = originalSerial;
- }
+ try {
+ delete process.env.UNITY_SERIAL;
+ const secrets = TaskParameterSerializer.readDefaultSecrets();
+ const serialSecret = secrets.find((s) => s.ParameterKey === 'UNITY_SERIAL');
+ expect(serialSecret).toBeUndefined();
+ } finally {
+ if (originalSerial !== undefined) {
+ process.env.UNITY_SERIAL = originalSerial;
+ } else {
+ delete process.env.UNITY_SERIAL;
+ }
+ }
});Also applies to: 194-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/services/core/task-parameter-serializer.test.ts`
around lines 178 - 192, The test that mutates process.env.UNITY_SERIAL should
guard the mutation with try/finally: store the original value in originalSerial,
set process.env.UNITY_SERIAL = 'test-serial', run the assertions that call
TaskParameterSerializer.readDefaultSecrets() and inspect serialSecret, and then
restore or delete process.env.UNITY_SERIAL in a finally block to ensure cleanup
even on assertion failure; apply the same try/finally pattern to the other
related test(s) that mutate process.env (the block around the second test at
lines noted in the review) so no env leak occurs between tests.
frostebite
changed the title
frostebite
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/orchestrator-integrity.yml (1)
205-205: Test selector is too broad for a deterministic “fast” gate.Line 205 matches by substring only, so future tests with similar names can be pulled in unintentionally. Anchor to test filenames (or switch to
--runTestsByPath) to keep runtime stable.🎯 Suggested pattern hardening
- --testPathPattern="orchestrator-guid|orchestrator-folders|task-parameter-serializer|follow-log-stream-service|runner-availability-service|provider-url-parser|provider-loader|provider-git-manager|orchestrator-image|orchestrator-hooks|orchestrator-github-checks" + --testPathPattern="(orchestrator-guid|orchestrator-folders|task-parameter-serializer|follow-log-stream-service|runner-availability-service|provider-url-parser|provider-loader|provider-git-manager|orchestrator-image|orchestrator-hooks|orchestrator-github-checks)\\.test\\.ts$"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/orchestrator-integrity.yml at line 205, The --testPathPattern entry uses an unanchored substring list (the string containing "orchestrator-guid|orchestrator-folders|...|orchestrator-github-checks"), which can accidentally match future test names; fix by anchoring each alternative (e.g., wrap each filename with ^ and $ in the regex) or replace this flag with --runTestsByPath and supply the explicit test file paths for deterministic selection. Update the line that sets --testPathPattern to either an anchored regex of the exact test filenames or switch to --runTestsByPath with the specific filenames to prevent accidental matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/orchestrator-integrity.yml:
- Around line 199-206: The "Run orchestrator unit tests (fast, no infra)" step
currently runs after heavy infra prep (LocalStack/AWS) which defeats the
fast-fail purpose; move the entire step (the job step named "Run orchestrator
unit tests (fast, no infra)" that runs yarn run test with --testPathPattern for
orchestrator-* and flags --verbose --detectOpenHandles --forceExit --runInBand)
to run immediately after checkout/setup and before any LocalStack/AWS
preparation steps so the infra-free fast gate executes first and can fail
quickly.
---
Nitpick comments:
In @.github/workflows/orchestrator-integrity.yml:
- Line 205: The --testPathPattern entry uses an unanchored substring list (the
string containing
"orchestrator-guid|orchestrator-folders|...|orchestrator-github-checks"), which
can accidentally match future test names; fix by anchoring each alternative
(e.g., wrap each filename with ^ and $ in the regex) or replace this flag with
--runTestsByPath and supply the explicit test file paths for deterministic
selection. Update the line that sets --testPathPattern to either an anchored
regex of the exact test filenames or switch to --runTestsByPath with the
specific filenames to prevent accidental matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ff29477-1ae3-4712-84ae-9140d388fdde
📒 Files selected for processing (1)
.github/workflows/orchestrator-integrity.yml
Comment on lines
+199
to
+206
| # FAST UNIT TESTS (no infra required, fast-fail gate) | ||
| # ========================================== | ||
| - name: Run orchestrator unit tests (fast, no infra) | ||
| timeout-minutes: 2 | ||
| run: >- | ||
| yarn run test | ||
| --testPathPattern="orchestrator-guid|orchestrator-folders|task-parameter-serializer|follow-log-stream-service|runner-availability-service|provider-url-parser|provider-loader|provider-git-manager|orchestrator-image|orchestrator-hooks|orchestrator-github-checks" | ||
| --verbose --detectOpenHandles --forceExit --runInBand |
There was a problem hiding this comment.
Fast-fail gate is still positioned after heavy infra setup.
Line 199 says this gate is infra-free, but it currently runs after LocalStack/AWS prep earlier in the job. That defeats the fast-fail intent and still spends setup time before failing.
🔧 Suggested reorder
- - name: Set up kubectl
- uses: azure/setup-kubectl@v4
- with:
- version: 'v1.34.1'
- - name: Install k3d
- run: |
- curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash
- k3d version | cat
- ...
- run: yarn install --frozen-lockfile
+ - name: Run orchestrator unit tests (fast, no infra)
+ timeout-minutes: 2
+ run: >-
+ yarn run test
+ --testPathPattern="orchestrator-guid|orchestrator-folders|task-parameter-serializer|follow-log-stream-service|runner-availability-service|provider-url-parser|provider-loader|provider-git-manager|orchestrator-image|orchestrator-hooks|orchestrator-github-checks"
+ --verbose --detectOpenHandles --forceExit --runInBand
+ - name: Set up kubectl
+ uses: azure/setup-kubectl@v4
+ with:
+ version: 'v1.34.1'
+ - name: Install k3d
+ run: |
+ curl -s https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash
+ k3d version | cat🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/orchestrator-integrity.yml around lines 199 - 206, The
"Run orchestrator unit tests (fast, no infra)" step currently runs after heavy
infra prep (LocalStack/AWS) which defeats the fast-fail purpose; move the entire
step (the job step named "Run orchestrator unit tests (fast, no infra)" that
runs yarn run test with --testPathPattern for orchestrator-* and flags --verbose
--detectOpenHandles --forceExit --runInBand) to run immediately after
checkout/setup and before any LocalStack/AWS preparation steps so the infra-free
fast gate executes first and can fail quickly.
|
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
frostebite
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #784 +/- ##
==========================================
+ Coverage 31.25% 32.96% +1.70%
==========================================
Files 84 84
Lines 4563 4563
Branches 1103 1103
==========================================
+ Hits 1426 1504 +78
+ Misses 3137 3058 -79
- Partials 0 1 +1 🚀 New features to boost your workflow:
|
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 64 new mock-based unit tests for orchestrator services that previously had zero test coverage. These tests run without any external infrastructure and expand the safety net for PRs.
New test files
task-parameter-serializer.test.tsToEnvVarFormat,UndoEnvVarFormat), round-trip fidelity,uniqBydeduplication, blocked parameter filtering, default secret readingfollow-log-stream-service.test.tserror,error:,command failed:,invalid,cannot be found)orchestrator-guid.test.tsstandaloneprefix stripping, platform lowercasing, nanoid uniqueness and character setorchestrator-folders.test.tsToLinuxFolderslash conversion, repo URL generation,purgeRemoteCachingenv flag, cache folder pathsWhy these files
These four source files are the most critical untested orchestrator components:
Test characteristics
yarn test:cion every PRTest plan
yarn test— 458 passed, 0 failures)🤖 Generated with Claude Code
Summary by CodeRabbit
Tracking: