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

test: refactor test-http-information-processing #32547

Merged
merged 1 commit into from
Mar 31, 2020
Merged

test: refactor test-http-information-processing #32547

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
27 changes: 12 additions & 15 deletions test/parallel/test-http-information-processing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,21 @@
require('../common');
const assert = require('assert');
const http = require('http');
const Countdown = require('../common/countdown');

const test_res_body = 'other stuff!\n';
const countdown = new Countdown(3, () => server.close());
const testResBody = 'other stuff!\n';
const kMessageCount = 2;

const server = http.createServer((req, res) => {
console.error('Server sending informational message #1...');
res.writeProcessing();
console.error('Server sending informational message #2...');
res.writeProcessing();
for (let i = 0; i < kMessageCount; i++) {
console.error(`Server sending informational message #${i}...`);
Trott marked this conversation as resolved.
Show resolved Hide resolved
res.writeProcessing();
}
console.error('Server sending full response...');
res.writeHead(200, {
'Content-Type': 'text/plain',
'ABCD': '1'
});
res.end(test_res_body);
res.end(testResBody);
Copy link
Member

Choose a reason for hiding this comment

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

common.mustCall variant can be used here.

Copy link
Member Author

@Trott Trott Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We could, but I'm not sure we should. This test isn't checking to see if res.end() fires its callback or not. If the response isn't sent, the test will fail. We don't need the extra check. Adding checks that are unrelated to the test case makes it harder for someone reading the code to understand what the test is actually supposed to be testing.

This makes it sound like I'm much more opposed to the idea than I actually am. I'm on the fence, around a -0 or so.

I was a -1 about 10 minutes ago, so 10 minutes from now, I'll probably be an enthusiastic +1.

Copy link
Member

Choose a reason for hiding this comment

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

This test isn't checking to see if res.end() fires its callback or not

correct me if I am wrong, but isn't that the case with most of 100+ http test cases?

  • intent of the test does not cover explicit checking on res.end
  • but as a matter of practice, they do that check
    if you can generalize a statement around whether http tests should check life cycle events are met or not I can also work towards implementing this across other tests too.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • intent of the test does not cover explicit checking on res.end
  • but as a matter of practice, they do that check

I'm not seeing that with res.end() calls. I'm using grep and doing a naive/superficial check, so results may be imperfect but close enough to draw reasonable conclusions:

  • Almost none of the 200+ calls to res.end() in parallel/test-http* have a callback wrapped in mustCall(). The majority of them are like the call here where res.endI() is called and no callback is provided.
  • Of those that do wrap a callback in muscCall(), only one file checks empty callback: test-http-outgoing-end-multiple.js. That makes sense for that test because it is literally checking that calling res.end() multiple times will work as expected.

I don't think we should add a check for a callback here that we're not using. Again, I'm not strongly opposed, but I am mildly resistant. 😀

Copy link
Member

Choose a reason for hiding this comment

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

I used this command grep -e "res.on('end" -e "response.on('end" test-http*.js | grep "mustCall" | wc -l and got around 50 instances. Unfortunately I don't have the insight to validate their context. so I will leave it at that. Thank you!

});

server.listen(0, function() {
Expand All @@ -29,24 +28,22 @@ server.listen(0, function() {
console.error('Client sending request...');

let body = '';
let infoCount = 0;

req.on('information', function(res) {
console.error('Client got 102 Processing...');
countdown.dec();
});
req.on('information', () => { infoCount++; });
Trott marked this conversation as resolved.
Show resolved Hide resolved

req.on('response', function(res) {
// Check that all 102 Processing received before full response received.
assert.strictEqual(countdown.remaining, 1);
assert.strictEqual(infoCount, kMessageCount);
assert.strictEqual(res.statusCode, 200,
`Final status code was ${res.statusCode}, not 200.`);
res.setEncoding('utf8');
res.on('data', function(chunk) { body += chunk; });
res.on('end', function() {
console.error('Got full response.');
assert.strictEqual(body, test_res_body);
assert.strictEqual(body, testResBody);
assert.ok('abcd' in res.headers);
countdown.dec();
server.close();
});
});
});