Repository navigation
test(shell): align brace.test.ts nested-empty-variant cases with bash-5.2 literal {} - #34882
Conversation
…-5.2 literal {}
#34856 and #34865 were developed in parallel and merged minutes apart.
#34856 makes the lexer demote any comma-less {...} group to literal text
(bash 5.2 semantics); #34865 added test cases to the nested-empty-variant
block that still expected the parser to treat a nested {} as one empty
variant. After both landed, four of those cases assert the pre-#34856
output and fail on every lane.
- {{a,}{b,}}: update expectation to ["{ab}","{a}","{b}","{}"]. The
outer group has no top-level comma so its braces are literal; the inner
{a,}/{b,} still exercise the trailing-empty-variant parser path this
block exists to cover.
- {x,a{}b}, {a,b{}}c, {x,{}y}: drop. The code path their comment
described (parser seeing a 0-variant {}) no longer exists because the
lexer demotes {} to text first, and the resulting literal-{} behaviour
is already asserted by p{q{},r}s / {a,b{}}z / {a,{}}z in the
comma-less-is-literal block #34856 added.
All four updated/removed expectations match bash 5.2.37 exactly.
|
Status: brace.test.ts green locally (40 pass / 0 fail on |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only fix for the #34856/#34865 merge race; updated expectations match bash 5.2 and the three dropped {} cases are already covered in the comma-less brace group is literal block. The inline nit about which code path {{a,}{b,}} now dispatches to is worth a look but non-blocking (the next entry p{q,{r,}{s,}}t still covers expand_nested).
Extended reasoning...
Overview
This PR touches only test/js/bun/shell/brace.test.ts. Two PRs (#34856: comma-less {...} is literal; #34865: nested trailing-empty variant fix) merged three minutes apart with a textually-clean but semantically-conflicting rebase, leaving four hard failures on main. The change updates one expectation ({{a,}{b,}} → ["{ab}","{a}","{b}","{}"]) and deletes three cases ({x,a{}b}, {a,b{}}c, {x,{}y}) whose documented code path — a 0-variant {} reaching the nested parser — no longer exists because the lexer demotes comma-less groups to text first.
Security risks
None. Test-only; no runtime, parser, or build changes.
Level of scrutiny
Low. This is a mechanical realignment of stale test expectations with already-merged runtime behaviour. I verified all five affected inputs against bash 5.2 locally and each matches the new/implied expectations exactly. I also confirmed the three deleted inputs' literal-{} behaviour is asserted by p{q{},r}s / {a,b{}}z / {a,{}}z in the comma-less brace group is literal (bash 5.2) block, so no coverage is lost — consistent with REVIEW.md's "every deletion needs a stated reason or replacement".
Other factors
The bug hunter raised one nit: the new comment on {{a,}{b,}} claims the inner {a,}/{b,} still exercise the nested trailing-empty path, but tracing tokenize_impl → flatten_tokens shows contains_nested stays false (the outer braces become Text, so brace_count never exceeds 1) and the input dispatches to expand_flat. This is a comment-accuracy issue only — the assertion is correct, and the very next entry p{q,{r,}{s,}}t genuinely routes through expand_nested with sibling trailing-empty groups, so #34865's fix remains guarded. Not worth blocking a red-main fix over.
After #34856 demotes the outer braces to text, this input dispatches to expand_flat (brace_count never exceeds 1), not the expand_nested path the nested-empty-variant block guards. Move it to the block that tests the behaviour it actually exercises; p{q,{r,}{s,}}t in the original block still covers sibling trailing-empty groups through expand_nested.
There was a problem hiding this comment.
LGTM — thanks for moving {{a,}{b,}} to the comma-less block in 0ecd8eb; that addresses the nit and the case now also runs through the shell subprocess assertion.
Extended reasoning...
Overview
Test-only change to test/js/bun/shell/brace.test.ts fixing four hard failures on main caused by a semantic merge conflict between #34856 (comma-less {...} demoted to literal in the lexer) and #34865 (nested trailing-empty-variant fix). Drops three test.each entries whose documented code path (parser encountering a 0-variant {}) no longer exists post-#34856, and moves {{a,}{b,}} with its bash-5.2-aligned expectation to the comma-less brace group is literal block.
Security risks
None. Pure test-expectation update; no src/ change.
Level of scrutiny
Low. This is un-breaking a red test file on main by aligning stale expectations with runtime behaviour that already matches bash 5.2. The dropped cases' literal-{} behaviour is already asserted by p{q{},r}s / {a,b{}}z / {a,{}}z in the same file, and the nested trailing-empty path #34865 fixed remains covered by p{q,{r,}{s,}}t, so no coverage is lost. I verified the moved case's expectation locally against bash (printf "[%s] " {{a,}{b,}} → [{ab}] [{a}] [{b}] [{}]).
Other factors
My earlier nit (the case was in the wrong describe with a comment misstating which parser path it exercises) was addressed in 0ecd8eb — the case now sits alongside {a{b,c}} in the comma-less block where it belongs, and picks up the shell-subprocess assertion for free via the shared cases array. The PR description states each deletion's reason and names the surviving coverage, satisfying REVIEW.md's "every deletion needs a stated reason or replacement".
2 participants
Fixes
test/js/bun/shell/brace.test.tswhich is red on main (four hard failures on every lane, e.g. build 76627).Cause
#34856 and #34865 were developed in parallel and merged three minutes apart. #34856 changed the brace lexer so a
{...}group with no top-level comma is demoted to literal text (bash 5.2 semantics) before the parser sees it. #34865 added anested with empty varianttest.eachblock whose last four entries still expect the pre-#34856 behaviour. #34865's own commit message notes #34856 was addressing the literal-{}question separately; the rebase was textually clean so neither PR's CI caught the semantic overlap.bash 5.2.37 agrees with the new output in every case:
Fix
All four entries are removed from the
nested with empty variantblock: after #34856 none of them reach theparse_expansion/expand_nestedpath that block guards, because the lexer demotes their comma-less group to text first. The remaining ten entries in that block (notablyp{q,{r,}{s,}}t, which still routes throughexpand_nestedwith sibling trailing-empty groups) continue to cover #34865's fix.{{a,}{b,}}is moved to thecomma-less brace group is literal (bash 5.2)block with the bash-correct expectation["{ab}","{a}","{b}","{}"]. That block also runs each case through the actual shell in a subprocess, so the pattern now gets both$.braces()and end-to-end shell coverage.{x,a{}b},{a,b{}}c,{x,{}y}are dropped together with their now-inaccurate comment (which describes a 0-variant{}reaching the parser, a state that no longer exists). Their literal-{}behaviour is already asserted byp{q{},r}s/{a,b{}}z/{a,{}}zin that same block.No
src/change: the runtime behaviour is correct (matches bash); only the stale expectations are realigned.Verification
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file