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: increase coverage of internal/util #10964

Closed
wants to merge 1 commit into from
Closed

test: increase coverage of internal/util #10964

wants to merge 1 commit into from

Conversation

DavidCai1111
Copy link
Member

@DavidCai1111 DavidCai1111 commented Jan 23, 2017 •
edited
Loading

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-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 23, 2017
cjihrig
cjihrig reviewed Jan 23, 2017
View reviewed changes
test/parallel/test-internal-util-assertCrypto.js

if (!process.versions.openssl) {
assert.throws(() => util.assertCrypto()
, /Node.js is not compiled with openssl crypto support/);
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.

cjihrig
cjihrig reviewed Jan 23, 2017
View reviewed changes
test/parallel/test-internal-util-assertCrypto.js

if (!process.versions.openssl) {
assert.throws(() => util.assertCrypto()
, /Node.js is not compiled with openssl crypto support/);
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 $.

cjihrig
cjihrig reviewed Jan 23, 2017
View reviewed changes
test/parallel/test-internal-util-assertCrypto.js
assert.throws(() => util.assertCrypto()
, /Node.js is not compiled with openssl crypto support/);
} else {
try {
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
Member Author

@cjihrig OK, updated :)

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Jan 23, 2017
cjihrig
cjihrig reviewed Jan 23, 2017
View reviewed changes
test/parallel/test-internal-util-assertCrypto.js
@@ -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

CI: https://ci.nodejs.org/job/node-test-commit/7533/

@jasnell
Copy link
Member

jasnell commented Jan 30, 2017

Failures in CI appear to be unrelated.

jasnell pushed a commit that referenced this pull request Jan 30, 2017
@DavidCai1111 @jasnell
test: increase coverage of internal/util
a7172b5 
PR-URL: #10964
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 30, 2017

Landed in a7172b5

@jasnell jasnell closed this Jan 30, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@DavidCai1111 @evanlucas
test: increase coverage of internal/util
65691d6 
PR-URL: #10964
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@DavidCai1111 @jasnell
test: increase coverage of internal/util
d462f25 
PR-URL: #10964
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@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

jasnell commented Mar 8, 2017

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@DavidCai1111 @addaleax @jasnell @MylesBorins @cjihrig @mscdex @nodejs-github-bot