Skip to content

Conversation

@DavidCai1111
Copy link
Member

@DavidCai1111DavidCai1111 commented Jan 23, 2017

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

test

@nodejs-github-botnodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 23, 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this pass make lint? The comma should be on the previous line.

Copy link
Contributor

Choose a reason for hiding this comment

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

The regex should include. ^ and $.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use assert.doesNotThrow()?

@DavidCai1111
Copy link
MemberAuthor

@cjihrig OK, updated :)

@mscdexmscdex added the util Issues and PRs related to the built-in util module. label Jan 23, 2017
@@ -0,0 +1,12 @@
// Flags: --expose-internals
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, this test won't actually increase test coverage reported at coverage.nodejs.org unless it is run with Node without crypto. And then, it would lose coverage on the else condition.

@addaleax
Copy link
Member

@jasnell
Copy link
Member

Failures in CI appear to be unrelated.

jasnell pushed a commit that referenced this pull request Jan 30, 2017
PR-URL: #10964 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

Landed in a7172b5

@jasnelljasnell closed this Jan 30, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
PR-URL: #10964 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@italoacasasitaloacasas mentioned this pull request Jan 31, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #10964 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

I had to revert this from v6.x-staging due to an error

=== release test-internal-util-decorate-error-stack === Path: parallel/test-internal-util-decorate-error-stack /Users/mborins/code/node/v6.x/test/parallel/test-internal-util-decorate-error-stack.js:70 internalUtil.setHiddenValue(err, kArrowMessagePrivateSymbolIndex, arrowMessage); ^ TypeError: name must be a string at TypeError (native) at Object.<anonymous> (/Users/mborins/code/node/v6.x/test/parallel/test-internal-util-decorate-error-stack.js:70:14) at Module._compile (module.js:570:32) at Object.Module._extensions..js (module.js:579:10) at Module.load (module.js:487:32) at tryModuleLoad (module.js:446:12) at Function.Module._load (module.js:438:3) at Module.runMain (module.js:604:10) at run (bootstrap_node.js:394:7) at startup (bootstrap_node.js:149:9) Command: out/Release/node --expose_internals /Users/mborins/code/node/v6.x/test/parallel/test-internal-util-decorate-error-stack.js 

It would appear that const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol']; is returning undefined. Any ideas?

@jasnell
Copy link
Member

Odd... I was certain I had run that test locally. Likely missing another commit. This one is likely safe to omit

@MylesBorins
Copy link
Contributor

if this gets backported it should come with #11620

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@DavidCai1111@addaleax@jasnell@MylesBorins@cjihrig@mscdex@nodejs-github-bot@davidtaikocha