-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix napi_get_value_string_utf8
to match node
#7175
Conversation
|
||
function checkSameOutput(test: string, args: any[]) { | ||
const nodeResult = runOn("node", test, args).trim(); | ||
let bunResult = runOn(join(__dirname, "../../build/bun-debug"), test, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let bunResult = runOn(join(__dirname, "../../build/bun-debug"), test, args); | |
let bunResult = runOn(bunExe(), test, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do this locally, tests fail again as if it was run on the current bun version... is bunExe()
supposed to refer to the version of bun built by the pipeline ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm. yes, is bun-debug
available in $PATH
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but it's available as bug-debug
, not bun
. Thus, these tests only pass locally if I run them via bun run build && bun-debug test test/napi
(with a lot of noisy logs, obviously).
If that's the executable that runs on your test pipeline instead of the current version of bun, then fine, lets commit that 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests pass locally with process.execPath
in use and they will not pass in CI without that.
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Thank you for this. I'm going to merge it and then get it running in CI a follow-up PR. |
Thanks ! |
* fix napi_get_value_string_utf8 to match node closes oven-sh#6949 * [autofix.ci] apply automated fixes * Update test/napi/napi.test.ts Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> * Update test/napi/napi.test.ts Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
closes #6949
What does this PR do?
As described in #6949, the
napi_get_value_string_utf8
function does not match Nodejs NAPI behaviour.This PR:
How did you verify your code works?
I believe NAPI is not unit tested at all.
In this PR, I'm proposing a way to test coherence between bun's NAPI & node's.
It consists of a NAPI node/bun application (in
test/napi/napi-app
) that can be called by unit tests.When tests run, it:
./build/bun-debug
executable)This is obviously a very opiniated way of testing things, so I'd totaly understand if this not OK.
As a side-note, I did not check wether if this is OK with your build pipeline, as it requires both
node
andyarn
to be available in path.I ran those tests locally using
bun run build && bun test test/napi
.Before fixes:
After fixes:
bun-debug test test-file-name.test
)