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: reduce string concatenations #12735

Closed
wants to merge 21 commits into from
Closed

test: reduce string concatenations #12735

wants to merge 21 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 28, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Refs: #12455

This is not an exhaustive deliverance from all string concatenations: if a result looked too ugly and messy, I've abstained to change, so some concatenations still remain. I am not sure if we should to add prefer-template: error to test .eslintrc.yaml, so I've also abstained from proposing, but let me know if this is discussable.

Concerning reviews: #12735 (comment)

  • 01. test: reduce string concatenations (the base commit, with test/common.js only)
  • 02. fixup: addons – known_issues, pseudo-tty
  • 03. fixup: pummel
  • 04. fixup: sequential
  • 05. fixup: parallel/assert* – child-process*
  • 06. fixup: parallel/cli* – cwd*
  • 07. fixup: parallel/dgram* – file*
  • 08. fixup: parallel/fs*
  • 09. fixup: parallel/http* – http-full-response
  • 10. fixup: parallel/http-get-pipeline-problem – http-write-empty-string!
  • 11. fixup: parallel/https* – https-drain
  • 12. fixup: parallel/https-eof-for-eom – https-truncate
  • 13. fixup: parallel/intl* – net*
  • 14. fixup: parallel/next-tick* – regress*
  • 15. fixup: parallel/repl* – spawn*
  • 16. fixup: parallel/stdin* – timers*
  • 17. fixup: parallel/tls* – tls-connect*
  • 18. fixup: parallel/tls-delayed* – tls-invoke-queued
  • 19. fixup: parallel/tls-js-stream – tls-securepair*
  • 20. fixup: parallel/tls-server* – tls-wrap-timeout
  • 21. fixup: parallel/url* – zlib*

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. debugger inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Apr 28, 2017
@vsemozhetbyt vsemozhetbyt added refactor to ES6+ and removed addons Issues and PRs related to native addons. debugger inspector Issues and PRs related to the V8 inspector protocol labels Apr 28, 2017
@vsemozhetbyt
Copy link
Contributor Author

@refack
Copy link
Contributor

refack commented Apr 28, 2017

Will you be able to activate any eslint rules after this?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 28, 2017

@refack There are still ~20 cases like these ones I've abstained to change, at least till I think up some equally readable solutions. But these ones can be addressed with ESLint disabling comments. So prefer-template: error can be added to the test .eslintrc.yaml with a very little churn after this PR.

@refack
Copy link
Contributor

refack commented Apr 28, 2017

You should add those (rule and comment exceptions) as it would provide extra motivation for this PR.
I'm a big believer better code, but there is code-churn and git history to consider. So IMHO adding an eslint rule will tip the scales in favor of this PR.

@vsemozhetbyt
Copy link
Contributor Author

I could do this if there is a minimal consensus if we should restrict this aspect from now on. Maybe some liberality is preferred here by the other collaborators)

@refack
Copy link
Contributor

refack commented Apr 28, 2017

P.S. for things like these ones, I would do a multiline `` with replace

`PUT /this HTTP/1.1
Content-Type: text/plain
Transfer-Encoding: chunked

4
ping
0
`.replace(/\r|\n|\r\n/g, '\r\n')

@refack
Copy link
Contributor

refack commented Apr 28, 2017

I could do this if there is a minimal consensus if we should restrict this aspect from now on. Maybe some liberality is preferred here by the other collaborators)

Yes, this is only my opinion and I actually want to +1

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 28, 2017

@refack I've considered this way, but there are nits here concerning indentation, leading/trailing returns and aligning. There are solutions, and maybe some util formatting function can be added to common.js lib, but this also may need to be discussed.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 28, 2017

@refack By the way, .replace(/\r|\n|\r\n/, '\r\n') may safely be just .replace(/\n/g, '\r\n'), as per spec in multiline template literals can only be \n regardless of OS, IIRC.

@refack
Copy link
Contributor

refack commented Apr 29, 2017

@vsemozhetbyt I love 2ality 🥇
As for "weird" situations, just exclude them with comments...

@benjamingr
Copy link
Member

I like the change but it's huge which makes it impractical to actually review. I went through the first 20-30 and they look good - but I don't feel comfortable with landing this unless someone goes through all changed content.

