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: add util inspect null getter test #27884

Closed
wants to merge 2 commits into from
Closed

test: add util inspect null getter test #27884

wants to merge 2 commits into from

Conversation

mihan007
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 26, 2019
@ryzokuken 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
ryzokuken approved these changes May 26, 2019
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM. /cc @BridgeAR

@addaleax
Copy link
Member

@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
Copy link
Contributor Author

@addaleax fixed

@nodejs-github-bot
Copy link
Collaborator

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

@ChALkeR 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
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ryzokuken
Copy link
Contributor

ryzokuken commented May 28, 2019 •
edited
Loading

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
Copy link
Collaborator

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

@ryzokuken
Copy link
Contributor

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

@Trott
Copy link
Member

Trott commented May 29, 2019

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
Copy link
Contributor

@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
Copy link
Member

Trott commented May 30, 2019

@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
Copy link
Contributor

@Trott I see! Sorry for misunderstanding.

@Trott
Copy link
Member

Trott commented May 30, 2019

@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
Copy link
Contributor

@Trott I could make a PR 🤷‍♂

ryzokuken added a commit to ryzokuken/node that referenced this pull request May 30, 2019
@ryzokuken
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 ryzokuken mentioned this pull request May 30, 2019
Closed
4 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

Trott pushed a commit to Trott/io.js that referenced this pull request May 30, 2019
@mihan007 @Trott
test: add util inspect null getter test
1dbf276 
PR-URL: nodejs#27884
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@Trott
Copy link
Member

Trott commented May 30, 2019

Landed in 1dbf276

@Trott Trott closed this May 30, 2019
@Trott
Copy link
Member

Trott commented May 30, 2019

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

targos pushed a commit that referenced this pull request May 31, 2019
@mihan007 @targos
test: add util inspect null getter test
38e3827 
PR-URL: #27884
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
ryzokuken added a commit that referenced this pull request Jun 1, 2019
@ryzokuken
test: rename test-performance to test-perf-hooks
389804e 
Rename test-performance to test-perf-hooks to better match its function

Refs: #27884 (comment)

PR-URL: #27969
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jun 1, 2019
@ryzokuken @targos
test: rename test-performance to test-perf-hooks
10bdd13 
Rename test-performance to test-perf-hooks to better match its function

Refs: #27884 (comment)

PR-URL: #27969
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@targos targos mentioned this pull request Jun 3, 2019
Merged
@kissofire kissofire mentioned this pull request Jul 22, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Jul 22, 2022
Open
@ron-a20 ron-a20 mentioned this pull request Jul 22, 2022
Open
@Dmarch28 Dmarch28 mentioned this pull request Jul 22, 2022
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Jul 22, 2022
Open
@137717unity 137717unity mentioned this pull request Jul 22, 2022
Open
@baby636 baby636 mentioned this pull request Jul 22, 2022
Open
@molllyn1 molllyn1 mentioned this pull request Jul 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChALkeR ChALkeR ChALkeR approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@antsmartian antsmartian antsmartian approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mihan007 @addaleax @nodejs-github-bot @ryzokuken @Trott @ChALkeR @antsmartian @cjihrig @BridgeAR