test: refactor test-httpparser.response.js

[erdun]

Replace string concatenation in test/async-hooks/test-httpparser.response.js with template literals

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

[Trott]

LGTM if Ci is green

[benjamingr]

Thanks for your contribution and welcome to the project :)

[tniessen]

IMO this does not represent the HTTP header structure as well as the old code did, where one code line was equivalent to one line of the request.

[benjamingr]

I tend to agree - I'd rather see the CRLF on a new line.

[Trott]

I think the thing to do is to go back to the old code layout, but replace lines 22, 23, and 24 with template literals:

const request = Buffer.from(
  `HTTP/1.1 200 OK${CRLF}` +
  `Content-types: text/plain${CRLF}` +
  `Content-Length: 4${CRLF}` +
  CRLF +
  'pong'
);

@erdun Does that seem good to you? Can you make that change?

[Trott]

Another possibility might be to use Array.prototype.join() but I'm not sure it's more readable:

const request = Buffer.from(
  ['HTTP 1.1 200 OK',
   'Content-types: text/plain',
   'Content-Length: 4',
   '',
   'pong'
  ].join(CRLF)
)

[Trott]

Yet another possibility for readable-and-an-improvement-over-what's-there-now is to leave it as strings but remove the inline concatenation by removing the CRLF variable:

  'HTTP/1.1 200 OK\r\n' +
  'Content-types: text/plain\r\n' +
  'Content-Length: 4\r\n' +
  '\r\n' +
  'pong'

Aside: Is Content-types is a typographical error and it is supposed to be Content-type? If so, maybe that should be fixed too.

[tniessen]

Oh okay, I assumed the capitalization was as much by design as the typo in the header name.

cc @thlorenz @nodejs/async_hooks

[gibfahn]

I thought the capitalization was by design since this is a test file.

IDK, looks like the test just checks that you get a response. You think it's checking that a different capitalisation is still valid?

[benjamingr]

@gibfahn I thought it was checking the misnamed header (It's Content-types here and not Content-Type) I assumed it was checking compatibility with nonconforming clients.

[AndreasMadsen]

I don't see any reason why Content-types vs Content-type would affect the test. I think it is just a meaningless error. The test just checks that creating an HTTPParser object correctly invokes async_hooks.

[refack]

BTW That header is not mandatory. IMHO it should be removed.
Actually the only header that "MUST" be included is the Status-Line and two CRLFs
Ref: https://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html#sec6

[vsemozhetbyt]

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

[tniessen]

1. The code is less readable than before. (Why do we even use CRLF instead of \r\n?)
2. The HTTP protocol is strictly line-oriented and the old code depicted that nicely by using one line of code per HTTP request line. These changes shadow the structure of the request (see test: refactor test-httpparser.response.js #14290 (comment)).

[jasnell]

Perhaps just replace the ${CRLF} with a literal \r\n ?

[tniessen]

+1 (see #14290 (review))

[refack]

Message should be minimized (optional headers removed)

[trevnorris]

git commit title is too long. please limit to 50 characters.

[Trott]

Fixed up commit message and nits. PTAL @trevnorris @refack @tniessen

[Trott]

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

Thanks for fixing the git commit title

[Trott]

Landed in 672e020.

Thanks for the contribution! 🎉