Skip to content

fix(test): remove quotes from string variables in test.each#23244

Merged
pfgithub merged 5 commits into
mainfrom
claude/fix-test-each-string-quotes
Oct 7, 2025
Merged

fix(test): remove quotes from string variables in test.each#23244
pfgithub merged 5 commits into
mainfrom
claude/fix-test-each-string-quotes

Conversation

@robobun

@robobun robobun commented Oct 4, 2025

Copy link
Copy Markdown
Collaborator

Summary

Fixes #23206

When using test.each with object syntax and $variable interpolation, string values were being quoted (e.g., "apple" instead of apple). This didn't match the behavior of %s formatting or Jest's behavior.

Changes

  • Modified formatLabel in src/bun.js/test/jest.zig to check if the value is a primitive string and use toString() instead of the formatter with quote_strings=true
  • Added regression test in test/regression/issue/23206.test.ts

Example

Before:

test.each([
  { name: "apple" },
  { name: "banana" }
])("fruit #%# is $name", fruit => {
  // Test names were:
  // "fruit #0 is "apple""
  // "fruit #1 is "banana""
});

After:

test.each([
  { name: "apple" },
  { name: "banana" }
])("fruit #%# is $name", fruit => {
  // Test names are now:
  // "fruit #0 is apple"
  // "fruit #1 is banana"
});

Test plan

  • Added regression test that verifies both %s and $name syntax produce consistent output
  • Tested with AGENT=0 - all tests pass
  • Verified other primitive types (numbers, booleans) still format correctly
  • Verified complex objects still use proper formatting

This matches Jest's behavior after their fix: jestjs/jest#7689

🤖 Generated with Claude Code

Fixes #23206

When using test.each with object syntax and $variable interpolation,
string values were being quoted (e.g., "apple" instead of apple).
This didn't match the behavior of %s formatting or Jest's behavior.

The fix checks if the value is a primitive string and uses toString()
instead of the formatter with quote_strings=true.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added the claude label Oct 4, 2025
@robobun

robobun commented Oct 4, 2025

Copy link
Copy Markdown
Collaborator Author
Updated 5:02 PM PT - Oct 6th, 2025

@pfgithub, your commit d61872d has 1 failures in Build #28206 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 23244

That installs a local version of the PR into your bun-23244 executable, so you can run:

bun-23244 --bun

@coderabbitai

coderabbitai Bot commented Oct 4, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Render string label parameters in Jest-style test output without quotes, add Output.flush() calls before exiting on certain CLI error and bail paths, update tests to expect unquoted output, and set AGENT="0" in the test environment.

Changes

Cohort / File(s) Summary
Jest label string formatting
src/bun.js/test/jest.zig
When formatting a label parameter, append string values as unquoted slices; non-string values continue to use ConsoleObject.Formatter with quoted output.
CLI flush on exit for errors/bail
src/cli/Arguments.zig, src/cli/test_command.zig
Call Output.flush() before exiting on invalid timeout/bail parsing errors and when test-run bail triggers.
Test expectations and env updates
test/cli/test/bun-test.test.ts
Adjust stderr assertions to expect unquoted strings and set AGENT: "0" in the environment for runTest.

Possibly related PRs

  • bun test dots reporter #22919 — Modifies test output/reporting logic in src/bun.js/test/jest.zig and reporter behavior; closely related to string formatting/reporting changes.

