test: add util inspect null getter test by mihan007 · Pull Request #27884 · nodejs/node

mihan007 · 2019-05-26T10:36:34Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

ryzokuken

addaleax · 2019-05-26T12:14:27Z

@mihan007 Could you take a look at the linter failures?

/home/travis/build/nodejs/node/test/parallel/test-util-inspect.js
  342:50  error  Missing semicolon               semi
  344:28  error  A space is required after '{'   object-curly-spacing
  344:42  error  A space is required before '}'  object-curly-spacing

mihan007 · 2019-05-26T12:28:28Z

@addaleax fixed

nodejs-github-bot · 2019-05-26T12:59:15Z

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

ryzokuken · 2019-05-28T17:21:33Z

Failure:

18:32:50 /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/obj.target/v8_base/deps/v8/src/bootstrapper.o:(.toc1+0x218): undefined reference to `v8::internal::StatisticsExtension::kSource'
18:32:50 /home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/obj.target/v8_base/deps/v8/src/bootstrapper.o:(.toc1+0x228): undefined reference to `vtable for v8::internal::StatisticsExtension'
18:32:51 collect2: error: ld returned 1 exit status
18:32:51 make[2]: *** [/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcle-ubuntu1404/out/Release/mksnapshot] Error 1

Completely unrelated to the PR, I guess?

nodejs-github-bot · 2019-05-28T21:52:58Z

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

ryzokuken · 2019-05-29T09:28:01Z

The tests pass but there's a... performance regression?

Trott · 2019-05-29T16:27:16Z

The tests pass but there's a... performance regression?

@ryzokuken Can you elaborate on that? I'm not sure what you're referring to but I am certainly curious.

ryzokuken · 2019-05-29T16:45:36Z

@Trott according to https://ci.nodejs.org/job/node-test-commit/28968/, all tests passed, but https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_debug_x64/13006/testReport/junit/(root)/test/sequential_test_performance/ reports a performance regression.

Trott · 2019-05-30T06:29:42Z

@Trott according to https://ci.nodejs.org/job/node-test-commit/28968/, all tests passed, but https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_debug_x64/13006/testReport/junit/(root)/test/sequential_test_performance/ reports a performance regression.

Ah! test-performance isn't a performance regression test. It's a test of the perf_hooks module.

ryzokuken · 2019-05-30T09:33:55Z

@Trott I see! Sorry for misunderstanding.

Trott · 2019-05-30T09:40:06Z

@Trott I see! Sorry for misunderstanding.

No worries! The only reason I know that is because I made the same exact mistake in the past! The test might benefit from being renamed test-perf-hooks

ryzokuken · 2019-05-30T09:50:16Z

@Trott I could make a PR 🤷‍♂

Rename test-performance to test-perf-hooks to better match its function Refs: nodejs#27884 (comment)

nodejs-github-bot · 2019-05-30T13:47:33Z

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

PR-URL: nodejs#27884 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Trott · 2019-05-30T14:36:34Z

Landed in 1dbf276

Trott · 2019-05-30T14:36:47Z

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

PR-URL: #27884 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Rename test-performance to test-perf-hooks to better match its function Refs: #27884 (comment) PR-URL: #27969 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

test: add util inspect null getter test

ca44748

nodejs-github-bot added the test Issues and PRs related to the tests. label May 26, 2019

ryzokuken added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label May 26, 2019

ryzokuken approved these changes May 26, 2019

View reviewed changes

addaleax approved these changes May 26, 2019

View reviewed changes

test: add util inspect null getter test

0fb6065

antsmartian approved these changes May 26, 2019

View reviewed changes

BridgeAR approved these changes May 26, 2019

View reviewed changes

cjihrig approved these changes May 26, 2019

View reviewed changes

Trott approved these changes May 27, 2019

View reviewed changes

ChALkeR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2019

ChALkeR approved these changes May 27, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

ryzokuken added a commit to ryzokuken/node that referenced this pull request May 30, 2019

test: rename test-performance to test-perf-hooks

e755f75

Rename test-performance to test-perf-hooks to better match its function Refs: nodejs#27884 (comment)

ryzokuken mentioned this pull request May 30, 2019

test: rename test-performance to test-perf-hooks #27969

Closed

4 tasks

This comment has been minimized.

Sign in to view

Trott closed this May 30, 2019

targos mentioned this pull request Jun 3, 2019

v12.4.0 release proposal #28040

Merged

kissofire mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities kissofire/node#73

Open

wedataintelligence mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities wedataintelligence/node#38

Open

ron-a20 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities ron-a20/node#71

Open

Dmarch28 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities Dmarch28/node#62

Open

aliceUnhinged613 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities aliceUnhinged613/node#64

Open

137717unity mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities 137717unity/node#62

Open

baby636 mentioned this pull request Jul 22, 2022

[Snyk] Fix for 1 vulnerabilities baby636/node#61

Open

molllyn1 mentioned this pull request Jul 23, 2022

[Snyk] Fix for 1 vulnerabilities molllyn1/node#53

Open

Uh oh!

Conversation

mihan007 commented May 26, 2019

Checklist

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 26, 2019

Uh oh!

mihan007 commented May 26, 2019

Uh oh!

nodejs-github-bot commented May 26, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

ryzokuken commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 28, 2019

Uh oh!

ryzokuken commented May 29, 2019

Uh oh!

Trott commented May 29, 2019

Uh oh!

ryzokuken commented May 29, 2019

Uh oh!

Trott commented May 30, 2019

Uh oh!

ryzokuken commented May 30, 2019

Uh oh!

Trott commented May 30, 2019

Uh oh!

ryzokuken commented May 30, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 30, 2019

Uh oh!

Trott commented May 30, 2019

Uh oh!

Trott commented May 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Comments

ryzokuken commented May 28, 2019 •

edited

Loading