Skip to content

Fix main: replace jsg::Number with f64 in jsg-test tests - #8074

Closed
nathanwhit wants to merge 1 commit into
mainfrom
nathanwhit/fix-main-number
Closed

nathanwhit wants to merge 1 commit into
mainfrom
nathanwhit/fix-main-number

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

#8045 added uses of jsg::Number in the resource_template and fast_api tests, which #8054 removed in favor of f64.

#8045 added uses of jsg::Number in the resource_template and fast_api
tests, which #8054 removed in favor of f64.
@nathanwhit
nathanwhit requested review from a team as code owners October 28, 2026 20:16
@nathanwhit
nathanwhit enabled auto-merge October 28, 2026 20:17
@clanky-the-clanker

Copy link
Copy Markdown

✅ Approved — This is a small test-only fix that removes the last uses of the deleted jsg::Number type. f64 implements every trait those tests rely on, and no problems were found.

The decision is advisory: this comment never approves, blocks or requests changes on the pull request.


✅ All checks passed

📋 Code Quality: ✅ LGTM

The change replaces jsg::Number with f64 in fast_api.rs and resource_template.rs. No jsg::Number references are left under src/rust/jsg-test/tests.

  • Trait support: f64 implements ToJS and FromJS (src/rust/jsg/wrappable.rs), so it works with ctx.eval::<f64>, with Lenient<f64> and as a jsg_method argument or return type.
  • Fast API: f64 also implements FastApiArg and FastApiReturn (src/rust/jsg/fast_api.rs).
  • Assertion: the rewritten (result - 7.0).abs() < f64::EPSILON check means the same as the original assertion.

The tests were not built or run as part of this review.


📝 Documentation: ✅ LGTM

The changed test files contain no documentation or comment changes that need attention.


Reviewed commit: bf734db8

@ask-bonk

ask-bonk Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

LGTM!


Reviewed commit: bf734db8 · github run

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.42%. Comparing base (d68580d) to head (bf734db).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8074      +/-   ##
==========================================
- Coverage   69.49%   69.42%   -0.07%     
==========================================
  Files         695      696       +1     
  Lines      181564   181749     +185     
  Branches    29204    29211       +7     
==========================================
+ Hits       126170   126185      +15     
- Misses      38328    38498     +170     
  Partials    17066    17066              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nathanwhit

Copy link
Copy Markdown
Member Author

Fixed by #8072

@nathanwhit nathanwhit closed this Oct 28, 2026
auto-merge was automatically disabled October 28, 2026 20:44

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants