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.

Sign up for GitHub

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

Jump to bottom

test: refactor test-net-connect-buffer #17710

Closed
wants to merge 2 commits into from
Closed

test: refactor test-net-connect-buffer #17710

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

  • Use arrow functions, common.mustCall()
  • Remove redundant console.log()s/turn them into assertions
  • Use common.expectsError()
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/net

@addaleax
test: refactor test-net-connect-buffer
8c7c173 
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`
@addaleax addaleax added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Dec 16, 2017
@BridgeAR
Copy link
Member

It feels weird that null is handled special. I do not find a good explanation why it was not combined with the other error message (also not in the PR that introduced it #6170). Does anyone know why? And does it not make sense to combine them now?

@addaleax
Copy link
Member Author

@BridgeAR I would assume it's because null is special in object mode.

@nodejs/streams I would agree with @BridgeAR ... how do you feel about only performing the special null check when in object mode?

@mcollina
Copy link
Member

Actually null is still special also in the binary case. You have to .push(null) on a Readable to terminate.

@addaleax
Copy link
Member Author

Right … just seems odd since we also do the same type checks for .write(), not just .push()

lpinca
lpinca approved these changes Dec 16, 2017
View reviewed changes
test/parallel/test-net-connect-buffer.js Outdated

assert.strictEqual(socket.connecting, true);
assert.strictEqual('opening', socket.readyState);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: now that you are here, can you please swap the arguments? There are more below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, done!

lpinca
lpinca reviewed Dec 16, 2017
View reviewed changes
test/parallel/test-net-connect-buffer.js Outdated
let connectHappened = false;

const tcp = net.Server(function(s) {
const tcp = net.Server(common.mustCall((s) => {
tcp.close();

console.log('tcp server connection');
Copy link
Member

Choose a reason for hiding this comment

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

I would also remove the remaining console.log(), they don't seem to be very useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! :)

@mcollina
Copy link
Member

@addaleax that's an oddity of streams backward compatibility. I'm ok in changing that if you want to fire a separate PR.

mcollina
mcollina approved these changes Dec 18, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@maclover7
Copy link
Contributor

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

@maclover7
Copy link
Contributor

Landing...

@maclover7 maclover7 self-assigned this Dec 21, 2017
@maclover7
Copy link
Contributor

Landed in 7352bf2

@maclover7 maclover7 closed this Dec 21, 2017
maclover7 pushed a commit to maclover7/node that referenced this pull request Dec 21, 2017
@addaleax @maclover7
test: refactor test-net-connect-buffer
7352bf2 
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: nodejs#17710
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@addaleax @MylesBorins
test: refactor test-net-connect-buffer
f5a6239 
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: #17710
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@addaleax @MylesBorins
test: refactor test-net-connect-buffer
8609a3f 
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: #17710
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@addaleax @MylesBorins
test: refactor test-net-connect-buffer
60ae556 
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: #17710
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@MylesBorins
Copy link
Contributor

test doesn't pass on 6.x or 8.x (commit lands on both).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@maclover7 maclover7 maclover7 approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

@maclover7 maclover7

Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@addaleax @BridgeAR @mcollina @maclover7 @MylesBorins @jasnell @lpinca @cjihrig