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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

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

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

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

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

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

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 <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>
@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 <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
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 <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
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 <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>
@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

@gibfahn gibfahn gibfahn left review comments

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@watilde watilde watilde approved these changes

@hiroppy hiroppy hiroppy approved these changes

@aqrln aqrln aqrln approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

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