-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
test: async iife in repl #44878
base: main
Are you sure you want to change the base?
test: async iife in repl #44878
Conversation
28b8f7f
to
096676e
Compare
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.
Should we use Closes
or Fixes
in the PR description instead of Ref
so that merging this PR automatically closes the issue?
'use strict'; | ||
require('../common'); | ||
|
||
// Note: This test ensures that async IIFE doesn't crash |
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.
Do you know the Node.js version for which this test would crash? It only affects debug builds, so it would probably require building Node.js locally for confirming that this is actually a regression test for the issue.
Seeing #38685 (comment) it appears that it got fixed later on, so I'm curious what the fix was.
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.
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.
it affected the master
branch in May 2021, so I would try with v14.0.0.
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.
Thanks @targos :)
Should I checkout the v14 branch and run the test it and see if it fails?
Hey 👋🏼 I could not make the test fail on Should I close it so? cc @RaisinTen |
Nvm, I didn't test with debug build :/ |
I finally build from sources for ➜ ./node
Welcome to Node.js v14.0.0.
Type ".help" for more information.
> (async() => { })()
Promise { undefined }
> ➜ ./node
Welcome to Node.js v17.0.0.
Type ".help" for more information.
> (async() => { })()
Promise {
undefined,
[Symbol(async_id_symbol)]: 26,
[Symbol(trigger_async_id_symbol)]: 5,
[Symbol(destroyed)]: { destroyed: false }
}
> Maybe I should try on my Fedora machine... |
@tony-go maybe you can try to spawn a subprocess with |
Yeah, good idea @aduh95 :) I just wonder if the PR still makes sense as I was not able to reproduce it: #44878 (comment) Even if I think that it is sill relevant to have tests anyway? WDYT? |
@tony-go what I meant was that maybe you would be able to reproduce on v14.x if you were using |
Okay I did not get it this way, but yeah I'll try this :) |
Hey @aduh95 👋🏼 I could not make it crash on version 14 and the :/ |
No updates on this in over a year. Needs to be revisited. Removed the author ready label. |
Context
Fixes: #38685
The previous PR has more than ten months, it's why I decided to jump on it.
Improvements
A regression test ensures that async IFEE will work on the REPL.
cc @addaleax