Open
Conversation
|
Contributor
📝 WalkthroughWalkthroughAdds a detect-changes job to CI to gate per-package test jobs, replaces the single test job with multiple conditional test jobs, centralizes Vitest into a workspace projects config, removes package-local Vitest configs, and splits tests into unit vs integration with serialized integration runs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Detect as detect-changes job
participant Orchestrator as Workflow controller
participant Runner as Job runner
participant Cache as Node modules cache
participant Registry as Package registry
participant Tests as Per-package test jobs
GH->>Detect: on push/PR trigger
Detect->>Detect: evaluate path filters (core, cli, mcp, root)
Detect-->>Orchestrator: emit outputs (core=true/false, cli=..., mcp=..., root=...)
Orchestrator->>Runner: start conditional test job (e.g., test-core) if output true
Runner->>Cache: restore node_modules cache (if present)
Runner->>Registry: install dependencies
Runner->>Tests: run unit tests
Tests->>Runner: unit tests complete
Runner->>Tests: run integration tests (serialized via --concurrency=1)
Tests->>Cache: save updated node_modules cache
Tests-->>GH: report job result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
- Fix loop.command.spec.ts mock return types (checkSandboxAuth returns {ready: boolean})
- Fix config-loader.service.spec.ts fs/promises mocking approach
- Fix runtime-state-manager.service.spec.ts module path and field name mismatches
- Fix config-manager.spec.ts mock constructors and API expectations
- Fix loop.service.spec.ts for sandbox opt-in and appendFile behavior
- Update task-id.ts regex to support sub-subtasks (1.2.3) and variable-length prefixes
- Increase generate.tool.test.ts timeout for 2-call test
- Add turbo:test, turbo:test:unit, turbo:test:integration scripts
Crunchyman-ralph
force-pushed
the
ralph/fix/tests.1
branch
from
26b40a8 to
cc13c08
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 186-190: The checkout step uses a shallow clone which prevents
dorny/paths-filter (id: filter) from seeing diffs across multi-commit pushes;
update the actions/checkout@v4 step to fetch full history by adding with:
fetch-depth: 0 (i.e., use actions/checkout@v4 with fetch-depth: 0) so the
dorny/paths-filter@v3 step can correctly detect changes across commits.
- Around line 191-210: The CI root filter currently omits package.json so
changes to the repo root package.json won't trigger the root job; update the
filters block for the root key (the YAML mapping labeled "root" alongside
"core", "cli", "mcp") to include 'package.json' (similar to existing
'package-lock.json') so modifications to root package metadata/scripts/deps will
run the root tests and related workflows.
cc13c08 to
c7dff2c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Split the main Vitest configuration into separate unit and integration configurations for better clarity and management. - Update CLI and MCP package scripts to use the new configuration files. - Remove legacy configuration files from CLI and MCP packages.
Crunchyman-ralph
force-pushed
the
ralph/fix/tests.1
branch
from
526cb58 to
2bdd656
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 194-198: In the CI workflow 'core' file-glob list update the
incorrect filename: replace the non-existent 'vitest.workspace.ts' entry with
the actual config file name 'vitest.config.ts' so the workflow references the
correct Vitest config; edit the list containing 'packages/tm-core/**',
'package-lock.json', 'tsconfig.json' and change the 'vitest.workspace.ts' item
to 'vitest.config.ts'.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
334-340: Root test job condition is correct but complex.The
ifcondition on line 340 is well-constructed:
always()ensures evaluation even if upstream jobs are skipped!cancelled()prevents running if workflow was cancelled!contains(needs.*.result, 'failure')blocks on any failure- Final gate on
detect-changes.outputs.rootConsider adding a brief inline comment in the YAML to document this logic for future maintainers.
vitest.config.ts (1)
17-72: Consider extracting shared alias configuration.The
resolve.aliasblock is duplicated betweencore:unit(lines 24-42) andcore:integration(lines 52-70). While this works, extracting a shared constant would reduce maintenance burden.♻️ Optional: Extract shared aliases
+const coreAliases = { + '@': path.resolve(__dirname, './packages/tm-core/src'), + '@/types': path.resolve(__dirname, './packages/tm-core/src/types'), + '@/providers': path.resolve(__dirname, './packages/tm-core/src/providers'), + '@/storage': path.resolve(__dirname, './packages/tm-core/src/storage'), + '@/parser': path.resolve(__dirname, './packages/tm-core/src/parser'), + '@/utils': path.resolve(__dirname, './packages/tm-core/src/utils'), + '@/errors': path.resolve(__dirname, './packages/tm-core/src/errors') +}; + export default defineConfig({ test: { projects: [ // Core package { test: { name: 'core:unit', root: './packages/tm-core', include: ['tests/**/*.spec.ts', 'src/**/*.spec.ts'], setupFiles: ['./tests/setup.ts'] }, - resolve: { - alias: { - '@': path.resolve(__dirname, './packages/tm-core/src'), - // ... duplicated aliases - } - } + resolve: { alias: coreAliases } }, { test: { name: 'core:integration', // ... }, - resolve: { - alias: { - '@': path.resolve(__dirname, './packages/tm-core/src'), - // ... duplicated aliases - } - } + resolve: { alias: coreAliases } },
Comment on lines
+194
to
+198
| core: | ||
| - 'packages/tm-core/**' | ||
| - 'package-lock.json' | ||
| - 'tsconfig.json' | ||
| - 'vitest.workspace.ts' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for vitest config files at repo root
fd -d 1 'vitest' --type fRepository: eyaltoledano/claude-task-master
Length of output: 142
Update CI workflow to reference correct vitest config file.
Line 198 references vitest.workspace.ts, which does not exist in the repository. The actual file is vitest.config.ts. Update this line to reference the correct filename.
🤖 Prompt for AI Agents
In @.github/workflows/ci.yml around lines 194 - 198, In the CI workflow 'core'
file-glob list update the incorrect filename: replace the non-existent
'vitest.workspace.ts' entry with the actual config file name 'vitest.config.ts'
so the workflow references the correct Vitest config; edit the list containing
'packages/tm-core/**', 'package-lock.json', 'tsconfig.json' and change the
'vitest.workspace.ts' item to 'vitest.config.ts'.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| restoreMocks: true, | ||
| mockReset: true | ||
| } | ||
| }); |
There was a problem hiding this comment.
Core setupFiles missing from standalone test configs
Low Severity
The standalone vitest.unit.config.ts and vitest.integration.config.ts configs (used by CI via npm run test:unit -w @tm/core) lack the setupFiles: ['./tests/setup.ts'] that the workspace config and the deleted per-package config both included. The setup file suppresses console.error during core tests. This creates an inconsistency between local runs (using the workspace config) and CI runs (using standalone configs).
Additional Locations (1)
| '@/utils': path.resolve(__dirname, './packages/tm-core/src/utils'), | ||
| '@/errors': path.resolve(__dirname, './packages/tm-core/src/errors') | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated alias configuration blocks in workspace config
Low Severity
The resolve.alias block is identically duplicated between the core:unit and core:integration project definitions. This increases the maintenance burden — if an alias is added or changed, it needs to be updated in both places, risking inconsistency. Extracting the shared alias map into a variable would eliminate the duplication.
Additional Locations (1)
| clearMocks: true, | ||
| restoreMocks: true, | ||
| mockReset: true | ||
| ] |
There was a problem hiding this comment.
Workspace config projects missing mock cleanup settings
Low Severity
The new workspace config's project definitions are missing clearMocks: true, restoreMocks: true, and mockReset: true. The old root config had these settings and they were inherited by per-package configs via mergeConfig. The standalone configs (vitest.unit.config.ts and vitest.integration.config.ts) correctly include them, but the workspace config projects do not. This creates an inconsistency where tests behave differently depending on which config is used — mocks won't be automatically cleaned up between tests when running via the workspace config.
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.
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
Note
Medium Risk
CI/test orchestration changes can cause missed or mis-scoped test runs if the paths filters or Vitest workspace config are incorrect, but no production runtime logic changes are included.
Overview
Refactors CI to only run tests for changed workspaces by adding a
detect-changesjob and gating newtest-core,test-cli,test-mcp, andtest-rootjobs on those outputs.Standardizes test execution by splitting unit (
.spec.ts) vs integration (.test.ts) runs across packages: packagetest:*scripts now point to sharedvitest.unit.config.ts/vitest.integration.config.ts, per-packagevitest.config.tsfiles are removed, and rootvitest.config.tsbecomes a Vitest workspace with per-project settings (including longer CLI integration timeouts). Rootturbo:test/turbo:test:integrationnow run integration tests with--concurrency=1.Also pins
task-master-aiinpackage-lock.jsonfrom*to0.42.0-rc.0for the extension workspace lockfile.Written by Cursor Bugbot for commit 2bdd656. This will update automatically on new commits. Configure here.