test: introduce tag functions to handle long strings in code by vsemozhetbyt · Pull Request #13016 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

Multiline template literals in ES6 make long strings handling easier in code. However, there are many annoying nits that prevent this option from using in many cases: code block indentation adds unwanted spaces to these strings, leading and trailing backticks break alignment or add unwanted leading and trailing line breaks.

This PR proposes 4 helper functions to mitigate these difficulties:

common.tagGlue() — joins a multiline template literal seamlessly, i.e. removes all literal line breaks and leading spaces (handy to code big text blobs not intended for human reading).
common.tagUnwrap() — joins a multiline template literal with a space, i.e. replaces all literal line breaks and leading spaces with a space, then trims one leading and one trailing space (handy to code long readable text lines).
common.tagLFy() — preserves line breaks as \n, but removes two framing line breaks and only auxiliary indents (handy to code multiline readable text, JavaScript metacode, HTML markup etc).
common.tagCRLFy() — same as tagLFy(), but replaces all literal \n by \r\n (handy to code raw HTTP requests/responses).

You can see examples in the proposed doc fragments.

This PR also demonstrates the applying these functions to many test fragments.

All these functions leave interpolated variables untouched, only literal parts are processed. In most cases, these functions get multiline strings with leading and trailing line breaks: this allows to divide code noise (backticks etc) and pure data. These leading and trailing line breaks are stripped by the tag functions. This should be taken into account: if the result needs a line break in the very end, an additional line break should be added (beware, for example, HTTP raw code — see samples in the second commit).

I understand that this is a big PR and mostly based on personal taste. So see it as a strawman and feel free to reject without thorough explanations.

Though the diff seems huge, the changes are mostly trivial, concern multiline blocks formatting and are easy to skim.

CI: https://ci.nodejs.org/job/node-test-pull-request/8061/
Another CI: https://ci.nodejs.org/job/node-test-pull-request/8069/

Rubber stamp LGTM. This definitely makes texts like HTTP messages easier to read..

.trim()?

On this stage, the replacing concerns the whole string, including interpolated variables, so I try to be extra careful here. i.e. to remove only two auxiliary spaces (which were framing line breaks before the reducing).

If I am reading correctly strings[0].match(/\n+( +)/) should work as well?

In case there are no indents — yes if I get it right, but maybe with * this returns earlier, without using the fallbacks || [] and = ''

@joyeecheung would fail for (badly formated) literals like:

const tagged = tagLF`
<!doctype html>
<html>
  <head>
    <meta charset="UTF-8">
    <title></title>
  </head>
  <body>
    ${process.versions}
  </body>
</html>
`;

would give firstIndent === undefined

Great idea 👍
soft suggestion I tagged with [suggestion]
And I'm only rubber stamping test/parallel/test-promises-unhandled-rejections.js

IMHO looks better 👍

Can you revert this file? it'll conflict with #13003

OK.

I'm rubber stamping this file...
IMHO you're mixing adding the new tags with refactoring.
(Could move to another PR or a different commit)

I have only reformatted the first string arguments in each function, nothing more. GitHub diff just goes astray here, sorry.

I'll take a look locally.

why no indent?

I will fix, thank you.

I have recalled why: see the long line without a break in the previous variant, this is the only way to preserve it.

[suggestion] I know you don't like to change too much, but I would go

const keepLF = '\n';
const fileName = path.join(common.tmpDir, '.node_repl_history');
const sameHistoryFilePaths = wrap`
  ${keepLF}
  The old repl history file has the same name and location as the new one i.e.,
  ${fileName} and is empty.${keepLF}
  Using it as is.${keepLF}
  ${keepLF}
`;

maybe even add export.keepLF = '\n' to common

I am afraid this would be complicated enough to scare out all other reviewers :)

Next PR suggestion: A tag with JSON.stringify for j{} variables?

Or with util.inspect() :)

[suggestion] maybe even rename tagHTTP

There were other cases in the tests. And potentially can be others (Windows stuff etc).

[suggestion] for better readability:

        console.error(exports.tagUnwrap`
          Looks like we're in a FreeBSD Jail.
          Please provide your default interface address as LOCALHOST 
          or expect some tests to fail.
        `);

move all consts out of the functions. no need to redefine on each invocation.
could be:

const TAG_CONSTANTS = {
  tagGlueBreaks: /\n */g,
  tagUnwrapBreaks: /(\n *)+/g,
  tagUnwarpTrimmer: /^ | $/g,
...

I would prefer to keep the functions self-sufficient for better modularity while they are not used in libs critical for performance.

I won't block on this, but you might find out you can reuse some, or other coders might reuse, also gives them better names.

I also remember that common module is considered too bloated, so I am afraid to pollute it with more global variables.

@joyeecheung would fail for (badly formated) literals like:

const tagged = tagLF`
<!doctype html>
<html>
  <head>
    <meta charset="UTF-8">
    <title></title>
  </head>
  <body>
    ${process.versions}
  </body>
</html>
`;

would give firstIndent === undefined

[Suggestion]
IMHO go stricter, i.e. new RegExp(\n {${firstIndent.length}}, 'g');
example:

const literlaHTML = tagLF`
  <!doctype html>
  <html>
    <head>
      <meta charset="UTF-8">
      <script>
var x = [1,2,3].map(functor, seed)
  .filter(predicate)
  .reduce(functor, '');
      <script>
      <title></title>
    </head>
    <body>
      ${process.versions}
    </body>
  </html>
`;

I would prefer to ignore such misindentation cases in favor of these cases:

{
  const tagged = tagLF`
    <!doctype html>
    <html>
      ...
    </html>
  `;
}

NB: indentation before the last backtick is less than the first indentation. With your variant, it will be preserved with the trailing line break.

BTW, these misindented fragments can be wrapped in ${''} and they will be untouched.

Read your comments 👍 on all but #13016 (comment), think again, but it's your call.

I've added a bit more explanations in the doc (including intro note) and addressed some requests.

CI: https://ci.nodejs.org/job/node-test-pull-request/8070/
OSX seems to be flaky these days.
Another CI: https://ci.nodejs.org/job/node-test-pull-request/8071/
And again: https://ci.nodejs.org/job/node-test-pull-request/8072/
One Windows failure seems unrelated.

Start to alleviate multiline template literals manipulations.

I've replaced tagLF and tagCRLF by the tagLFy and tagCRLFy to make them more verbal.

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

@refack I mean https://en.oxforddictionaries.com/definition/-fy Does it make sense?

@refack I mean https://en.oxforddictionaries.com/definition/-fy Does it make sense?

I'm just making faces
If the new names make you more comfortable, I approve.

cc @nodejs/collaborators : this PR extends test common library and adds 4 new functions to many places of many tests. Does anybody object to landing?

I am 👍 with the change, but 👎 on the amount of breakage that might introduce. I already had to backport a PR from master to v7 and soon v6, because of changes in the test harness.
I think we should have a solid plan to backport this to v6 before we land this in master.

We keep adding more and more stuff to common.js and I honestly don't like this very much. It increases the cognitive load required to write tests. Tests should ideally only use an assertion library imho.

That said, I have no objections to these changes.

@mcollina It is a pity how the backporting burden prevents us from lightening other burdens, but it seems we should respect this burden as we respect gravitation.

@lpinca I understand and I recall the recent attempts to slim down the common. But the help from these functions seems to surpass its cognitive load for me. However, this is too personal (I have been using some of them for a while), so I do not insist.

It seems we have sound (yet soft) objections against the both commits: the fist makes common more bloated, the second brings too much churn. So if there is no strong "pro" from many collaborators I shall abstain to land.

I'm in favor of this change, but only if it is backported asap to v6. Otherwise it creates too much churn.

@mcollina I can do a backport PR for v6 after landing if this is not backported cleanly. How much asap this should be made?

cc @nodejs/lts for that answer.

It increases the cognitive load required to write tests.

@lpinca I agree that it is very important to lower the barrier (cognitive or other) for writing tests! But IMHO in this specific case the trade-off of cognitive load while writing code is cognitive load while reading/fixing code. There are good examples that definatly make the code much more readable, like here, and here

@vsemozhetbyt will these tags benefit /benchmarks/?

@refack

will these tags benefit /benchmarks/?

I have not checked yet, I just want to see if this can be accepted for tests. But I planned to try to apply for benchmarks as well.

As for tradeoffs... For writing, the load is eased by optionality of using these functions. For reading, the load is increased by the "what the ... this tag*`` means" surprise effects :)

@refack I'm a bit unconvinced by your second example.

{
  data: tagCRLFy`
          POST / HTTP/1.0
          Connection: keep-alive


  `
}

is actually harder for me to grok than

{
  data: 'POST / HTTP/1.0\r\n' +
        'Connection: keep-alive\r\n' +
        '\r\n'
}

Anyway, to reiterate, I have nothing against these changes. I'm fine if this gets merged.

@lpinca @refack Yes, the examples with some trailing line breaks look a bit awkward with this approach, but this is the price of the function universality. I can revert these changes if there is strong -1 here.

@refack I'm a bit unconvinced by your second example.

fair enough...

the examples with some trailing line breaks look a bit awkward with this approach, but this is the price of the function universality

To follow up on #13016 (comment) IMHO since HTTP will probably be the main use case for this one, we could add (in following PR) a #{HTTP_SECTION_END} symbol.

I think I'm -1 on these changes for two reasons.

It has become increasingly more difficult to cherry pick changes into the staging branches and I think this will continue to exacerbate the problem
I find using a tag for this more difficult to immediately understand than using simple string concatenation. I feel like this adds more indirection.

@evanlucas I hear you.

@vsemozhetbyt :] I definitely appreciate the effort put in

A couple thoughts:

I think the verbose cut-n-paste in the tests decreases their readability and maintainability (like cut-n-paste in any other code base), and am generally in favour of test utility functions.
These specific changes appear to make the code longer/more verbose, and less clear in its intent, so I'm not particularly convinced they are an improvement.
For backporting, @nodejs/lts attempts to make sure that changes like this are backported so changes after it will cherry-pick clean. I can't speak for the WG, but I'm pretty sure we'd land this on 6.x if it lands on 7.x.

Well, it seems this is a too controversial proposition. So I better close for now to not distract more collaborators anymore. Thank you for all the feedback and time. I hope this was not completely in vain)

IMHO the most controversial was tagGlue, but tagCRLFy (that should be named tagHTTPfy) and tagLFy have some very good uses. So maybe don't try to cover everything and focus on the really good ones:

@refack I shall ponder on this.

vsemozhetbyt added dont-land-on-v4.x test Issues and PRs related to the tests. labels May 13, 2017

nodejs-github-bot added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels May 13, 2017

vsemozhetbyt removed the tools Issues and PRs related to the tools directory. label May 13, 2017

joyeecheung approved these changes May 14, 2017

View reviewed changes

refack suggested changes May 14, 2017

View reviewed changes

refack approved these changes May 14, 2017

View reviewed changes

vsemozhetbyt added 2 commits May 15, 2017 22:54

test: add some base tag functions to common

243dfb9

Start to alleviate multiline template literals manipulations.

test: apply tag functions

306f228

evanlucas suggested changes May 16, 2017

View reviewed changes

sam-github added the lts-watch-v6.x label May 16, 2017

vsemozhetbyt closed this May 16, 2017

vsemozhetbyt deleted the test-common-tags branch May 16, 2017 18:11

test: introduce tag functions to handle long strings in code by vsemozhetbyt · Pull Request #13016 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: introduce tag functions to handle long strings in code - #13016

test: introduce tag functions to handle long strings in code#13016vsemozhetbyt wants to merge 2 commits intonodejs:masternodejs/node:masterfrom vsemozhetbyt:test-common-tagsCopy head branch name to clipboard

Conversation

vsemozhetbyt commented May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

test: introduce tag functions to handle long strings in code#13016
vsemozhetbyt wants to merge 2 commits into
nodejs:masterfrom
vsemozhetbyt:test-common-tags

vsemozhetbyt commented May 13, 2017 •
edited
Loading

vsemozhetbyt commented May 13, 2017 •
edited
Loading

vsemozhetbyt May 14, 2017 •
edited
Loading

vsemozhetbyt May 14, 2017 •
edited
Loading

refack May 14, 2017 •
edited
Loading

vsemozhetbyt May 14, 2017 •
edited
Loading

refack May 14, 2017 •
edited
Loading

vsemozhetbyt May 14, 2017 •
edited
Loading

refack May 14, 2017 •
edited
Loading

refack May 14, 2017 •
edited
Loading

vsemozhetbyt commented May 14, 2017 •
edited
Loading

vsemozhetbyt commented May 16, 2017 •
edited
Loading

lpinca commented May 16, 2017 •
edited
Loading

vsemozhetbyt commented May 16, 2017 •
edited
Loading

refack commented May 16, 2017 •
edited
Loading

vsemozhetbyt commented May 16, 2017 •
edited
Loading

vsemozhetbyt commented May 16, 2017 •
edited
Loading