test: add coverage for sparse array maxArrayLength by wentout · Pull Request #27901 · nodejs/node

wentout · 2019-05-26T14:04:23Z

code and learn task for additional coverage for situation when
maxArrayLength option is passed to util.inspect for sparsed array
and is set to number lower than actual number of entries

Checklist

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

ryzokuken

LGTM, thanks!

mhdawson

LGTM

BridgeAR

Does this really cover the correct part? It seems to be handled in the fast path (formatArray) since it did not yet detect that it's actually a sparse array.

Trott · 2019-05-29T13:11:51Z

(Typo in commit message. Can be fixed when landing, though. s/sparsed/sparse/)

wentout · 2019-05-29T23:20:56Z

Does this really cover the correct part? It seems to be handled in the fast path (formatArray) since it did not yet detect that it's actually a sparse array.

The task was to try to make it for .formatSpecialArray method of lib/internal/util/inspect.js L1183
https://coverage.nodejs.org/coverage-99268b1e996d13a0/lib/internal/util/inspect.js.html#L1183.

The test is for this line only. The array is already made sparsed on previous test lines. So the detection is on the previous one-liner test there is <1 empty item> string check.

As for failed build checks, I don't understand how it was able to touch the build itself via test file. Please navigate me to understanding.

wentout · 2019-05-30T07:05:19Z

I think after changes coverage is correct.

nodejs-github-bot · 2019-05-30T07:23:31Z

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

Trott · 2019-05-30T13:41:57Z

/ping @BridgeAR Is this good to land? Or are you still concerned that it doesn't cover the code path intended?

BridgeAR

LGTM. This will now cover the correct part. It would still be nice though to address the comment to simplify the test case.

BridgeAR · 2019-05-30T13:53:05Z

Ideally this could be moved down to be the very last test case. That way there are already enough entries in the array and it would work without pushing / splicing / deleting entries. Calling splice should definitely not be required.
(So below here https://github.com/nodejs/node/blob/5ce3d10434e0d81934f7013a2a13d1502be6aca6/test/parallel/test-util-inspect.js#L520)

Ah, I understand.
I will redo.
.splice is heavy :)

Moved too the end of sparse array tests.
Made coverage check for line 1183 again, it is there/

code and learn task for additional coverage for situation when maxArrayLength option is passed to util.inspect for sparsed array and is set to number lower than actual number of entries

BridgeAR

Thanks a lot for following up on it!

wentout · 2019-05-30T17:19:19Z

Thanks a lot for following up on it!

Nothing for!
And vice a versa for you from all this explanations. Much appreciate! Wish I'd be as good mentor.

nodejs-github-bot · 2019-06-01T08:15:51Z

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

ryzokuken · 2019-06-01T09:01:27Z

Passed! Landing this.

code and learn task for additional coverage for situation when maxArrayLength option is passed to util.inspect for sparse array and is set to number lower than actual number of entries PR-URL: nodejs#27901 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

ryzokuken · 2019-06-01T09:07:34Z

Landed in dcd0ba1 w/ @Trott's comments. 🎉

code and learn task for additional coverage for situation when maxArrayLength option is passed to util.inspect for sparse array and is set to number lower than actual number of entries PR-URL: #27901 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

addaleax approved these changes May 26, 2019
View reviewed changes

cjihrig approved these changes May 26, 2019
View reviewed changes

ryzokuken approved these changes May 26, 2019
View reviewed changes

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

Trott approved these changes May 27, 2019
View reviewed changes

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

This comment has been minimized.
Sign in to view

gireeshpunathil approved these changes May 27, 2019
View reviewed changes

This comment has been minimized.
Sign in to view

mhdawson approved these changes May 27, 2019
View reviewed changes

BridgeAR reviewed May 29, 2019
View reviewed changes

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

wentout changed the title ~~test: add coverage for sparsed array maxArrayLength~~test: add coverage for sparse array maxArrayLengthMay 29, 2019

wentout force-pushed the cl_test_4_sparsed_array branch 2 times, most recently from 4fec302 to 5ce3d10Compare May 30, 2019 00:32

BridgeAR approved these changes May 30, 2019
View reviewed changes

wentout added 2 commits May 30, 2019 19:53

test: add coverage for sparse array maxArrayLength
5e41d93
code and learn task for additional coverage for situation when maxArrayLength option is passed to util.inspect for sparsed array and is set to number lower than actual number of entries

code review fixes
3eca651

wentout force-pushed the cl_test_4_sparsed_array branch from 5ce3d10 to 3eca651Compare May 30, 2019 16:53

BridgeAR approved these changes May 30, 2019
View reviewed changes

ryzokuken closed this Jun 1, 2019

targos mentioned this pull request Jun 3, 2019
v12.4.0 release proposal #28040
Merged

nodejs deleted a comment from tacoto12 Apr 8, 2020

Uh oh!

Uh oh!

Conversation

wentout commented May 26, 2019

Checklist

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented May 29, 2019

Uh oh!

wentout commented May 29, 2019

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented May 30, 2019

Uh oh!

Trott commented May 30, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeARMay 30, 2019

Choose a reason for hiding this comment

Uh oh!

wentoutMay 30, 2019

Choose a reason for hiding this comment

Uh oh!

wentoutMay 30, 2019

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

wentout commented May 30, 2019

Uh oh!

nodejs-github-bot commented Jun 1, 2019

Uh oh!

ryzokuken commented Jun 1, 2019

Uh oh!

ryzokuken commented Jun 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

wentout commented May 30, 2019•
edited
Loading