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

test: increase coverage of internal/util by DavidCai1111 · Pull Request #10964 · nodejs/node · GitHub

/ node Public

test: increase coverage of internal/util - #10964

Closed
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:master
ConversationCommits (1)ChecksFiles changed
Closed

test: increase coverage of internal/util#10964
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:master

Conversation

DavidCai1111 commented Jan 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Member
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 added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 23, 2017
cjihrig reviewed Jan 23, 2017
View reviewed changes
Comment thread test/parallel/test-internal-util-assertCrypto.js Outdated

cjihrig Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

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

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

cjihrig Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

The regex should include. ^ and $.

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

cjihrig Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

Why not use assert.doesNotThrow()?

DavidCai1111 commented Jan 23, 2017

Copy link
Copy Markdown
Member Author

@cjihrig OK, updated :)

mscdex added the util Issues and PRs related to the built-in util module. label Jan 23, 2017
cjihrig reviewed Jan 23, 2017
View reviewed changes
Comment thread test/parallel/test-internal-util-assertCrypto.js
@@ -0,0 +1,12 @@
// Flags: --expose-internals

cjihrig Jan 23, 2017

Copy link
Copy Markdown
Contributor

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

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 commented Jan 29, 2017

Copy link
Copy Markdown
Member

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

jasnell commented Jan 30, 2017

Copy link
Copy Markdown
Member

Failures in CI appear to be unrelated.

jasnell pushed a commit that referenced this pull request Jan 30, 2017
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 commented Jan 30, 2017

Copy link
Copy Markdown
Member

Landed in a7172b5

jasnell closed this Jan 30, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
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 mentioned this pull request Jan 31, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
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 commented Mar 8, 2017

Copy link
Copy Markdown
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 commented Mar 8, 2017

Copy link
Copy Markdown
Member

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

MylesBorins commented Apr 17, 2017

Copy link
Copy Markdown
Contributor

if this gets backported it should come with #11620

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
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.

8 participants

Back | FazBrowse Home | New Git URL