@vsemozhetbyt
Copy link
Contributor Author

Maybe we can divide: a reviewer can write down the last file reviewed, next reviewer can start from the next file.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Almost went over all.
A few nits, some suggestions, and request to change path manipulation to path.join

@@ -34,11 +34,11 @@ let quit;

function startDebugger(scriptToDebug) {
scriptToDebug = process.env.NODE_DEBUGGER_TEST_SCRIPT ||
common.fixturesDir + '/' + scriptToDebug;
`${common.fixturesDir}/${scriptToDebug}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be path.join

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many such things in the tests. Should we address them 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.

Separate PR is likely safer.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think a path.join is important here, node modules always use / (so it's not an OS compatibility concern). The main advantage of path.join is that it abstracts joining complex paths, for concatenating two strings / should be fine.

That said, if this view is contested - we can just leave it as is in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO if we're touching this, so might as well "fix" it. There are multiple path concatenations done as strings, I feel they should all be changed, as path.join is semantically different, even if string concat works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be ~300 re-editions in the PR...

Copy link
Contributor

@refack refack Apr 29, 2017

Choose a reason for hiding this comment

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

Your call @vsemozhetbyt if you have the energy do it. I'll review it.
If you say not this PR I'll retract those comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am in doubt if there is a minimum consensus for this. If there are 2-3 +1 and no strong -1, I can try to refactor with path.join().

Copy link
Member

Choose a reason for hiding this comment

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

Like James and Benjamin, I’m fine with leaving this as-is. Leaving path.join() refactors for a later PR sounds good to me. (If there’s consensus they should happen.)

Copy link
Member

Choose a reason for hiding this comment

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

Yep, seems like a job for a separate PR.

assert(message['result'], JSON.stringify(message) + ' (response to ' +
JSON.stringify(this.messages_[id]) + ')');
assert(message['result'], `${JSON.stringify(message)} (response to ${
JSON.stringify(this.messages_[id])})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

+0
personally I'd rather you define as vars then do this trick

const msg_json = JSON.stringify(message);
const id_json =  JSON.stringify(this.messages_[id]);
assert(message['result'], `${msg_json} (response to ${id_json})`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created some new variables if this improved many places per file. But I also tried not to be too creative to not overload the diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

not a big fan of the trick... And otherwise you can't fit it in one line...

Copy link
Member

Choose a reason for hiding this comment

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

+1 to @refack's suggestion, I think it definitely improves the readability.

assert(!message['error'], JSON.stringify(message) + ' (replying to ' +
JSON.stringify(this.messages_[id]) + ')');
assert(!message['error'], `${JSON.stringify(message)} (replying to ${
JSON.stringify(this.messages_[id])})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -215,10 +215,10 @@ TestSession.prototype.sendInspectorCommands = function(commands) {
timeoutId = setTimeout(() => {
let s = '';
for (const id in this.messages_) {
s += id + ', ';
s += `${id}, `;
Copy link
Contributor

Choose a reason for hiding this comment

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

const s = this.messages_.map(m => m.id).join(', ');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same: I would abstain from refactoring too deep: this will make reviewing and testing a lot more difficult.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I say, leave it as is, comment with TODO

Copy link
Member

Choose a reason for hiding this comment

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

+1 to @refack's suggestion again. While you're here you might as well simplify, and it's a simple atomic change (so not too painful to review).

assert.fail('Messages without response: ' +
s.substring(0, s.length - 2));
assert.fail(
`Messages without response: ${s.substring(0, s.length - 2)}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

If using map, no need for substring

expected.slice(120, 180)} \n${
expected.slice(180, 240)} \n${
expected.slice(240, 300)}\n${
expected.slice(300, 360)}\n`;
Copy link
Contributor

Choose a reason for hiding this comment

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

-1
IMHO doesn't improve readability.
Maybe:

const expectedWhite =
`${expected.slice(0, 60)}\u0020
${expected.slice(60, 120)}\u0020
${expected.slice(120, 180)}\u0020
${expected.slice(180, 240)}\u0020
${expected.slice(240, 300)}
${expected.slice(300, 360)}
`;

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 29, 2017

Choose a reason for hiding this comment

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

This may not be linted (indents).

Copy link
Member

@jasnell jasnell Apr 29, 2017

Choose a reason for hiding this comment

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

Definitely not a fan of multiline string literals. An array with a join might be an even better option.

const expectedWrite = [
  expected.slice(0, 60),
  expected.slice(60, 120),
  expected.slice(120, 180),
  expected.slice(180, 240),
  expected.slice(240, 300),
  expected.slice(300, 360)
].join(' \n') + '/n';

expected.slice(120, 180)} \x00${
expected.slice(180, 240)} \x98${
expected.slice(240, 300)}\x03${
expected.slice(300, 360)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

same

useTryCatchOpt
].join(' ');
cmdToExec += `${process.argv[0]} ${cmdLineOption ? cmdLineOption : ''} ${
process.argv[1]} child ${throwInDomainErrHandlerOpt} ${useTryCatchOpt}`;

const child = exec(cmdToExec);
Copy link
Contributor

Choose a reason for hiding this comment

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

+-0
IMHO change the whole assembly to [].push(x).join(' ')

[
[entry, '../' + common.tmpDirName + '/cycles/root.js']
[entry, `../${common.tmpDirName}/cycles/root.js`]
].forEach(function(t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

path.join

test3(fs.createWriteStream(common.tmpDir + '/dummy3'));
test1(fs.createWriteStream(`${common.tmpDir}/dummy1`));
test2(fs.createWriteStream(`${common.tmpDir}/dummy2`));
test3(fs.createWriteStream(`${common.tmpDir}/dummy3`));

Copy link
Contributor

@refack refack Apr 29, 2017

Choose a reason for hiding this comment

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

my progress marker

@refack
Copy link
Contributor

refack commented Apr 29, 2017

That's a huge PR indeed

@refack refack mentioned this pull request Apr 29, 2017
2 tasks
@vsemozhetbyt
Copy link
Contributor Author

I've reverted changes in the test/parallel/test-buffer-alloc.js and resolved conflicts.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 29, 2017

I don't know what to do. To divide this PR in small ones, I lack criterions (by files number? by tests topics? other?). And I understand that other collaborators may not have my 2 free days to review all the changes (diff is very slow in addition). Feel free to say to close, I shall not consider this as a waste of time, this was a useful experience in many respects for me)

@refack
Copy link
Contributor

refack commented Apr 29, 2017

I don't know what to do. To divide this PR in small ones, I lack criterions (by files number? by tests topics? other?). And I understand that other collaborators may not have my 2 free days to review all the changes (diff is very slow in addition). Feel free to say to close, I shall not consider this as a waste of time, this was a useful experience in many respects for me)

IMHO Keep it. It's done, it's good. I'll go over it all.

@refack
Copy link
Contributor

refack commented Apr 29, 2017

Removed requests to hoist stuff to common.js, agree that that's a different PR.

@vsemozhetbyt
Copy link
Contributor Author

O my) Conflicts resolved. New CI: https://ci.nodejs.org/job/node-test-pull-request/7758/

@vsemozhetbyt
Copy link
Contributor Author

Some new conflicts resolved. Hopefully, the last CI: https://ci.nodejs.org/job/node-test-pull-request/7891/

vsemozhetbyt added a commit that referenced this pull request May 5, 2017
PR-URL: #12735
Refs: #12455
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@vsemozhetbyt
Copy link
Contributor Author

Landed in 8b76c3e

Sorry for all the conflicts it may cause...

@vsemozhetbyt vsemozhetbyt deleted the test-templates branch May 5, 2017 14:42
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
PR-URL: nodejs#12735
Refs: nodejs#12455
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

@vsemozhetbyt do you have the patience to backport this to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@vsemozhetbyt
Copy link
Contributor Author

@gibfahn I can try, but will anybody have the time to review?

@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

@gibfahn I can try, but will anybody have the time to review?

If you raise the PR I promise to review 😁 . Feel free to assign it to me.

@vsemozhetbyt
Copy link
Contributor Author

@gibfahn Hopefully backported: #13835

MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
PR-URL: #12735
Refs: #12455
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
Backport-PR-URL: #13835
PR-URL: #12735
Refs: #12455
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Backport-PR-URL: #13835
PR-URL: #12735
Refs: #12455
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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.

8 participants