test: refactor test-http-response-splitting #11429

notarseniy · 2017-02-16T20:34:44Z

move repeated code to function
remove unneeded common.mustCall() usage with function arguments that
are not callbacks
add error message checking

like #11274.

Checklist

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

Affected core subsystem(s)

test http

jasnell

Almost there

jasnell · 2017-02-16T20:38:28Z

test/parallel/test-http-response-splitting.js

+function test(res, code, header) {
+  assert.throws(() => {
+    res.writeHead(code, header);
+  });


This should actually be updated to test the actual error message

thank you! implemented.

notarseniy · 2017-02-16T21:10:16Z

Thanks for review :-) Recommited with error message checking and with new commit message.

cjihrig · 2017-02-16T21:14:08Z

test/parallel/test-http-response-splitting.js

@@ -19,23 +19,26 @@ const y = 'foo⠊Set-Cookie: foo=bar';

 let count = 0;

+function test(res, code, header) {
+  const errRegExp = new RegExp(
+    'TypeError: The header content contains invalid characters'


Can you add ^ and $ to the regular expression. You can also use a regex literal instead of the RegExp() constructor.

Yeah, seems better. Thank you for pointing this out! Fixed.

notarseniy · 2017-02-16T21:33:10Z

Re-forcepushed again with fixes. Feel free to commenting anything more :)

cjihrig

LGTM. Only comment is that it might look a little cleaner to drop errRegExp, and just put the regex inline.

* move repeated code to function * remove unneeded `common.mustCall()` usage with function arguments that are not callbacks * add error message checking

targos · 2017-02-18T14:45:34Z

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

notarseniy · 2017-02-18T19:01:16Z

Why test on armv8-ubuntu1404 is break down? This test is passed, but there's hudson-related issue (unrelated to this PR):

ok 1397 sequential/test-vm-timeout-rethrow
  ---
  duration_ms: 0.510
  ...
make[1]: Leaving directory `/home/iojs/build/workspace/node-test-commit-arm/nodes/armv8-ubuntu1404'
[armv8-ubuntu1404] $ /bin/sh -xe /tmp/hudson5359784332414623123.sh
+ set +x
Sat Feb 18 07:00:53 PST 2017
+ pgrep node
+ true
FATAL: command execution failed
hudson.remoting.ChannelClosedException: channel is already closed
	at hudson.remoting.Channel.send(Channel.java:604)
	at hudson.remoting.Request.call(Request.java:130)
	at hudson.remoting.Channel.call(Channel.java:821)
	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:256)
	at com.sun.proxy.$Proxy106.isAlive(Unknown Source)
	at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1043)
	at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1035)
	at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:154)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:108)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:65)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
	at hudson.model.Build$BuildExecution.build(Build.java:205)
	at hudson.model.Build$BuildExecution.doRun(Build.java:162)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:534)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)
Caused by: java.io.IOException: Connection aborted: org.jenkinsci.remoting.nio.NioChannelHub$MonoNioTransport@ffbeda2[name=Channel to /121.212.45.83]
	at org.jenkinsci.remoting.nio.NioChannelHub$NioTransport.abort(NioChannelHub.java:210)
	at org.jenkinsci.remoting.nio.NioChannelHub.run(NioChannelHub.java:635)
	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.IOException: Connection timed out
	at sun.nio.ch.FileDispatcherImpl.read0(Native Method)
	at sun.nio.ch.SocketDispatcher.read(SocketDispatcher.java:39)
	at sun.nio.ch.IOUtil.readIntoNativeBuffer(IOUtil.java:223)
	at sun.nio.ch.IOUtil.read(IOUtil.java:197)
	at sun.nio.ch.SocketChannelImpl.read(SocketChannelImpl.java:380)
	at org.jenkinsci.remoting.nio.FifoBuffer$Pointer.receive(FifoBuffer.java:142)
	at org.jenkinsci.remoting.nio.FifoBuffer.receive(FifoBuffer.java:359)
	at org.jenkinsci.remoting.nio.NioChannelHub.run(NioChannelHub.java:564)
	... 6 more
Build step 'Execute shell' marked build as failure
[Current build status] check if current [FAILURE] is worse or equals then [UNSTABLE] and better or equals then [SUCCESS]
Run condition [Current build status] preventing perform for step [[Execute a set of scripts]]
[Current build status] check if current [FAILURE] is worse or equals then [ABORTED] and better or equals then [FAILURE]
Run condition [Current build status] enabling perform for step [[Execute a set of scripts]]
ERROR: Step ?Publish TAP Results? failed: no workspace for node-test-commit-arm/nodes=armv8-ubuntu1404 #7848
Checking ^not ok
ERROR: Build step failed with exception
java.lang.NullPointerException
Build step 'Jenkins Text Finder' marked build as failure
Notifying upstream projects of job completion
Finished: FAILURE

gibfahn · 2017-02-19T07:11:14Z

Why test on armv8-ubuntu1404 is break down? This test is passed, but there's hudson-related issue (unrelated to this PR):

Probably related to nodejs/build#611

* move repeated code to function * remove unneeded `common.mustCall()` usage with function arguments that are not callbacks * add error message checking PR-URL: #11429 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

jasnell · 2017-02-19T20:53:58Z

Landed in 6647688

* move repeated code to function * remove unneeded `common.mustCall()` usage with function arguments that are not callbacks * add error message checking PR-URL: #11429 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 16, 2017

jasnell requested changes Feb 16, 2017

View reviewed changes

notarseniy force-pushed the http-response-splitting branch 2 times, most recently from e84501e to 42dbf60 Compare February 16, 2017 21:05

cjihrig requested changes Feb 16, 2017

View reviewed changes

notarseniy force-pushed the http-response-splitting branch 4 times, most recently from 308738e to a84c506 Compare February 16, 2017 21:25

cjihrig approved these changes Feb 16, 2017

View reviewed changes

test: refactor test-http-response-splitting

fc16912

* move repeated code to function * remove unneeded `common.mustCall()` usage with function arguments that are not callbacks * add error message checking

notarseniy force-pushed the http-response-splitting branch from a84c506 to fc16912 Compare February 16, 2017 21:43

mscdex added the http Issues or PRs related to the http subsystem. label Feb 16, 2017

jasnell approved these changes Feb 16, 2017

View reviewed changes

lpinca approved these changes Feb 17, 2017

View reviewed changes

targos approved these changes Feb 18, 2017

View reviewed changes

ChALkeR approved these changes Feb 19, 2017

View reviewed changes

jasnell closed this Feb 19, 2017

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

This was referenced Jul 6, 2023

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#23

Open

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#57

Open

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#77

Open

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#147

Open

aliscco mentioned this pull request Jul 10, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#628

Open

leonardoadame mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 leonardoadame/node#199

Open

This was referenced Nov 29, 2023

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#785

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#799

Open

leonardoadame mentioned this pull request Nov 30, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#281

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#1050

Open

leonardoadame mentioned this pull request Dec 5, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#539

Open

aliscco mentioned this pull request Dec 8, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1158

Open

This was referenced Dec 10, 2023

[Snyk] Security upgrade jest from 25.5.4 to 27.0.0 leonardoadame/node#577

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 leonardoadame/node#578

Open

This was referenced Dec 10, 2023

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#1165

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1167

Open

leonardoadame mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 leonardoadame/node#581

Open

aliscco mentioned this pull request Jan 1, 2024

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#1182

Open

aliscco mentioned this pull request Jan 31, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1205

Open

leonardoadame mentioned this pull request Feb 12, 2024

[Snyk] Fix for 3 vulnerabilities leonardoadame/node#684

Open

leonardoadame mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#831

Open

aliscco mentioned this pull request May 15, 2024

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1499

Open

leonardoadame mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 leonardoadame/node#963

Open

This was referenced May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1578

Open

[Snyk] Security upgrade jest from 26.4.2 to 27.0.0 aliscco/alisco-node#1581

Open

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 aliscco/alisco-node#1601

Open

leonardoadame mentioned this pull request May 19, 2024

[Snyk] Security upgrade jest from 26.6.3 to 27.0.0 leonardoadame/node#998

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactor test-http-response-splitting #11429

test: refactor test-http-response-splitting #11429

notarseniy commented Feb 16, 2017 •

edited

Loading

jasnell left a comment

jasnell Feb 16, 2017

notarseniy Feb 16, 2017

notarseniy commented Feb 16, 2017

cjihrig Feb 16, 2017

notarseniy Feb 16, 2017

notarseniy commented Feb 16, 2017 •

edited

Loading

cjihrig left a comment

targos commented Feb 18, 2017

notarseniy commented Feb 18, 2017 •

edited

Loading

gibfahn commented Feb 19, 2017

jasnell commented Feb 19, 2017

test: refactor test-http-response-splitting #11429

test: refactor test-http-response-splitting #11429

Conversation

notarseniy commented Feb 16, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

jasnell Feb 16, 2017

Choose a reason for hiding this comment

notarseniy Feb 16, 2017

Choose a reason for hiding this comment

notarseniy commented Feb 16, 2017

cjihrig Feb 16, 2017

Choose a reason for hiding this comment

notarseniy Feb 16, 2017

Choose a reason for hiding this comment

notarseniy commented Feb 16, 2017 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

targos commented Feb 18, 2017

notarseniy commented Feb 18, 2017 • edited Loading

gibfahn commented Feb 19, 2017

jasnell commented Feb 19, 2017

notarseniy commented Feb 16, 2017 •

edited

Loading

notarseniy commented Feb 16, 2017 •

edited

Loading

notarseniy commented Feb 18, 2017 •

edited

Loading