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: revise test-http-client-default-headers-exist #32493

Merged
merged 1 commit into from
Mar 27, 2020
Merged

test: revise test-http-client-default-headers-exist #32493

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
19 changes: 7 additions & 12 deletions test/parallel/test-http-client-default-headers-exist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
const common = require('../common');
const assert = require('assert');
const http = require('http');
const Countdown = require('../common/countdown');

const { once } = require('events');

const expectedHeaders = {
'DELETE': ['host', 'connection'],
Expand All @@ -37,10 +38,6 @@ const expectedHeaders = {

const expectedMethods = Object.keys(expectedHeaders);

const countdown =
new Countdown(expectedMethods.length,
common.mustCall(() => server.close()));

const server = http.createServer(common.mustCall((req, res) => {
res.end();

Expand All @@ -49,9 +46,8 @@ const server = http.createServer(common.mustCall((req, res) => {

const requestHeaders = Object.keys(req.headers);
requestHeaders.forEach((header) => {
assert.strictEqual(
assert.ok(
expectedHeaders[req.method].includes(header.toLowerCase()),
true,
`${header} should not exist for method ${req.method}`
);
});
Expand All @@ -61,15 +57,14 @@ const server = http.createServer(common.mustCall((req, res) => {
expectedHeaders[req.method].length,
`some headers were missing for method: ${req.method}`
);

countdown.dec();
}, expectedMethods.length));

server.listen(0, common.mustCall(() => {
expectedMethods.forEach((method) => {
http.request({
Promise.all(expectedMethods.map(async (method) => {
Copy link
Member

Choose a reason for hiding this comment

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

It's not an issue with this particular test, but keep in mind that changing all of these to use Promises is changing the timing of these tests... rather than executing the countdown cleanup logic synchronously it is being deferred to the microtask queue. That could end up causing the test to miss issues that may exist when closing things down synchronously so worth watching... also, keep this issue in mind also when refactoring these :-) .. as I said, not an issue on this particular test but there are some where it may be relevant.

Copy link
Member Author

@Trott Trott Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I want to make sure I understand your point about the cleanup code and the microtask queue. Are you saying that with countdown, the callback that checks things executes synchronously whenever the pertinent .dec() call happens, whereas with Promise.all(), it is whenever the microtask queue runs the resolve callback in the .then() call, so it might execute at a different time and with things in a different state than countdown?

If so: That's something I hadn't considered and worth watching out for. Thanks for pointing it out! (And thanks for making me take a few minutes to re-learn about the microtask queue, which is something I almost never think about!)

const request = http.request({
method: method,
port: server.address().port
}).end();
});
return once(request, 'response');
})).then(common.mustCall(() => { server.close(); }));
}));