test: Add tests for add/remove header after sent. #8682

niklasi · 2016-09-21T07:14:13Z

Checklist

tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test, http

Description of change

This change adds tests to make sure an Error is thrown if a header
is added or removed after they are sent.

This change adds tests to make sure an Error is thrown if a header is added or removed after they are sent.

santigimeno · 2016-09-21T07:26:56Z

test/parallel/test-http-response-add-header-after-sent.js

+server.listen(common.PORT, () => {
+  http.get({port: server.address().port}, () => {
+    server.close()
+  });


I would listen in the 0 port so it picks a random port

Agreed. s/common.PORT/0/

Ok, I will fix that. Just a question about it... In the guide for how to write tests it is mentioned that you should "always use common.PORT instead of an arbitrary value as it allows to run tests in parallel safely". It doesn't mention that you can use port 0 as well. If it is preferable to use port 0 than maybe the guide should be updated? Or did I misunderstand the guide?

No, you're right. The guide was written just before changing the policy regarding the port. It should be updated to reflect this change.

I have updated the PR with 0 as port number now. Thanks for the feedback.

santigimeno · 2016-09-21T07:27:29Z

test/parallel/test-http-response-remove-header-after-sent.js

+server.listen(common.PORT, () => {
+  http.get({port: server.address().port}, () => {
+    server.close()
+  });


santigimeno · 2016-09-21T07:29:52Z

test/parallel/test-http-response-add-header-after-sent.js

+  assert.throws(() => {
+    res.setHeader('header2', 2);
+  }, Error, 'setHeader after write should throw');
+  res.end()


I think it can be useful checking the error message too. 'Can\'t set headers after they are sent.'

I went back and forth with this one. I can see why it could be useful to check the error message, but I can also see why it could be problematic. The way I see it is that the error message is an "implementation detail" and not part of the api and therefore you make the test more fragile if you test the error message. I don't think the test should fail if you change the error message, but on the other hand how do you know if it is the "right" Error? I'm not sure what the best way to do this is, in this context, and I'm more than happy to discuss this.
Btw, this is my first PR for the node-project and I really appreciate your feedback.

If I'm not wrong changing the error message is considered a breaking change so I'm +1 for also checking the error message.

Well, if that's considered a breaking change then I'm also for checking the error message :-)
Thank you for the clarification. I will fix it.

I have fixed the error message check now.

santigimeno · 2016-09-21T07:30:05Z

test/parallel/test-http-response-remove-header-after-sent.js

+  assert.throws(() => {
+    res.removeHeader('header2', 2);
+  }, Error, 'removeHeader after write should throw');
+  res.end()


Check error message here too.

santigimeno · 2016-09-21T13:49:49Z

You're missing some semicolons. Running make jslint will help you identifying formatting issues.

santigimeno

LGTM with the lint issues fixes and a green CI.

niklasi · 2016-09-21T14:03:32Z

I have fixed the semicolons now. Thanks for the tip with make jslint.

santigimeno · 2016-09-21T14:05:59Z

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

santigimeno · 2016-09-21T15:15:15Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/4193/ the last one disappeared :o

jasnell

LGTM. The tests could be condensed into one but this is perfectly fine also.

lpinca · 2016-09-21T16:18:26Z

test/parallel/test-http-response-add-header-after-sent.js

+  res.write('abc');
+  assert.throws(() => {
+    res.setHeader('header2', 2);
+  }, /Can't set headers after they are sent./);


Nit: the final dot should be escaped or removed.

I have fixed that now.

lpinca

LGTM

imyller

LGTM

imyller · 2016-09-23T21:31:42Z

I'll start landing this:

Four LGTMs
No objections
Requested changes have been made
CI test passed (only unrelated CI failures)

imyller · 2016-09-23T21:38:29Z

landed in a5994f7

Thank you for your contribution, @niklasi

This change adds tests to make sure an Error is thrown if a header is added or removed after they are sent. PR-URL: #8682 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

test: Add tests for add/remove header after sent.

361542b

This change adds tests to make sure an Error is thrown if a header is added or removed after they are sent.

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 21, 2016

mscdex added the http Issues or PRs related to the http subsystem. label Sep 21, 2016

santigimeno reviewed Sep 21, 2016

View reviewed changes

niklasi added 2 commits September 21, 2016 12:47

Use port 0 instead of common.PORT.

8787d21

Check error messages.

b8ed5ee

santigimeno approved these changes Sep 21, 2016

View reviewed changes

Fix missing semicolon.

a72b6fc

jasnell approved these changes Sep 21, 2016

View reviewed changes

lpinca reviewed Sep 21, 2016

View reviewed changes

Escape '.' when checking error message.

60a1081

lpinca approved these changes Sep 22, 2016

View reviewed changes

imyller approved these changes Sep 23, 2016

View reviewed changes

imyller self-assigned this Sep 23, 2016

imyller closed this Sep 23, 2016

imyller removed their assignment Sep 23, 2016

MylesBorins added the land-on-v4.x label Oct 6, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

snyk-bot mentioned this pull request Jul 17, 2020

[Snyk] Security upgrade eslint from 3.19.0 to 4.0.0 adamlaska/node#38

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add tests for add/remove header after sent. #8682

test: Add tests for add/remove header after sent. #8682

niklasi commented Sep 21, 2016

santigimeno Sep 21, 2016

mscdex Sep 21, 2016

niklasi Sep 21, 2016

santigimeno Sep 21, 2016

niklasi Sep 21, 2016

santigimeno Sep 21, 2016

santigimeno Sep 21, 2016

niklasi Sep 21, 2016 •

edited

Loading

lpinca Sep 21, 2016

niklasi Sep 21, 2016

niklasi Sep 21, 2016

santigimeno Sep 21, 2016

santigimeno commented Sep 21, 2016

santigimeno left a comment

niklasi commented Sep 21, 2016

santigimeno commented Sep 21, 2016

santigimeno commented Sep 21, 2016

jasnell left a comment

lpinca Sep 21, 2016

niklasi Sep 21, 2016

lpinca left a comment

imyller left a comment

imyller commented Sep 23, 2016

imyller commented Sep 23, 2016

test: Add tests for add/remove header after sent. #8682

test: Add tests for add/remove header after sent. #8682

Conversation

niklasi commented Sep 21, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasi Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santigimeno commented Sep 21, 2016

santigimeno left a comment

Choose a reason for hiding this comment

niklasi commented Sep 21, 2016

santigimeno commented Sep 21, 2016

santigimeno commented Sep 21, 2016

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca left a comment

Choose a reason for hiding this comment

imyller left a comment

Choose a reason for hiding this comment

imyller commented Sep 23, 2016

imyller commented Sep 23, 2016

niklasi Sep 21, 2016 •

edited

Loading