fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

190n · 2025-12-17T06:58:10Z

What does this PR do?

fixes both functions returning false for double-encoded values (even if the numeric value is a valid int32/uint32)
fixes IsUint32() returning false for values that don't fit in int32
fixes the test from feat: implement V8 Value type checking APIs #22462 not testing anything (the native functions were being passed a callback to run garbage collection as the first argument, so it was only ever testing what the type check APIs returned for that function)
extends the test to cover the first edge case above

How did you verify your code works?

The new tests fail without these fixes.

coderabbitai · 2025-12-17T07:01:08Z

Walkthrough

Replaced V8 value int32/uint32 predicate calls with versions that treat ints uniformly; updated tests to pass CLI args as optional strings via eval, removed BigInt JSON helpers and GC wrapper, and applied minor test formatting changes.

Changes

Cohort / File(s)	Summary
V8 Value Type Checking `src/bun.js/bindings/v8/V8Value.cpp`	Swapped `IsInt32()`/`IsUint32()` to call `isInt32AsAnyInt()`/`isUInt32AsAnyInt()` respectively for integer type checks
Test Harness & Runner `test/v8/v8-module/main.js`, `test/v8/v8-module/main.cpp`	Removed BigInt JSON reviver and `parseArgs`; replaced `JSON.parse` with `eval` for CLI args; removed `runGC` and adjusted test invocation to pass args directly; minor whitespace formatting in `main.cpp`
Test Suite `test/v8/v8.test.ts`	Changed `checkSameOutput` and `runOn` signatures to accept optional string `jsArgs` instead of arrays; removed custom BigInt serialization; updated many tests to use stringified argument literals and added Math.fround-related checks

Possibly related PRs

feat: implement V8 Value type checking APIs #22462: Modifies IsInt32/IsUint32 predicate implementations at the same V8Value.cpp boundary and is directly related to these predicate changes.

Suggested reviewers

Jarred-Sumner
taylordotfish

Pre-merge checks

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing edge cases in v8::Value's IsInt32() and IsUint32() type-checking functions.
Description check	✅ Passed	The description fully covers both required sections: it clearly explains what the PR does (four specific fixes/improvements) and how verification was performed (new tests fail without the fixes).

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.

❤️ Share

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

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

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 1e86ceb and 46c51a2.

📒 Files selected for processing (4)

src/bun.js/bindings/v8/V8Value.cpp (2 hunks)
test/v8/v8-module/main.cpp (1 hunks)
test/v8/v8-module/main.js (1 hunks)
test/v8/v8.test.ts (5 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

src/**/*.{cpp,zig}

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

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

src/bun.js/bindings/v8/V8Value.cpp

src/bun.js/bindings/v8/V8*.cpp

📄 CodeRabbit inference engine (src/bun.js/bindings/v8/CLAUDE.md)

src/bun.js/bindings/v8/V8*.cpp: Create V8 class implementations with .cpp extension following the pattern V8ClassName.cpp that include the header, v8_compatibility_assertions.h, use ASSERT_V8_TYPE_LAYOUT_MATCHES macro, and implement methods using isolate->currentHandleScope()->createLocal() for handle creation
Use isolate->currentHandleScope()->createLocal() to create local V8 handles and ensure all V8 values are created within an active handle scope
Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations
Include ASSERT_V8_TYPE_LAYOUT_MATCHES(v8::ClassName) macro at the beginning of each V8 class implementation file to verify type layout compatibility with V8
Use V8_UNIMPLEMENTED() macro for functions not yet implemented in V8 compatibility classes
Use JSC::WriteBarrier for heap-allocated references in V8 objects and implement visitChildren() for custom heap objects to support garbage collection

Files:

src/bun.js/bindings/v8/V8Value.cpp

src/bun.js/bindings/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings

Files:

src/bun.js/bindings/v8/V8Value.cpp

test/**/*.{js,ts,jsx,tsx}

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

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

test/v8/v8-module/main.js
test/v8/v8.test.ts

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

test/v8/v8.test.ts

**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

test/v8/v8.test.ts

test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

test/v8/v8.test.ts

🧠 Learnings (38)

📓 Common learnings

Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22278
File: src/ast/E.zig:980-1003
Timestamp: 2025-09-04T02:04:43.094Z
Learning: In Bun's Zig codebase, `as(i32, u8_or_u16_value)` is sufficient for casting u8/u16 to i32 in comparison operations. `intCast` is not required in this context, and the current casting approach compiles successfully.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time

Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Validate function arguments using validators from `internal/validators` and throw `$ERR_*` error codes for invalid arguments

📚 Learning: 2025-11-24T18:37:47.899Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp
test/v8/v8-module/main.js

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp
test/v8/v8-module/main.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Create V8 class implementations with .cpp extension following the pattern V8ClassName.cpp that include the header, v8_compatibility_assertions.h, use ASSERT_V8_TYPE_LAYOUT_MATCHES macro, and implement methods using isolate->currentHandleScope()->createLocal<T>() for handle creation

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp
test/v8/v8-module/main.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp
test/v8/v8-module/main.cpp
test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp
test/v8/v8-module/main.cpp
test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-10-01T21:59:54.571Z

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use V8_UNIMPLEMENTED() macro for functions not yet implemented in V8 compatibility classes

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.h : Create V8 class headers with .h extension following the pattern V8ClassName.h that include pragma once, v8.h, V8Local.h, V8Isolate.h, and declare classes extending from Data with BUN_EXPORT static methods

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp

📚 Learning: 2025-09-05T19:49:26.188Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.

Applied to files:

src/bun.js/bindings/v8/V8Value.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object

Applied to files:

test/v8/v8-module/main.cpp
test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Include ASSERT_V8_TYPE_LAYOUT_MATCHES(v8::ClassName) macro at the beginning of each V8 class implementation file to verify type layout compatibility with V8

Applied to files:

test/v8/v8-module/main.cpp

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process

Applied to files:

test/v8/v8-module/main.cpp
test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-14T16:07:01.064Z

Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.

Applied to files:

test/v8/v8-module/main.js

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes

Applied to files:

test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `.$call()` and `.$apply()` instead of `.call()` and `.apply()` to prevent user tampering with function invocation

Applied to files:

test/v8/v8-module/main.js

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)

Applied to files:

test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced

Applied to files:

test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers

Applied to files:

test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode

Applied to files:

test/v8/v8-module/main.js
test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/bundle.test.ts : Organize bundle tests in bundle.test.ts for tests concerning bundling bugs that only occur in DevServer

Applied to files:

test/v8/v8-module/main.js

📚 Learning: 2025-11-24T18:35:39.205Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality

Applied to files:

test/v8/v8-module/main.js

📚 Learning: 2025-12-16T00:21:32.163Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.163Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-09-20T03:39:41.770Z

Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use string literal `require()` statements only; dynamic requires are not permitted

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always check exit codes and test error scenarios in error tests

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `describe.each()` for parameterized tests

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Tests should be located in the `test/` directory, organized by category: `cli/` for CLI tests, `js/` for JavaScript/TypeScript tests (further subdivided into `bun/`, `node/`, `web/`, `third_party/`), `napi/` for N-API tests, `v8/` for V8 C++ API tests, `bundler/` for bundler/transpiler tests, and `regression/issue/[number]` for regression tests

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

test/v8/v8.test.ts

🧬 Code graph analysis (2)

test/v8/v8-module/main.js (1)

test/js/node/test_runner/fixtures/02-hooks.js (1)

JSON (7-7)

test/v8/v8.test.ts (2)

test/v8/v8-module/main.cpp (2)

describe (37-68)

describe (37-37)

test/v8/v8-module/main.js (1)

args (9-9)

🪛 Biome (2.1.2)

test/v8/v8-module/main.js

[error] 9-9: eval() exposes to security risks and performance issues.

See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().

(lint/security/noGlobalEval)

🪛 Cppcheck (2.18.0)

test/v8/v8-module/main.cpp

[information] Limiting analysis of branches. Use --check-level=exhaustive to analyze all branches.

(normalCheckLevelMaxBranches)

🔇 Additional comments (7)

test/v8/v8-module/main.cpp (1)

1117-1139: No functional changes detected.

These appear to be whitespace/formatting adjustments only. The test_v8_value_type_checks function logic remains unchanged and correctly validates the input and prints type check results for comparison between Node.js and Bun.

test/v8/v8-module/main.js (1)

16-16: Simplified invocation looks correct.

Removing the runGC callback injection fixes the issue mentioned in the PR objectives where the previous test was inadvertently testing the GC callback rather than the intended values.

test/v8/v8.test.ts (4)

7-7: LGTM on the new import.

The jscDescribe import from bun:jsc is appropriately used to verify the internal representation of Math.fround(1).

139-163: Excellent test coverage for int32/uint32 edge cases.

The prerequisite test (line 142) ensures Math.fround(1) produces a double-encoded value, which is essential for validating the IsInt32()/IsUint32() fixes. The parameterized tests comprehensively cover:

Boundary values: INT32_MAX, INT32_MIN, UINT32_MAX

Overflow cases: INT32_MAX + 1, INT32_MIN - 1, UINT32_MAX + 1

The critical double-encoded case: Math.fround(1)

Various other types for regression coverage

This directly validates the PR's stated fix.

326-331: Function signature update aligns with the new argument-passing model.

The change from typed array arguments to an optional string (args?: string) correctly supports the eval()-based argument parsing in main.js. Running all three build configurations (node release, bun release, bun debug) in parallel provides good coverage.

354-376: Clear documentation and correct implementation.

The JSDoc comment explaining that jsArgs should eval() to an array is helpful for future maintainers. The default to "[]" at line 375 ensures backward compatibility with existing tests that don't pass arguments.

src/bun.js/bindings/v8/V8Value.cpp (1)

27-30: Remove inaccurate claims about code changes.

The review comment incorrectly states that the code uses isInt32AsAnyInt() and isUInt32AsAnyInt(). The actual implementation uses the regular isInt32() and isUInt32() methods from JSValue. Do not approve based on mischaracterized code changes.

Likely an incorrect or invalid review comment.

coderabbitai · 2025-12-17T07:01:11Z

test/v8/v8-module/main.js

-
 const testName = process.argv[2];
-const args = parseArgs(process.argv[3] ?? "[]");
+const args = eval(process.argv[3] ?? "[]");


🧹 Nitpick | 🔵 Trivial

Intentional use of eval() for test expressions—consider adding a comment.

The switch from JSON.parse() to eval() is necessary to support test arguments containing JavaScript expressions like Math.fround(1) that cannot be represented in JSON. Since this is a test harness file with controlled inputs (only invoked by the test suite), the security risk is acceptable.

Consider adding a brief comment explaining why eval() is used:

-const args = eval(process.argv[3] ?? "[]"); +// eval() is needed to support JS expressions (e.g., Math.fround(1)) that JSON cannot represent +const args = eval(process.argv[3] ?? "[]");

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

const args = eval(process.argv[3] ?? "[]");

// eval() is needed to support JS expressions (e.g., Math.fround(1)) that JSON cannot represent

const args = eval(process.argv[3] ?? "[]");

🧰 Tools

🪛 Biome (2.1.2)

[error] 9-9: eval() exposes to security risks and performance issues.

See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().

(lint/security/noGlobalEval)

🤖 Prompt for AI Agents

In test/v8/v8-module/main.js around line 9, the use of eval(process.argv[3] ?? "[]") is intentional but lacks an explanatory comment; add a brief comment immediately above this line stating that eval is used intentionally to allow JavaScript expressions (e.g., Math.fround(1)) in test arguments, that inputs are controlled by the test suite, and that this is not a security risk in this context, leaving the eval call unchanged.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

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 46c51a2 and 7217f3a.

📒 Files selected for processing (1)

test/v8/v8.test.ts (5 hunks)

🧰 Additional context used

📓 Path-based instructions (4)

test/**/*.{js,ts,jsx,tsx}

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

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

test/v8/v8.test.ts

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

test/v8/v8.test.ts

**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

test/v8/v8.test.ts

test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

test/v8/v8.test.ts

🧠 Learnings (25)

📓 Common learnings

Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22278
File: src/ast/E.zig:980-1003
Timestamp: 2025-09-04T02:04:43.094Z
Learning: In Bun's Zig codebase, `as(i32, u8_or_u16_value)` is sufficient for casting u8/u16 to i32 in comparison operations. `intCast` is not required in this context, and the current casting approach compiles successfully.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Create test functions in test/v8/v8-module/main.cpp that take FunctionCallbackInfo<Value> parameter, use the test V8 API, print results for comparison with Node.js, and return Undefined

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use JSC intrinsics (prefixed with `$`) such as `$Array.from()`, `$isCallable()`, and `$newArrayWithSize()` for performance-critical operations

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts : Builtin functions must include `this` parameter typing in TypeScript to enable direct method binding in C++

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: See `test/harness.ts` for common test utilities and helpers

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : Organize source-map tests in sourcemap.test.ts for tests verifying source-maps are correct

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-06T00:58:23.965Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-08T04:06:33.198Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `-e` flag for single-file tests when spawning Bun processes

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-12-16T00:21:32.179Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-10-26T01:32:04.844Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8-module/main.cpp : Register new V8 API test functions in the Init method using NODE_SET_METHOD with exports object

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always check exit codes and test error scenarios in error tests

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `describe.each()` for parameterized tests

Applied to files:

test/v8/v8.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Tests should be located in the `test/` directory, organized by category: `cli/` for CLI tests, `js/` for JavaScript/TypeScript tests (further subdivided into `bun/`, `node/`, `web/`, `third_party/`), `napi/` for N-API tests, `v8/` for V8 C++ API tests, `bundler/` for bundler/transpiler tests, and `regression/issue/[number]` for regression tests

Applied to files:

test/v8/v8.test.ts

🧬 Code graph analysis (1)

test/v8/v8.test.ts (3)

test/v8/v8-module/main.cpp (2)

describe (37-68)

