Fix test runner not working for tests using relative paths in workspace projects#2236
Fix test runner not working for tests using relative paths in workspace projects#2236dan-niles wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe test runner is refactored to clean projects before testing, simplify command generation by removing workspace-specific path logic, extract test report paths directly from ChangesTest Runner Refactoring
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-extension/src/features/test-explorer/runner.ts (1)
230-246: ⚡ Quick winRun pre-test project cleanup concurrently to avoid serializing LSP round-trips.
When a run touches multiple projects (common for workspace runs — exactly the scenario this PR targets),
cleanAndValidateProjectis awaited one project at a time, so each LSP round-trip blocks the next. Since the per-project cleanups are independent and errors are already swallowed/logged,Promise.allSettledgets the same semantics with parallelism.Additionally, consider checking
token.isCancellationRequestedbefore/while cleaning so users can abort during the pre-test phase (today cancellation is only honored inside the per-testforEachbelow).♻️ Suggested refactor
- const langClient = extension.ballerinaExtInstance.langClient; - for (const projectPath of projectPaths) { - try { - await cleanAndValidateProject(langClient, projectPath); - } catch (err) { - console.error(`Failed to clean project before test run: ${projectPath}`, err); - } - } + const langClient = extension.ballerinaExtInstance.langClient; + await Promise.allSettled( + Array.from(projectPaths).map(async (projectPath) => { + try { + await cleanAndValidateProject(langClient, projectPath); + } catch (err) { + console.error(`Failed to clean project before test run: ${projectPath}`, err); + } + }) + ); + if (token.isCancellationRequested) { + include.forEach((test) => run.skipped(test)); + run.end(); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workspaces/ballerina/ballerina-extension/src/features/test-explorer/runner.ts` around lines 230 - 246, The current serial pre-test cleanup loops over projectPaths and awaits cleanAndValidateProject for each, causing serialized LSP round-trips; change this to run cleanAndValidateProject concurrently using Promise.allSettled over an array of promises created from projectPaths (keep using extension.ballerinaExtInstance.langClient and the existing cleanAndValidateProject function), and before starting and/or inside each cleanup promise check token.isCancellationRequested to short-circuit cancelled runs; preserve the existing error-logging behavior (console.error) for failures since allSettled will return results you can inspect and log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@workspaces/ballerina/ballerina-extension/src/features/test-explorer/runner.ts`:
- Around line 230-246: The current serial pre-test cleanup loops over
projectPaths and awaits cleanAndValidateProject for each, causing serialized LSP
round-trips; change this to run cleanAndValidateProject concurrently using
Promise.allSettled over an array of promises created from projectPaths (keep
using extension.ballerinaExtInstance.langClient and the existing
cleanAndValidateProject function), and before starting and/or inside each
cleanup promise check token.isCancellationRequested to short-circuit cancelled
runs; preserve the existing error-logging behavior (console.error) for failures
since allSettled will return results you can inspect and log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45164d2a-ca5e-4bf5-a3df-6b66c45fba48
📒 Files selected for processing (1)
workspaces/ballerina/ballerina-extension/src/features/test-explorer/runner.ts
There was a problem hiding this comment.
Pull request overview
This PR updates the Ballerina test explorer runner to better support workspace projects where tests rely on relative paths, by changing how bal test is invoked and how the generated test report is located.
Changes:
- Run
bal testwith the package directory as the working directory (instead of workspace root + project name argument). - Parse
bal teststdout to extract the generatedtest_results.jsonpath and use it when reporting results. - Align the test run flow with run/debug by invoking
cleanAndValidateProjectbefore executing tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dan-niles
force-pushed
the
fix-test-runner-in-workspaces
branch
from
9827f19 to
587c4e5
Compare
Purpose
This PR fixes the test runner not working when using relative paths in tests. This updates the test runner to
cdinto the relevant packages before executing the bal test command.Screen.Recording.2026-05-17.at.8.03.47.PM.mov
Summary by CodeRabbit
Bug Fixes
Improvements