Skip to content

test: replace [].join() with ''.repeat() - #12305

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:repeat
Closed

test: replace [].join() with ''.repeat()#12305
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:repeat

Conversation

@JacksonTian

@JacksonTianJacksonTian commented Apr 10, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Like PR #12170 .

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Apr 10, 2017
@hiroppy

hiroppy commented Apr 10, 2017

Copy link
Copy Markdown
Member

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

@vsemozhetbyt

vsemozhetbyt commented Apr 10, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

@abouthiroppy It seems these two cases in test-stringbytes-external.js also can be addressed:

node/test/parallel/test-stringbytes-external.js

Lines 48 to 49 in 98e54b0

write_str=Array(size).join(write_str);
ucs2_control=Array(size).join(ucs2_control);

@hiroppy

hiroppy commented Apr 11, 2017

Copy link
Copy Markdown
Member

@vsemozhetbyt Good catch!
@JacksonTian Could you modify these two cases?

@JacksonTian

JacksonTian commented Apr 11, 2017

Copy link
Copy Markdown
ContributorAuthor

Thanks @vsemozhetbyt@abouthiroppy , done.

@hiroppy

hiroppy commented Apr 11, 2017

Copy link
Copy Markdown
Member

CI2: https://ci.nodejs.org/job/node-test-pull-request/7323/

aqrln
aqrln reviewed Apr 15, 2017
View reviewed changes
Comment threadtest/parallel/test-stringbytes-external.js
const size = 1 + (1 << 20);
write_str = Array(size).join(write_str);
ucs2_control = Array(size).join(ucs2_control);
const size = 1 << 20;

@aqrlnaqrlnApr 15, 2017

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.

Is it necessary to change size in this PR?

@gibfahngibfahnApr 15, 2017

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.

When you do the Array.join() it ends up as one less than size, so when you move to repeat you have to decrease size by one to get the same string length.

>Array(3).join('a')'aa'>'a'.repeat(3)'aaa'

@aqrlnaqrlnApr 15, 2017

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.

Ah, I see. Thanks.

@benjamingr

benjamingr commented Apr 16, 2017

Copy link
Copy Markdown
Member

CI failure unrelated.

Figured we can land this. Landed in 4a782e1

benjamingr pushed a commit that referenced this pull request Apr 16, 2017
@JacksonTianBenjamin Gruenbaum
test: replace [].join() with ''.repeat()
4a782e1
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@JacksonTian
JacksonTian deleted the repeat branch April 19, 2017 02:31
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@JacksonTian@evanlucas
test: replace [].join() with ''.repeat()
5b9bf19
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@evanlucasevanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@JacksonTian@evanlucas
test: replace [].join() with ''.repeat()
0532361
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@JacksonTian@evanlucas
test: replace [].join() with ''.repeat()
4a19062
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@gibfahn

gibfahn commented May 15, 2017

Copy link
Copy Markdown
Member

Should this be backported to v6.x?

@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@JacksonTian@gibfahn
test: replace [].join() with ''.repeat()
cff0043
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@gibfahn

gibfahn commented Jun 18, 2017

Copy link
Copy Markdown
Member

Should this be backported to v6.x?

This only affects test/, so let's say yes

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@JacksonTian@gibfahn
test: replace [].join() with ''.repeat()
6df6af1
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@JacksonTian@MylesBorins
test: replace [].join() with ''.repeat()
a23aca4
Replace [].join calls with the more modern .repeat
PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@watildewatildewatilde approved these changes
+4 more reviewers
@gibfahngibfahngibfahn left review comments
@hiroppyhiroppyhiroppy approved these changes
@aqrlnaqrlnaqrln approved these changes
@vsemozhetbytvsemozhetbytvsemozhetbyt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@JacksonTian@hiroppy@vsemozhetbyt@benjamingr@gibfahn@jasnell@lpinca@watilde@aqrln@nodejs-github-bot