describe (37-37)

test/v8/v8-module/main.js (1)

args (9-9)

test/harness.ts (2)

isASAN (47-47)

isWindows (23-23)

🔇 Additional comments (4)

test/v8/v8.test.ts (4)

2-2: LGTM!

The import of jscDescribe is necessary for verifying the double-encoded value representation in the new test case.

145-163: LGTM!

The refactoring to it.each improves test organization and the new test cases comprehensively cover the edge cases mentioned in the PR objectives:

Double-encoded values via Math.fround(1)

INT32_MAX/MIN boundary cases

UINT32_MAX boundary cases

The string-based argument passing aligns well with the updated test harness that uses eval() in main.js.

326-326: LGTM!

The signature changes to accept string arguments that eval to arrays are well-designed:

Making args/jsArgs optional with a sensible default ("[]")

Clear JSDoc documentation explaining the expected format

Consistent application across both functions

This approach works well with the eval-based argument parsing in main.js.

Also applies to: 354-358, 375-375

128-128: LGTM!

All test call sites have been consistently updated to use the new signature. Tests that don't require arguments cleanly omit the optional parameter, while those that do (like line 257) correctly pass string representations.

Also applies to: 134-134, 167-167, 171-171, 174-174, 180-180, 184-184, 187-187, 190-190, 194-194, 201-201, 207-207, 213-213, 216-216, 219-219, 222-222, 227-227, 230-230, 233-233, 236-236, 239-239, 245-245, 251-251, 257-259, 262-262, 282-282, 288-288, 295-295, 303-303, 309-309, 315-315, 321-321

coderabbitai · 2025-12-17T07:26:06Z

test/v8/v8.test.ts

+    it("Math.fround returns a double-encoded value", () => {
+      // If this fails, you need to find a new way to make a JSValue which uses the double encoding
+      // but holds an int32 value (maybe Float64Array?)
+      expect(jscDescribe(Math.fround(1))).toBe("Double: 4607182418800017408, 1.000000");
+    });


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash # Find and examine the test file around the Math.fround test find . -name "v8.test.ts" -type f 2>/dev/null | head -5 # Look at the test file content, especially around lines 139-143 if [ -f "test/v8/v8.test.ts" ]; then echo "=== File content around Math.fround test ===" sed -n '120,160p' test/v8/v8.test.ts echo -e "\n=== Look for imports and jscDescribe usage ===" head -50 test/v8/v8.test.ts echo -e "\n=== Search for other tests using jscDescribe ===" grep -n "jscDescribe" test/v8/v8.test.ts | head -20 fi

Repository: oven-sh/bun

Length of output: 3362

Add Node.js comparison or clarify test intent. This test uses only jscDescribe() without comparing against Node.js output, breaking the pattern of other V8 tests in the same file that use checkSameOutput(). Either add a Node.js comparison via checkSameOutput() to validate the double-encoded behavior is consistent, or move this to a JSC-specific test if it's intentionally testing only JavaScriptCore's internal representation. The existing comment acknowledges brittleness but doesn't address why this test diverges from the established V8 testing pattern.

🤖 Prompt for AI Agents

In test/v8/v8.test.ts around lines 139 to 143, the Math.fround test uses only jscDescribe() and breaks the pattern of other V8 tests that call checkSameOutput(); update this test to either call checkSameOutput() with an equivalent Node.js invocation to verify the double-encoded behavior matches Node, or, if the intent is to validate a JSC-specific internal representation, move the test into a JSC-specific test file and keep it as-is but rename/annotate it to indicate it is JSC-only; implement the change by replacing the expect(jscDescribe(...)) assertion with a checkSameOutput() call that runs the same expression under Node.js and JSC, or relocating the test and updating test suite organization and comments accordingly.

Jarred-Sumner · 2025-12-17T08:52:26Z

Thanks

fix: v8::Value::IsInt32()/IsUint32() edge cases

46c51a2

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

[autofix.ci] apply automated fixes

7217f3a

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

Jarred-Sumner merged commit fa5a5bb into oven-sh:main Dec 17, 2025
51 of 54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

Uh oh!

190n commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Dec 17, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Dec 17, 2025

Uh oh!

Uh oh!

Jarred-Sumner commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	const args = eval(process.argv[3] ?? "[]");
	// eval() is needed to support JS expressions (e.g., Math.fround(1)) that JSON cannot represent
	const args = eval(process.argv[3] ?? "[]");

fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

fix: v8::Value::IsInt32()/IsUint32() edge cases #25548

Uh oh!

Conversation

190n commented Dec 17, 2025

What does this PR do?

How did you verify your code works?

Uh oh!

coderabbitai bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Pre-merge checks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jarred-Sumner commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Dec 17, 2025 •

edited

Loading