Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/bun.js/test/Execution.zig
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,8 @@ fn onSequenceStarted(_: *Execution, sequence: *ExecutionSequence) void {
sequence.started_at = bun.timespec.now();

if (sequence.test_entry) |entry| {
log("Running test: \"{}\"", .{std.zig.fmtEscapes(entry.base.name orelse "(unnamed)")});

if (entry.base.test_id_for_debugger != 0) {
if (jsc.VirtualMachine.get().debugger) |*debugger| {
if (debugger.test_reporter_agent.isEnabled()) {
Expand Down Expand Up @@ -619,6 +621,8 @@ pub fn handleUncaughtException(this: *Execution, user_data: bun_test.BunTest.Ref
};
}

const log = bun.Output.scoped(.jest, .visible);

Comment on lines +624 to +625

@coderabbitai coderabbitai Bot Oct 18, 2025

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.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the visibility level: should this be .hidden instead of .visible?

According to the coding guideline and the PR description, debug logs should use .hidden to gate them behind the BUN_DEBUG_jest=1 environment variable. The current implementation uses .visible, which may cause these logs to always appear rather than only when the environment variable is set.

Based on learnings

Apply this diff if .hidden is the correct visibility level:

-const log = bun.Output.scoped(.jest, .visible);
+const log = bun.Output.scoped(.jest, .hidden);

Alternatively, please confirm whether .visible is intentional and explain how the BUN_DEBUG_jest=1 environment variable gates the output with this visibility setting.


I need to investigate the codebase to verify the correct visibility level for the logger.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find examples of bun.Output.scoped usage in Zig files to identify the correct pattern
rg -n 'bun\.Output\.scoped' --type zig -B2 -A2

Length of output: 39967


🏁 Script executed:

#!/bin/bash
# Search for BUN_DEBUG environment variable usage to understand gating behavior
rg -n 'BUN_DEBUG' --type zig -B3 -A3 | head -100

Length of output: 6820


🏁 Script executed:

#!/bin/bash
# Check if there's documentation or configuration for visibility levels
rg -n '\.hidden|\.visible' --type zig | grep -i output | head -50

Length of output: 4349


🏁 Script executed:

#!/bin/bash
# Look at the context around line 624 in the file under review to confirm the scope
sed -n '620,630p' src/bun.js/test/Execution.zig

Length of output: 207


🏁 Script executed:

#!/bin/bash
# Search for other loggers declared in Execution.zig or nearby test files
rg -n 'const log = ' src/bun.js/test/ --type zig -B1 -A1

Length of output: 151


Change .visible to .hidden to enable environment variable gating.

The logger at line 624 currently uses .visible, which makes logs always appear. However, the PR states logging should be "enabled by the environment variable BUN_DEBUG_jest=1". According to src/output.zig (lines 767-773), this behavior requires .hidden visibility. Additionally, src/cli/test/Scanner.zig:18 already declares a jest-scoped logger with .hidden, confirming the correct pattern for this module.

-const log = bun.Output.scoped(.jest, .visible);
+const log = bun.Output.scoped(.jest, .hidden);
📝 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 log = bun.Output.scoped(.jest, .visible);
const log = bun.Output.scoped(.jest, .hidden);
🤖 Prompt for AI Agents
In src/bun.js/test/Execution.zig around lines 624-625, the logger is created
with bun.Output.scoped(.jest, .visible) which forces logs to always appear;
change the visibility argument from .visible to .hidden so the jest-scoped
logger respects the environment gating (BUN_DEBUG_jest=1) as implemented in
src/output.zig and consistent with src/cli/test/Scanner.zig: use
bun.Output.scoped(.jest, .hidden) instead.

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.

Visible means it is enabled by default in debug builds unless BUN_DEBUG_QUIET_LOGS=1 is set, which is what we want. If that is set, then it is only enabled with BUN_DEBUG_jest=1

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.

@pfgithub Thank you for the clarification! I understand now—.visible is indeed the correct choice here. It enables the logs by default in debug builds, while still respecting BUN_DEBUG_QUIET_LOGS=1 to silence them, and BUN_DEBUG_jest=1 to re-enable them when quieted.

I appreciate you taking the time to explain the distinction between .visible and .hidden for scoped loggers. This is helpful context for understanding the debug logging system.

    /)_/)
   (o.o)  oops, my mistake!
   (   )

✏️ Learnings added
Learnt from: pfgithub
PR: oven-sh/bun#23796
File: src/bun.js/test/Execution.zig:624-625
Timestamp: 2025-10-18T02:06:31.595Z
Learning: In Zig files using bun.Output.scoped(), the visibility level `.visible` means logs are enabled by default in debug builds unless `BUN_DEBUG_QUIET_LOGS=1` is set; if that environment variable is set, the logs can be re-enabled with `BUN_DEBUG_<scope>=1`. Use `.visible` for logs that should appear by default in debug builds, and `.hidden` for logs that require explicit opt-in via `BUN_DEBUG_<scope>=1`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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 : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions

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 : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Use BUN_DEBUG_QUIET_LOGS=1 to silence debug logs or BUN_DEBUG_<scope>=1 to enable scoped logs in debug builds

const std = @import("std");
const test_command = @import("../../cli/test_command.zig");

Expand Down