Skip to content
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.

Sign up for GitHub

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

Jump to bottom

async_hooks: Adding regression test case for async/await #22374

Closed
wants to merge 1 commit into from
Closed

async_hooks: Adding regression test case for async/await #22374

wants to merge 1 commit into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Aug 17, 2018 •
edited
Loading

This #20274 (comment) is fixed by V8 update. This PR adds a regression test case.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 17, 2018
jasnell
jasnell reviewed Aug 17, 2018
View reviewed changes
test/parallel/test-async-hooks-async-await.js
@@ -0,0 +1,25 @@
'use strict';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a require('../common') as the first require

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be helpful to have a comment in here that explains what this is testing for.

jasnell
jasnell reviewed Aug 17, 2018
View reviewed changes
test/parallel/test-async-hooks-async-await.js Outdated
}).enable();

async function main() {
console.log('hello');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the console.log() statements are not required, then let's not have them. If they are required for the test, please add a comment so folks don't remove them

@BridgeAR
Copy link
Member

Refs: #20467

@antsmartian
Copy link
Contributor Author

@BridgeAR Oops, didn't notice that there was a PR already and got closed. May be I can back-port the same testing file here?

@antsmartian
Copy link
Contributor Author

@BridgeAR I just ran the test file https://raw.githubusercontent.com/BridgeAR/node/da7eaa8ef0fac2444f6892753c9db5cae39f6e1a/test/parallel/test-async-hooks-async-await-regression.js, looks like test is failing.

@antsmartian
Copy link
Contributor Author

Thanks @jasnell, addressed your comments.

@BridgeAR I'm not really sure why the test case that you have written over here : https://raw.githubusercontent.com/BridgeAR/node/da7eaa8ef0fac2444f6892753c9db5cae39f6e1a/test/parallel/test-async-hooks-async-await-regression.js, fails. But the test case included in this PR is working fine. Also, I ran same on node version 9 too (as the original issue claimed it stopped working from version 10). Things looks good to me.

Note: Test case from this PR: #20467, even fails on node version 9.

@BridgeAR
Copy link
Member

@antsmartian don't worry. I am not sure about the test anymore. It has been a while.

As far as I remember the console.log did have a purpose though.

@bmeurer @MayaLekova PTAL

@antsmartian antsmartian force-pushed the regression-test branch 2 times, most recently from 2dcdd60 to 5985e16 Compare August 17, 2018 17:03
@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/16606/

mcollina
mcollina approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joyeecheung
Copy link
Member

This PR needs a rebase against master to avoid the git failure in the CI.

@antsmartian
Copy link
Contributor Author

@joyeecheung Taken care.

@joyeecheung
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/16639/

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2018
jasnell
jasnell reviewed Aug 21, 2018
View reviewed changes
test/parallel/test-async-hooks-async-await.js Outdated
// asyncIds & triggerAsyncId for async-await
'use strict';

const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint issue: the common is never used. Just make this...

require('./common');

without assigning it to anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell: Taken care also, rebased with master. So CI should be green now.

@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/16676/

@mcollina
Copy link
Member

(the previous one is an infra failure and I doubt it will be resumable)

CI: https://ci.nodejs.org/job/node-test-pull-request/16681/

@mcollina
Copy link
Member

mcollina commented Aug 22, 2018 •
edited
Loading

@nodejs/build this is blocked on nodejs/build#1469.

@mcollina
Copy link
Member

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16691/

@mcollina
Copy link
Member

Landed in c8a27a7

@mcollina mcollina closed this Aug 23, 2018
mcollina pushed a commit that referenced this pull request Aug 23, 2018
@antsmartian @mcollina
async_hooks: adding regression test case for async/await
c8a27a7 
The actual bug was fixed by a V8 update in Node v10.4.0.

See: #19989
PR-URL: #22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@antsmartian
Copy link
Contributor Author

Thanks @mcollina

@antsmartian antsmartian deleted the regression-test branch August 23, 2018 08:26
@mcollina
Copy link
Member

You are welcome!

targos pushed a commit that referenced this pull request Aug 24, 2018
@antsmartian @targos
async_hooks: adding regression test case for async/await
634f856 
The actual bug was fixed by a V8 update in Node v10.4.0.

See: #19989
PR-URL: #22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
@antsmartian @targos
async_hooks: adding regression test case for async/await
e2a801a 
The actual bug was fixed by a V8 update in Node v10.4.0.

See: #19989
PR-URL: #22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@antsmartian @BridgeAR @mcollina @joyeecheung @jasnell @nodejs-github-bot