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

test: use String.prototype.repeat() for clarity by Trott · Pull Request #5311 · nodejs/node · GitHub

/ node Public

test: use String.prototype.repeat() for clarity - #5311

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:moar-recommended
ConversationCommits (1)ChecksFiles changed
Closed

test: use String.prototype.repeat() for clarity#5311
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:moar-recommended

Conversation

Trott commented Feb 18, 2016

Copy link
Copy Markdown
Member

There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using .repeat() makes the
code clearer.

For example, before:

for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

lots_of_headers = lots_of_headers.repeat(256);

Using .repeat() makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

Trott added test Issues and PRs related to the tests. lts-watch-v4.x labels Feb 18, 2016
thefourtheye reviewed Feb 19, 2016
View reviewed changes
Comment thread test/parallel/test-crypto-from-binary.js Outdated

thefourtheye Feb 19, 2016

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

How about skipping the while loop and doing

ucs2_control = ucs2_control.repeat(EXTERN_APEX / ucs2_control.length);

Trott Feb 19, 2016

Copy link
Copy Markdown
Member Author

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

That will loop until the string is just under length EXTERN_APEX but I think what we want is for it to loop until it exceeds EXTERN_APEX.

To fix that, you could simply add 1 to the number that results from the division. But by looping, I preserve the power of two length as well. That may not be significant, but I don't like to mess with crypto, so I did it this way so it was exactly the same.

thefourtheye Feb 19, 2016

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

Fair enough.

thefourtheye commented Feb 19, 2016

Copy link
Copy Markdown
Contributor

LGTM.

targos commented Feb 19, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell reviewed Feb 19, 2016
View reviewed changes
Comment thread test/parallel/test-http-dns-error.js Outdated

jasnell Feb 19, 2016

Copy link
Copy Markdown
Member

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

var host = '*'.repeat(256); ?

jasnell commented Feb 19, 2016

Copy link
Copy Markdown
Member

LGTM with a nit

test: use String.prototype.repeat() for clarity
95cc99e 
There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using `.repeat()` makes the
code clearer.

For example, before:

    for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

    lots_of_headers = lots_of_headers.repeat(256);

Using `.repeat()` makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

Trott commented Feb 19, 2016

Copy link
Copy Markdown
Member Author

Applied nit from @jasnell, rebased against master, force pushed.

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

jasnell commented Feb 20, 2016

Copy link
Copy Markdown
Member

LGTM!

Trott added a commit to Trott/io.js that referenced this pull request Feb 21, 2016
test: use String.prototype.repeat() for clarity
97f76f3 
There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using `.repeat()` makes the
code clearer.

For example, before:

    for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

    lots_of_headers = lots_of_headers.repeat(256);

Using `.repeat()` makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

PR-URL: nodejs#5311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott commented Feb 21, 2016

Copy link
Copy Markdown
Member Author

Landed in 97f76f3

Trott closed this Feb 21, 2016
rvagg pushed a commit that referenced this pull request Feb 21, 2016
test: use String.prototype.repeat() for clarity
0e85530 
There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using `.repeat()` makes the
code clearer.

For example, before:

    for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

    lots_of_headers = lots_of_headers.repeat(256);

Using `.repeat()` makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

PR-URL: #5311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
JacksonTian mentioned this pull request Feb 22, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
test: use String.prototype.repeat() for clarity
2081ab7 
There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using `.repeat()` makes the
code clearer.

For example, before:

    for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

    lots_of_headers = lots_of_headers.repeat(256);

Using `.repeat()` makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

PR-URL: #5311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
test: use String.prototype.repeat() for clarity
9d6623e 
There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using `.repeat()` makes the
code clearer.

For example, before:

    for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

    lots_of_headers = lots_of_headers.repeat(256);

Using `.repeat()` makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

PR-URL: #5311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
test: use String.prototype.repeat() for clarity
6d5f4c4 
There are a few places where tests repeatedly concatenate strings to
themselves in order to make them very long. Using `.repeat()` makes the
code clearer.

For example, before:

    for (var i = 0; i < 8; ++i) lots_of_headers += lots_of_headers;

After:

    lots_of_headers = lots_of_headers.repeat(256);

Using `.repeat()` makes it clear that the string will be repeated 256
times rather than 8 times. ("What?! That first one doesn't repeat 256
times! It only repeats 8... Oh, wait. Yes, I see your point now.")

PR-URL: #5311
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Trott deleted the moar-recommended branch January 9, 2022 22:01
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

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL