FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

async_hooks: Adding regression test case for async/await by antsmartian · Pull Request #22374 · nodejs/node · GitHub

/ node Public

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

Closed
antsmartian wants to merge 1 commit into
nodejs:masterfrom
antsmartian:regression-test
ConversationCommits (1)ChecksFiles changed
Closed

async_hooks: Adding regression test case for async/await#22374
antsmartian wants to merge 1 commit into
nodejs:masterfrom
antsmartian:regression-test

Conversation

antsmartian commented Aug 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the test Issues and PRs related to the tests. label Aug 17, 2018
jasnell reviewed Aug 17, 2018
View reviewed changes
Comment thread test/parallel/test-async-hooks-async-await.js Outdated

jasnell Aug 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell Aug 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell reviewed Aug 17, 2018
View reviewed changes
Comment thread test/parallel/test-async-hooks-async-await.js Outdated

jasnell Aug 17, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 17, 2018

Copy link
Copy Markdown
Member

Refs: #20467

antsmartian commented Aug 17, 2018

Copy link
Copy Markdown
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 commented Aug 17, 2018

Copy link
Copy Markdown
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 commented Aug 17, 2018

Copy link
Copy Markdown
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 commented Aug 17, 2018

Copy link
Copy Markdown
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 force-pushed the regression-test branch 2 times, most recently from 2dcdd60 to 5985e16 Compare August 17, 2018 17:03

mcollina commented Aug 20, 2018

Copy link
Copy Markdown
Member

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

mcollina approved these changes Aug 20, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

joyeecheung commented Aug 20, 2018

Copy link
Copy Markdown
Member

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

antsmartian commented Aug 21, 2018

Copy link
Copy Markdown
Contributor Author

@joyeecheung Taken care.

joyeecheung commented Aug 21, 2018

Copy link
Copy Markdown
Member

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

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 21, 2018
jasnell reviewed Aug 21, 2018
View reviewed changes
Comment thread test/parallel/test-async-hooks-async-await.js Outdated

jasnell Aug 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

require('./common');

without assigning it to anything.

antsmartian Aug 22, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina commented Aug 22, 2018

Copy link
Copy Markdown
Member

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

mcollina commented Aug 22, 2018

Copy link
Copy Markdown
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 commented Aug 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

mcollina commented Aug 22, 2018

Copy link
Copy Markdown
Member

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

mcollina commented Aug 23, 2018

Copy link
Copy Markdown
Member

Landed in c8a27a7

mcollina closed this Aug 23, 2018
mcollina pushed a commit that referenced this pull request Aug 23, 2018
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 commented Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

Thanks @mcollina

antsmartian deleted the regression-test branch August 23, 2018 08:26

mcollina commented Aug 23, 2018

Copy link
Copy Markdown
Member

You are welcome!

targos pushed a commit that referenced this pull request Aug 24, 2018
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
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 mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
mcollina mcollina approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

6 participants

Back | FazBrowse Home | New Git URL