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

test: Refactors test/parallel/test-async-wrap-post-did-throw.js #8625

Closed
wants to merge 2 commits into from
Closed

test: Refactors test/parallel/test-async-wrap-post-did-throw.js #8625

wants to merge 2 commits into from

Conversation

graphicbeacon
Copy link
Contributor

Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

?

Description of change

This changes var to let in test/parallel/test-async-wrap-post-did-throw.js. Replaces equal method on assert module to strictEqual.

@graphicbeacon
test: Changes var to let in parallel/test-async-wrap-post-did-throw.j…
747ed7f 
…s. Replaces equal method on assert module to strictEqual.
@graphicbeacon
Merge commit 'a89c23f8ebb8d47e2f577d3594bfb465c1861d25'
6b51f93 
* commit 'a89c23f8ebb8d47e2f577d3594bfb465c1861d25': (30 commits)
  test: improve test-https-agent.js
  test: fix test-debug-signal-cluster.js flakyness
  benchmark: ignore significance when using --runs 1
  inspector: fix tests on Windows
  doc/json: make sure links are correctly passed to marked
  repl: improve .help message
  test: make test-tick-processor.js non-flaky
  doc: link onboarding to contributing guide
  test: add coverage for client._addHandle()
  2016-09-14, Version 6.6.0 (Current)
  doc: remove duplicate content from readline doc
  timers: remove unreachable code
  doc: capitalize arguments' type names in url doc
  doc: add gibfahn to collaborators
  doc: add imyller to collaborators
  doc: standardize rest parameters
  test: fix flaky test-force-repl
  inspector: listen on process.debugPort
  doc: add not-an-aardvark to collaborators
  doc: link SIGTSTP / SIGCONT events in readline doc
  ...
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2016

LGTM. Would you care to squash the commits?

@mscdex mscdex added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap labels Sep 17, 2016
@Fishrock123
Copy link
Member

Probably not worthwhile if it conflicts with #8531 in any significant way?

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

I'd say it's still worthwhile even if the test ends up getting refactored by #8531. This is one of the commits coming from the code and learn over the weekend and the primary intent is to not only help improve the codebase but help new contributors through the process of getting a PR landed. I'm sure that any potential conflict that arises from this PR is likely quite minimal.

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

addaleax
addaleax approved these changes Sep 21, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM. Right now this doesn’t conflict with the async-hooks PR anyway.

lpinca
lpinca approved these changes Sep 22, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Sep 22, 2016
@graphicbeacon @jasnell
test: fixup parallel/test-async-wrap-post-did-throw.js
0a9f56a 
* change var to let
* replace equal with strictEqual

PR-URL: #8625
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 22, 2016

Landed in 0a9f56a. Thank you!

@jasnell jasnell closed this Sep 22, 2016
@graphicbeacon
Copy link
Contributor Author

Thank you all for your help. Really appreciated.

jasnell pushed a commit that referenced this pull request Sep 29, 2016
@graphicbeacon @jasnell
test: fixup parallel/test-async-wrap-post-did-throw.js
0fbd59e 
* change var to let
* replace equal with strictEqual

PR-URL: #8625
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@graphicbeacon @Fishrock123
test: fixup parallel/test-async-wrap-post-did-throw.js
1b494d3 
* change var to let
* replace equal with strictEqual

PR-URL: #8625
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. 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
9 participants
@graphicbeacon @cjihrig @Fishrock123 @jasnell @addaleax @lpinca @mscdex @MylesBorins @nodejs-github-bot