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: http2 client socket operations #16211

Closed
wants to merge 1 commit into from
Closed

test: http2 client socket operations #16211

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions test/parallel/test-http2-client-socket-operations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const h2 = require('http2');

const server = h2.createServer();
server.on('stream', common.mustCall((stream) => {
stream.respond();
stream.end();
}));

server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
const req = client.request({ ':path': '/' });

req.on(
'response',
common.mustCall(() => {
client.socket.pause();
// Force 'pause' when _handle.reading is false
client.socket.emit('pause');
Copy link
Member

Choose a reason for hiding this comment

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

Reposting this since github incorrectly hides it.

I'll leave this up to @jasnell & @mcollina but I don't think this is actually testing what it claims to. Feels like a way to bump up code coverage without truly testing the functionality which is worse than not having a test since it hides potential problem areas.

Unfortunately I don't really have great suggestion for how to properly test it given how the socket is used in h2.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd tried mocking calls to readStart and readStop the same way as submitSettings is mocked as discussed in #16083 (comment)

However, the call was not reaching the mock. I'll wait for inputs from @jasnell / @mcollina
I agree that current commit doesn't confirm that pause, resume and drain methods were called.

Copy link
Member Author

@trivikr trivikr Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

One solution I can think of is ensuring that operations were called on client.socket and testing value stored in client.socket._handle.reading as follows:

client.socket.on('pause', common.mustCall(() => {
  assert(!client.socket._handle.reading);
}, 2));
client.socket.on('resume', common.mustCall(() => {
  assert(client.socket._handle.reading);
}, 2));
client.socket.on('drain', common.mustCall());


client.socket.resume();
// Force 'resume' when _handle.reading is true
client.socket.emit('resume');

client.socket.emit('drain');
})
);
req.on('data', common.mustNotCall());

req.on(
'end',
common.mustCall(() => {
server.close();
client.destroy();
})
);

// On the client, the close event must call
client.on('close', common.mustCall());
req.end();
}));