feat(ws): ws test consults adapter, supports Gradle, auto-filters by test name#38
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds support for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as "User"
participant CLI as "ws CLI"
participant Adapter as "Overlay Adapter\n(commands.test)"
participant Gradle as "Gradle (gradlew)"
participant Makefile as "Makefile (test)"
participant Go as "Go (go test)"
participant Py as "Python (pytest)"
User->>CLI: run `ws test <component> [TestName|args...]`
CLI->>CLI: parse args -> flags vs test_filter
CLI->>Adapter: check `commands.test`
alt Adapter present
Adapter->>Adapter: run adapter test (pass args/filter)
Adapter-->>CLI: exit
else Adapter absent
CLI->>Gradle: check `gradlew`
alt Gradle present
CLI->>Gradle: if test_filter -> _ws_gradle_find_test() -> run `:subproject:cleanTest` then `:subproject:test --tests "*.<filter>"` else run full suite
else
CLI->>Makefile: check `make test` target
alt Makefile present and no filter/extra flags
Makefile-->>CLI: run `make test`
else
CLI->>Go: check `go.mod`
alt Go present
Go-->>CLI: run `go test ./... -count=1 -run "$test_filter"` (or full suite)
else
CLI->>Py: check `pyproject.toml`
Py-->>CLI: run `uv run pytest` (with `-k "$test_filter"` if present)
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ws`:
- Around line 347-352: The flags-only branch (elif checking ${`#runner_args`[@]}
-gt 0) currently runs go test "${runner_args[@]}" only in the current package;
change that invocation to run the full module tree with cache disabled by
invoking go test ./... -count=1 "${runner_args[@]}" so that both the flags-only
path and the no-arg / filtered paths behave consistently; update the command in
the elif branch accordingly (the conditional around runner_args and the go test
invocation are the targets).
- Around line 215-220: The current derivation of subproject from matches[0]
fails for root tests because the pattern removal `${subproject%%/src/test/*}`
expects a leading slash; update the logic around the `subproject` variable
assignment so you first normalize or explicitly handle paths that start with
`./src/test/` (or otherwise detect the root-test pattern) and set `subproject`
to "." or empty before the later check, then keep the existing conditional that
echoes `:test` when `subproject` is "." or empty; adjust the code that does
`local subproject="${matches[0]#./}"` and the subsequent
`${subproject%%/src/test/*}` expansion to account for root-test paths.
- Around line 305-317: The argument loop currently splits flags from their
values causing flags like -run, --tests, and -k to be separated into test_filter
and runner_args; modify the parsing in the for-loop that builds test_filter and
runner_args so that when you encounter a flag that expects a value (at minimum
the known set "-run", "--tests", "-k") you consume the next positional argument
as its value and append both the flag and its value to runner_args (leaving
test_filter empty), rather than treating the value as a test_filter; ensure the
logic still handles flags without values and multiple test patterns by checking
the current arg against the known value-taking flags before assigning to
test_filter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Cervator
added a commit
that referenced
this pull request
Apr 22, 2026
…, safe exec Address four CodeRabbit findings on PR #38: 1. Root-project Gradle test path derivation: strip pattern expected a leading slash, breaking root-level tests. Detect `src/test/*` prefix explicitly and emit `:test`. 2. Arg parser split value-expecting flags from their values. `-run TestFoo` became `runner_args=(-run)` + `test_filter=TestFoo`. Track known value-expecting flags (`-run`, `--tests`, `-k`, etc.) and keep the pair together. 3. Adapter Gradle invocation used `bash -c` with unquoted substitution, enabling command injection via crafted test filter. Replace with direct exec using array expansion. 4. Go flags-only branch ran `go test "${runner_args[@]}"` without `./... -count=1`, inconsistent with the other Go branches. Add them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
force-pushed
the
feat/ws-test-adapter
branch
from
07591a8 to
104950a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ws`:
- Around line 326-329: The case block that sets expect_value=true incorrectly
treats Gradle's --parallel as a value-taking flag; update the case in the
scripts/ws loop (the case that assigns expect_value=true) to remove --parallel
and instead add Go flags that do consume values (e.g., -count, -bench,
-benchtime, -cpu, -shuffle) so the parser doesn't swallow the next token; keep
the expect_value mechanic and only list flags that actually take an argument.
- Around line 356-358: The current replacement for clean_task uses global
substitution on gradle_task which can mangle project names containing ":test";
instead, derive clean_task by removing only the trailing ":test" suffix from
gradle_task and then appending ":cleanTest" (so only the task suffix is
changed), and keep using that clean_task when invoking ./gradlew with
gradle_task, test_filter and runner_args; update the code around the gradle_task
-> clean_task assignment to perform suffix-stripping (for example using shell
parameter expansion to remove the trailing ":test") before appending
":cleanTest".
- Around line 352-364: The targeted-task branch is calling ./gradlew directly
and drops adapter-provided base args; instead reuse gradle_argv so overlay args
(like --no-daemon or init scripts) are preserved. Change the branch where
gradle_task and clean_task are used to invoke the command via gradle_argv: build
a base array from gradle_argv with any trailing task element removed (so you can
append "$clean_task" and "$gradle_task"), then execute "${base[@]}"
"$clean_task" "$gradle_task" --tests "*.$test_filter" "${runner_args[@]}";
reference symbols: _ws_gradle_find_test, gradle_task, clean_task, gradle_argv,
test_filter, runner_args.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Cervator
added a commit
that referenced
this pull request
Apr 23, 2026
…fix sub Address 3 CodeRabbit findings on PR #38: 1. --parallel was wrongly listed as value-consuming; it's a Gradle boolean flag. Also add Go flags that DO take values (-count, -bench, -benchtime, -cpu, -shuffle, etc.) and Gradle init/project flags (-I, --init-script, --build-file, --project-dir). 2. Targeted-Gradle branch hardcoded ./gradlew, dropping adapter base args like --no-daemon or -I init.gradle. Reuse gradle_argv minus the trailing task so adapter flags flow through. Also: guard the Gradle path for non-Gradle adapters (just run the adapter with runner_args, no task substitution). 3. Use suffix substitution (${gradle_task%:test}:cleanTest) instead of global substitution to avoid mangling subproject names that happen to contain ':test' (e.g., :testSupport:test). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test name - Check overlay adapter test command before auto-detection - Add Gradle runner (gradlew) to auto-detection chain - Any non-flag arg is treated as a test name and translated to the runner's filter syntax (--tests for Gradle, -run for Go, -k for pytest) - Targeted Gradle runs auto-clear test cache (cleanTest) to avoid stale cached failures - Auto-discover Gradle subproject for a test class via find, with warning on ambiguous multi-match - Help text no longer hardcodes runner priority order — points to ws actions instead Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, safe exec Address four CodeRabbit findings on PR #38: 1. Root-project Gradle test path derivation: strip pattern expected a leading slash, breaking root-level tests. Detect `src/test/*` prefix explicitly and emit `:test`. 2. Arg parser split value-expecting flags from their values. `-run TestFoo` became `runner_args=(-run)` + `test_filter=TestFoo`. Track known value-expecting flags (`-run`, `--tests`, `-k`, etc.) and keep the pair together. 3. Adapter Gradle invocation used `bash -c` with unquoted substitution, enabling command injection via crafted test filter. Replace with direct exec using array expansion. 4. Go flags-only branch ran `go test "${runner_args[@]}"` without `./... -count=1`, inconsistent with the other Go branches. Add them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…fix sub Address 3 CodeRabbit findings on PR #38: 1. --parallel was wrongly listed as value-consuming; it's a Gradle boolean flag. Also add Go flags that DO take values (-count, -bench, -benchtime, -cpu, -shuffle, etc.) and Gradle init/project flags (-I, --init-script, --build-file, --project-dir). 2. Targeted-Gradle branch hardcoded ./gradlew, dropping adapter base args like --no-daemon or -I init.gradle. Reuse gradle_argv minus the trailing task so adapter flags flow through. Also: guard the Gradle path for non-Gradle adapters (just run the adapter with runner_args, no task substitution). 3. Use suffix substitution (${gradle_task%:test}:cleanTest) instead of global substitution to avoid mangling subproject names that happen to contain ':test' (e.g., :testSupport:test). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agent-refr
force-pushed
the
feat/ws-test-adapter
branch
from
8169481 to
4c2de87
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ws`:
- Around line 285-301: The nested-Go detection currently prefers a single entry
in go_candidates and sets runner="go" before ever checking for a top-level
pyproject.toml, which makes Python ignored in mixed repos; update the logic
around go_candidates so the existence of a top-level pyproject.toml is
considered at the same precedence as a single nested go.mod (e.g., check for -f
pyproject.toml alongside the single go_candidates case or explicitly test for
pyproject.toml before setting runner="go"), and if you keep Go precedence emit a
clear informational message indicating Go was chosen; reference the
go_candidates array and the runner variable and ensure the pyproject.toml check
is hoisted out of the sole-else branch so Python projects aren’t silently
ignored.
- Around line 348-359: The non-Gradle branch in the case for "adapter|gradle"
drops test_filter when it runs the adapter directly; update the branch where it
currently executes `"${adapter_argv[@]}" "${runner_args[@]}"` and returns so
that it either appends the test_filter as a trailing positional argument (e.g.,
include test_filter after runner_args when non-empty) or, alternatively, fail
fast with a clear message instructing users to use `ws exec` for filtered runs;
modify the logic around adapter_argv, runner_args and test_filter in the
non-Gradle path to implement one of these behaviors and ensure the command
invocation and return remain consistent.
- Around line 199-227: _ws_gradle_find_test currently only searches
src/test/java for "${class_name}.java" so it misses tests in other JVM source
dirs (kotlin, groovy, scala); update the find invocation in _ws_gradle_find_test
to match any file under */src/test/*/${class_name}.* (or explicitly include
.java .kt .kts .groovy .scala) so it discovers tests regardless of language;
keep the existing subproject extraction logic (subproject="${matches[0]#./}" and
subproject="${subproject%%/src/test/*}") as-is since it already strips the
language directory correctly.
- Around line 341-346: The adapter command string splitting using read -r -a
adapter_argv on adapter_cmd breaks quoted arguments (e.g., "*.Foo Bar") — either
change the contract to require commands be provided as YAML sequences (consume
them as arrays directly into adapter_argv) or replace the naive whitespace split
with a shell-safe tokenizer: feed adapter_cmd through a safe splitter (e.g., use
printf '%s\n' "$adapter_cmd" piped into xargs/while-read to preserve quoted
tokens) and populate adapter_argv from those tokens; update code paths that
reference adapter_cmd and adapter_argv accordingly and add a note in the adapter
contract if you keep the simple whitespace-split behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…adapter Address 3 CodeRabbit findings on PR #38: 1. _ws_gradle_find_test only searched src/test/java, missing Kotlin (.kt), Groovy (.groovy), and Scala (.scala) test sources. Loop over all four language directories. 2. Non-Gradle adapter path silently dropped test_filter, running the full suite with no warning. Now errors with guidance pointing to 'ws exec' when a filter is provided against a non-Gradle adapter. 3. Document the "no quoted args" contract on adapter_cmd parsing — adapter commands are whitespace-separated tokens; use a wrapper script if complex quoting is needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ws`:
- Around line 240-257: The usage/help block in the ws test command currently
treats explicit help and missing-argument cases the same and always exits with
1; split the conditional so that the no-argument case (check on $# -lt 1) prints
the usage to stderr and exits 1, while the explicit help case (when "$1" ==
"--help" or "$1" == "-h") prints the same usage text to stdout and exits 0;
update the conditional around the existing usage text in the scripts/ws file
(the block that checks "$#" and "$1") to handle these two branches separately
and preserve the existing messages and formatting.
- Around line 337-343: The case block inside the argument parsing logic in
scripts/ws currently lists pytest flags that consume a value (the pattern
matching for -run|--tests|... leading to expect_value=true) but omits the pytest
marker flag -m, causing markers to be misparsed; update that case pattern to
include -m so the parser sets expect_value=true for -m (consider also adding
other value-taking pytest flags like -o, -r, --tb, --maxfail if desired) so that
when parsing in the function handling args (where expect_value is used to
collect runner_args/test_filter) the marker expression is consumed as the flag's
value instead of being treated as a test name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Pull request overview
Updates the workspace CLI’s ws test subcommand to better integrate overlay adapters and support more ecosystems (notably Gradle), while adding a convenience “test name” positional argument that’s translated into runner-specific filter flags.
Changes:
- Detects and prefers overlay adapter
commands.testbefore falling back to auto-detected runners. - Adds Gradle runner support (including subproject discovery for test classes and targeted
--testsexecution). - Treats the first non-flag argument as a test filter and passes runner flags through.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lags Address 5 review findings on PR #38 (CodeRabbit + Copilot): - Runner precedence: Gradle before Make, match ws-overlay.sh ws_actions ordering (was Make → Gradle → Go → Python, now Gradle → Make → Go → Python) - pyproject.toml detection hoisted above nested go.mod search so mixed Go+Python repos don't silently pick Go - --help / -h now exits 0 and prints to stdout (was exit 1, stderr) - Gradle --tests pattern: pass FQNs (org.foo.Bar) and wildcards through as-is; only prefix *. for bare class names - Add pytest value-taking flags to expect_value list: -m, -r, -o, --maxfail, --tb - Remove 2>/dev/null suppression on ws_detect_overlay so YAML parse errors surface to the user Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ws`:
- Line 18: The header line in the script's help output still documents the old
runner precedence "(adapter > Makefile > Gradle > Go > Python)"; update that
help/header string in the scripts/ws file to match the actual runner order
implemented in ws_test (adapter > Gradle > Makefile > Go > Python) so the "ws
help" output reflects the true precedence. Locate the help/header comment that
begins with "# test <comp> [TestName|args...] Run tests" and change the
parenthetical runner order to "adapter > Gradle > Makefile > Go > Python" so it
aligns with the ws_test implementation.
- Around line 411-414: The slice that builds gradle_base can produce an empty
array if gradle_argv contains a single token; add a defensive guard after
computing gradle_base to handle that case: if ${`#gradle_base`[@]} -eq 0 then set
gradle_base to the single token (e.g., gradle_base=("${gradle_argv[0]}")) so the
invocation uses the adapter executable rather than trying to execute a task as a
program; refer to gradle_argv, gradle_base, clean_task and gradle_task when
making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Member
|
This is turning into another GDD-showcase PR, I think. It highlights some benefits:
To also show a snippet of what happens in the local GDD workspace here's a bit of session content: Not sure what happened to that bare |
Address 2 CodeRabbit findings on PR #38: - Header comment (rendered by 'ws help') listed the old runner precedence. Align with the implementation: adapter > Gradle > Makefile > Go > Python. - Targeted-Gradle branch assumed the adapter's last token is a strippable task. If an adapter is misconfigured with a single token (e.g. just './gradlew'), the base slice would be empty and the next exec would try to run ':subproject:cleanTest' as a program. Add a guard that errors with a clear format hint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cervator
dismissed
coderabbitai[bot]’s stale review
Dismissing stale review that didn't auto-resolve
Cervator
approved these changes
Apr 24, 2026
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
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
ws testnow consults overlay adapter commands before auto-detection, and adds Gradle as a detected runner--testsfor Gradle,-runfor Go,-kfor pytest)cleanTest) to avoid stale cached failuresfind, with warning on ambiguous matchesws actionsinsteadTest plan
ws test terasology ClientNetworkStateTest— routes through adapter, discovers:engine-tests:test, clears cache, passesws testwith no args shows updated helpws actions terasologyshows adapter + auto-detected commandsbash -n scripts/ws)Related