Suggested reviewers

  • dylan-conway
  • nektro

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The pull request includes flush logic additions in src/cli/Arguments.zig and src/cli/test_command.zig to ensure error output is flushed before exiting, which is unrelated to the string quoting behavior addressed in issues #23206 and #7689 and thus falls outside the intended scope of this fix. Consider moving the CLI flush logic changes into a separate pull request so this one remains focused on removing string quotes in test.each formatting.
Description Check ⚠️ Warning The pull request description does not follow the repository’s template headings; it uses “## Summary” and “## Changes” instead of the required “### What does this PR do?” and “### How did you verify your code works?” sections. Please restructure the PR description to use the required template headings and provide “What does this PR do?” and “How did you verify your code works?” sections with the corresponding details.
✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately describes the primary change of removing quotes from string variables in test.each and aligns closely with the code modifications to format string labels without quotes.
Linked Issues Check ✅ Passed The pull request modifies the label formatting logic in jest.zig to detect primitive string values and append them without quotes, updates tests to expect unquoted output, and adds a regression test ensuring both %s and $variable syntaxes produce unquoted strings, directly addressing the removal of automatic string quotes requested in issues [#23206] and [#7689].
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/fix-test-each-string-quotes

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f5d72a7 and d61872d.

📒 Files selected for processing (1)
  • src/cli/Arguments.zig (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cli/Arguments.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Format

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/regression/issue/23206.test.ts (1)

1-19: LGTM! Regression tests cover both interpolation paths.

The tests appropriately verify the fix for issue #23206:

  • Test 1 (lines 4-10) exercises %s format with string array, confirming existing unquoted behavior.
  • Test 2 (lines 12-19) exercises $name interpolation with object array, confirming the newly fixed unquoted behavior.

The comments clearly document expected test names (e.g., "fruit #0 is apple" not "fruit #0 is "apple""), making the intent explicit for reviewers and future maintainers.

Optional: Consider adding edge case coverage.

While the current tests validate the core fix, you could optionally add test cases for edge scenarios:

test.each([
  { name: "" },           // empty string
  { name: "a b" },        // string with spaces
  { name: "it's" },       // string with quotes/apostrophes
])("edge case: $name", fruit => {
  expect(fruit.name).toBeDefined();
});

This would ensure the unquoted string rendering handles special characters correctly, though the fix should handle these cases naturally.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3c96c8a and 6e6ea38.

📒 Files selected for processing (2)
  • src/bun.js/test/jest.zig (1 hunks)
  • test/regression/issue/23206.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

  • test/regression/issue/23206.test.ts
test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

  • test/regression/issue/23206.test.ts
test/**/*.test.ts

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.ts: Name test files *.test.ts and use bun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; use port: 0 to get a random port
When spawning Bun in tests, use bunExe() and bunEnv from harness
Prefer async/await in tests; for a single callback, use Promise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Use tempDir/tempDirWithFiles from harness for temporary files and directories in tests
For large/repetitive strings in tests, prefer Buffer.alloc(count, fill).toString() over "A".repeat(count)
Import common test utilities from harness (e.g., bunExe, bunEnv, tempDirWithFiles, tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and use toThrow for synchronous errors
Use describe blocks for grouping, describe.each for parameterized tests, snapshots with toMatchSnapshot, and lifecycle hooks (beforeAll, beforeEach, afterEach); track resources for cleanup in afterEach
Use using/await using with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests

Files:

  • test/regression/issue/23206.test.ts
test/regression/issue/**

📄 CodeRabbit inference engine (test/CLAUDE.md)

Place regression tests under test/regression/issue/ and organize by issue number

Place regression tests under test/regression/issue/ (one per bug fix)

Files:

  • test/regression/issue/23206.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests

Files:

  • test/regression/issue/23206.test.ts
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Implement debug logs in Zig using const log = bun.Output.scoped(.${SCOPE}, false); and invoking log("...", .{})

In Zig code, manage memory carefully and use defer for cleanup of allocations/resources

src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig

Files:

  • src/bun.js/test/jest.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)

**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue

Files:

  • src/bun.js/test/jest.zig
src/bun.js/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy

Files:

  • src/bun.js/test/jest.zig
🧠 Learnings (4)
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Prefer data-driven tests (e.g., test.each) to reduce boilerplate

Applied to files:

  • test/regression/issue/23206.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test

Applied to files:

  • test/regression/issue/23206.test.ts
📚 Learning: 2025-10-04T09:51:30.265Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.265Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests

Applied to files:

  • test/regression/issue/23206.test.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect

Applied to files:

  • src/bun.js/test/jest.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Format
🔇 Additional comments (1)
src/bun.js/test/jest.zig (1)

387-397: LGTM! String interpolation now matches Jest behavior.

The conditional logic correctly detects primitive strings and appends their literal value without quotes, while preserving the existing formatter behavior (with quote_strings = true) for non-string types like numbers, booleans, and objects. Memory management is sound with proper defer cleanup of owned_slice.

This aligns the $variable interpolation path with the %s format path (which already produces unquoted strings via consumeArg at line 415) and matches Jest's behavior per the referenced issue.

Comment thread test/regression/issue/23206.test.ts Outdated
Claude Bot and others added 2 commits October 6, 2025 07:36
After merging main, several tests were failing due to:
1. Output not being flushed before Global.exit() calls
2. AGENT env var causing test names to not be printed
3. Test expectations needing updates for fixed test.each string quote behavior

Changes:
- Add Output.flush() before Global.exit() in Arguments.zig and test_command.zig
- Set AGENT=0 in runTest() to ensure test names are printed
- Update test expectations to match new behavior (strings without quotes)
- Delete useless test/regression/issue/23206.test.ts

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@Jarred-Sumner Jarred-Sumner requested a review from pfgithub October 6, 2025 16:30
@pfgithub pfgithub self-assigned this Oct 6, 2025
@pfgithub pfgithub merged commit 0b7aed1 into main Oct 7, 2025
58 of 60 checks passed
@pfgithub pfgithub deleted the claude/fix-test-each-string-quotes branch October 7, 2025 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test.each quotes some string variables Why test.each table-variable for string data type, has double quotes?

3 participants