Skip to content

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

Closed
JacksonTian wants to merge 1 commit intonodejs:masterfrom
JacksonTian:repeat
Closed

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

Conversation

@JacksonTian
Copy link
Contributor

@JacksonTian JacksonTian commented Apr 10, 2017 •
edited
Loading

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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 10, 2017
@hiroppy
Copy link
Member

hiroppy commented Apr 10, 2017

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 10, 2017 •
edited
Loading

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

hiroppy commented Apr 11, 2017

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

@JacksonTian
Copy link
Contributor Author

JacksonTian commented Apr 11, 2017

Thanks @vsemozhetbyt @abouthiroppy , done.

@hiroppy
Copy link
Member

hiroppy commented Apr 11, 2017

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

aqrln
aqrln reviewed Apr 15, 2017
View reviewed changes
test/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;
Copy link
Contributor

@aqrln aqrln Apr 15, 2017

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?

Copy link
Member

@gibfahn gibfahn Apr 15, 2017

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'

Copy link
Contributor

@aqrln aqrln Apr 15, 2017

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

benjamingr commented Apr 16, 2017

CI failure unrelated.

Figured we can land this. Landed in 4a782e1

@benjamingr benjamingr closed this Apr 16, 2017
benjamingr pushed a commit that referenced this pull request Apr 16, 2017
@JacksonTian
test: replace [].join() with ''.repeat()
4a782e1 
Replace [].join calls with the more modern .repeat

PR-URL: #12305
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@evanlucas evanlucas 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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented May 15, 2017

Should this be backported to v6.x?

@gibfahn gibfahn 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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@watilde watilde watilde approved these changes

+4 more reviewers

@gibfahn gibfahn gibfahn left review comments

@hiroppy hiroppy hiroppy approved these changes

@aqrln aqrln aqrln approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

Reviewers whose approvals may not affect merge requirements

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.

10 participants

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