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.

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

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

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 18, 2016

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 Trott added test Issues and PRs related to the tests. lts-watch-v4.x labels Feb 18, 2016
@@ -19,7 +19,7 @@ var ucs2_control = 'a\u0000';

// grow the strings to proper length
while (ucs2_control.length <= EXTERN_APEX) {
ucs2_control += ucs2_control;
ucs2_control = ucs2_control.repeat(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about skipping the while loop and doing

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@thefourtheye
Copy link
Contributor

LGTM.

@targos
Copy link
Member

targos commented Feb 19, 2016

LGTM

host += host;
host += host;
host += host;
var host = '********'.repeat(32);
Copy link
Member

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

LGTM with a nit

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

Trott commented Feb 19, 2016

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

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

@jasnell
Copy link
Member

jasnell commented Feb 20, 2016

LGTM!

Trott added a commit to Trott/io.js that referenced this pull request Feb 21, 2016
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Trott
Copy link
Member Author

Trott commented Feb 21, 2016

Landed in 97f76f3

@Trott Trott closed this Feb 21, 2016
rvagg pushed a commit that referenced this pull request Feb 21, 2016
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Trott Trott deleted the moar-recommended branch January 9, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants