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.

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

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

Closed

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

- 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()


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!

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.

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

@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
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: #17710
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: #17710
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
- Use arrow functions, `common.mustCall()`
- Remove redundant `console.log()`s/turn them into assertions
- Use `common.expectsError()`

PR-URL: #17710
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
@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
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants