fix(cli): improve test output readability by graying result lines (#24071) #31474
Conversation
This commit refactors `QuotaExceededError` to be a proper subclass of `DOMException`, aligning with the WHATWG WebIDL update. Previously, `QuotaExceededError` was a `DOMException` instance with a specific name.
The changes involve:
- Removing legacy `QuotaExceededError` handling from `ext/web/01_dom_exception.js`.
- Introducing a new `QuotaExceededError` class in `ext/web/02_quota_exceeded_error.js` that extends `DOMException`.
- Updating `ext/web/lib.rs` to include the new JavaScript file in the build.
- Exposing `QuotaExceededError` globally in `runtime/js/98_global_scope_shared.js`.
- Modifying the error builder in `runtime/js/99_main.js` to instantiate the new `QuotaExceededError` class.
- Updating TypeScript definitions in `cli/tsc/dts/lib.deno_web.d.ts` to reflect the new class.
This ensures `QuotaExceededError` has the correct prototype chain and improves adherence to web standards. Existing `new DOMException("message", "QuotaExceededError")` calls will still function, but `code` will be 0 as it's no longer mapped directly in DOMException.
Remove incorrectly added and its reference from . is an instance of with a specific name, not a separate class. This fixes CI failures related to Web API error handling and snapshot generation.
…noland#24071) Make test output easier to scan by applying gray coloring to the status and timing portion of test results. This reduces visual clutter while maintaining visibility for failures. Changes: - For passing/ignored/cancelled tests: entire result line is grayed - For failed tests: status remains colored, timing is grayed - Applied consistently to both main tests and test steps Also includes minor whitespace formatting fix in ext/web/lib.rs. Fixes denoland#24071
WalkthroughThis pull request introduces a dedicated Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crypto/lib.rs (1)
211-215: This change is incorrect and would break functionality.The proposed binding change from
#[class("DOMExceptionQuotaExceededError")]to#[class("QuotaExceededError")]references a class that does not exist. The only registered error builder isDOMExceptionQuotaExceededError, which is defined inruntime/js/99_main.jsas a constructor that returnsnew DOMException(msg, "QuotaExceededError"). The raw class nameQuotaExceededErroris not exported or available in the global scope.Keep the binding as
#[class("DOMExceptionQuotaExceededError")]to maintain consistency withext/webstorage/lib.rsand the error registration system.
🧹 Nitpick comments (3)
cli/tools/test/reporters/pretty.rs (1)
122-164: Implementation looks correct for step result formatting.The gray coloring is properly applied to Ok/Ignored results while Failed tests retain their visibility. One minor simplification opportunity: on line 151, since
failureis already destructured from the match arm, you could use:- if !matches!(result, TestStepResult::Failed(TestFailure::Incomplete)) { + if !matches!(failure, TestFailure::Incomplete) {This avoids re-matching on
resultwhenfailureis already available.ext/web/lib.rs (1)
115-116: Consider removing the blank line for consistency.The blank line after the new entry is inconsistent with the rest of the
esmarray formatting, where entries are listed consecutively without blank lines."01_dom_exception.js", "02_quota_exceeded_error.js", - "01_mimesniff.js",cli/tsc/dts/lib.deno_web.d.ts (1)
70-71: Consider adding JSDoc category annotation for consistency.Other declarations in this file include
@category Platformor similar annotations. For consistency:+/** @category Platform */ declare class QuotaExceededError extends DOMException {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cli/tools/test/reporters/pretty.rs(2 hunks)cli/tsc/dts/lib.deno_web.d.ts(1 hunks)ext/crypto/lib.rs(1 hunks)ext/web/01_dom_exception.js(0 hunks)ext/web/02_quota_exceeded_error.js(1 hunks)ext/web/lib.rs(1 hunks)ext/webstorage/lib.rs(1 hunks)runtime/js/98_global_scope_shared.js(2 hunks)runtime/js/99_main.js(0 hunks)tests/unit/quota_exceeded_error_test.ts(1 hunks)tests/unit/webstorage_test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- runtime/js/99_main.js
- ext/web/01_dom_exception.js
🧰 Additional context used
📓 Path-based instructions (4)
ext/**/lib.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions should provide ops (operations) exposed to JavaScript in Rust code within
ext/<extension_name>/directories
Files:
ext/webstorage/lib.rsext/crypto/lib.rsext/web/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/webstorage/lib.rsext/crypto/lib.rsext/web/lib.rscli/tools/test/reporters/pretty.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/web/02_quota_exceeded_error.jsruntime/js/98_global_scope_shared.jstests/unit/quota_exceeded_error_test.tstests/unit/webstorage_test.tscli/tsc/dts/lib.deno_web.d.ts
cli/tools/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI tools should be implemented in
cli/tools/<tool>orcli/tools/<tool>/mod.rs
Files:
cli/tools/test/reporters/pretty.rs
🧠 Learnings (1)
📚 Learning: 2025-11-20T09:12:20.198Z
Learnt from: marvinhagemeister
Repo: denoland/deno PR: 31358
File: ext/node/polyfills/process.ts:759-766
Timestamp: 2025-11-20T09:12:20.198Z
Learning: In ext/node/polyfills/process.ts, the process.sourceMapsEnabled property should be defined with enumerable: true to match Node.js behavior (as seen in Node.js source: lib/internal/bootstrap/node.js).
Applied to files:
runtime/js/98_global_scope_shared.js
🧬 Code graph analysis (2)
ext/web/02_quota_exceeded_error.js (1)
ext/web/01_dom_exception.js (4)
primordials(11-23)_name(28-28)_message(29-29)_code(30-30)
cli/tools/test/reporters/pretty.rs (2)
tests/util/server/src/lib.rs (1)
gray(1277-1279)cli/util/display.rs (1)
human_elapsed(51-53)
🪛 Biome (2.1.2)
ext/web/02_quota_exceeded_error.js
[error] 12-12: Do not shadow the global "Symbol" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ 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). (7)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: build libs
🔇 Additional comments (11)
cli/tools/test/reporters/pretty.rs (1)
304-351: Test result formatting implementation is consistent and correct.The gray coloring pattern is properly applied: Ok, Ignored, and Cancelled results are fully grayed, while Failed results keep the status label prominent and only gray the timing portion. This aligns with the PR objectives for improved readability.
ext/webstorage/lib.rs (1)
26-28: Error class mapping updated correctly.The change from
"DOMExceptionQuotaExceededError"to"QuotaExceededError"aligns with the new dedicated error class. This ensures consistency with the JS-sideQuotaExceededErrorclass definition.tests/unit/webstorage_test.ts (1)
17-42: Test updates correctly validate QuotaExceededError behavior.The pattern of catching
DOMExceptionand then verifyingerr.name === "QuotaExceededError"is appropriate. SinceQuotaExceededErrorextendsDOMException, both approaches work, but this explicit name check clearly documents the expected error type.runtime/js/98_global_scope_shared.js (2)
31-31: LGTM!The import follows the established pattern for web API primitives.
69-69: LGTM!The global exposure follows the correct pattern for web APIs, using
propNonEnumerableconsistently with other error classes likeDOMException.tests/unit/quota_exceeded_error_test.ts (3)
5-9: LGTM!Correctly verifies the inheritance chain.
11-15: LGTM!Correctly verifies the default name and code (22 = QUOTA_EXCEEDED_ERR per spec).
17-22: LGTM!Correctly tests the constructor with a custom message.
ext/web/02_quota_exceeded_error.js (3)
10-19: LGTM! Static analysis warning is a false positive.The import pattern matches the existing
DOMExceptionimplementation. ImportingSymbolfrom primordials is the standard practice in Deno to prevent prototype pollution attacks.
21-29: LGTM!The class correctly extends
DOMExceptionwith the proper name and code (22 = QUOTA_EXCEEDED_ERR). The constructor correctly delegates to the parent with the type parameter.
31-33: LGTM!Interface configuration and export are correct.
| Deno.test("new DOMException('...', 'QuotaExceededError')", () => { | ||
| const err = new DOMException("The quota has been exceeded.", "QuotaExceededError"); | ||
| assert(err instanceof DOMException); | ||
| assertEquals(err.name, "QuotaExceededError"); | ||
| assertEquals(err.message, "The quota has been exceeded."); | ||
| // The code will be 0 when using the DOMException constructor with a name | ||
| // that is not in the nameToCodeMapping. This is expected. | ||
| assertEquals(err.code, 0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "nameToCodeMapping|QuotaExceededError" ext/web/01_dom_exception.js -A 10 -B 2Repository: denoland/deno
Length of output: 1660
Fix incorrect code assertion in test.
The test expects err.code to be 0, but QuotaExceededError is present in the nameToCodeMapping (line 84 of ext/web/01_dom_exception.js) and maps to QUOTA_EXCEEDED_ERR. The constructor logic (line 109) retrieves the mapped code or defaults to 0 only for unmapped names. Since QuotaExceededError is mapped, the code should be QUOTA_EXCEEDED_ERR, not 0. Either correct the assertion or update the test comment to reflect the actual behavior.
🤖 Prompt for AI Agents
In tests/unit/quota_exceeded_error_test.ts around lines 24-32, the test
incorrectly asserts err.code is 0 despite "QuotaExceededError" being present in
the nameToCodeMapping; update the assertion to expect the mapped
QUOTA_EXCEEDED_ERR value (e.g. assertEquals(err.code, QUOTA_EXCEEDED_ERR) or
assertEquals(err.code, DOMException.QUOTA_EXCEEDED_ERR) depending on available
symbol) and update the comment to explain that the constructor maps the name to
the corresponding QUOTA_EXCEEDED_ERR code rather than defaulting to 0.
There was a problem hiding this comment.
These changes are unrelated to the test output
2 participants
This PR addresses issue #24071 by improving the readability of the test reporter output. It modifies the
PrettyTestReporter
to apply gray coloring to the result portion of the test output (status and timing).
As requested in the issue, this change reduces visual clutter by making the "ok" status and timing information less prominent for passing tests, allowing users to more easily scan through the list of test names.
Changes
Passing / Ignored / Cancelled Tests: The entire result line (e.g., ok (1ms)) is now printed in gray.
Failed Tests: The status label (FAILED) remains colored (red/bold) to maintain high visibility, while the timing information is grayed.
Test Steps: The same formatting logic is applied to nested test steps for consistency.
Visual Example
Before:
test name ... ok (1ms)
(All text has the same visual weight)
After:
test name ... ok (1ms)
(The ok (1ms) portion is grayed out)
Verification
Ran cargo fmt to ensure code style compliance.
Verified that the changes only affect ANSI color codes and do not alter the actual text content of the output.
Existing test snapshots should pass as they strip ANSI codes before comparison.
Fixes #24071