test: reuse ffi.suffix instead of reimplementing it - #64840
Open
leah-1ee wants to merge 1 commit into
Open
Conversation
ffi-test-common.js duplicated the platform branch already exposed as ffi.suffix, risking drift if lib/ffi.js changes. Reuse it, and require it after skipIfFFIMissing() so a build without FFI skips instead of throwing. Also assert ffi.suffix resolves to the correct value per platform, since test-ffi-module.js only checked that the key exists. Signed-off-by: leah-1ee <dltjddms1028@gmail.com>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
needs-ci
There was a problem hiding this comment.
Pull request overview
This PR removes duplicated platform-specific FFI library-suffix logic from the FFI test helpers by reusing the canonical ffi.suffix export, and strengthens test coverage by asserting the suffix value matches the current platform.
Changes:
- Replaced ad-hoc platform branching in
test/ffi/ffi-test-common.jswithrequire('node:ffi').suffix. - Added a dedicated test asserting
ffi.suffixisdll/dylib/sobased onprocess.platform.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/ffi/test-ffi-module.js | Adds an explicit assertion that ffi.suffix matches the expected platform-specific extension. |
| test/ffi/ffi-test-common.js | Uses the canonical ffi.suffix when building the fixture library path to avoid drift from lib/ffi.js. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64840 +/- ##
==========================================
+ Coverage 90.14% 90.15% +0.01%
==========================================
Files 746 746
Lines 242764 242764
Branches 45739 45741 +2
==========================================
+ Hits 218848 218876 +28
+ Misses 15426 15379 -47
- Partials 8490 8509 +19 🚀 New features to boost your workflow:
|
ShogunPanda
approved these changes
Jul 30, 2026
avivkeller
approved these changes
Jul 30, 2026
daeyeon
added
author ready
github-actions
Bot
removed
the
request-ci
Collaborator
Collaborator
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ffi-test-common.jsre-implemented the same branch thatffi.suffixalready exposes, risking drift from
lib/ffi.js.test-ffi-module.jsonly checked the key exists, not its value.
ffi.suffixinffi-test-common.jsinstead of re-implementing itffi.suffixmatches the expected value per platformRelated to #64805 (same fix for the docs)