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-https-truncate #10225

Closed
wants to merge 1 commit into from
Closed

test: refactor test-https-truncate #10225

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
16 changes: 8 additions & 8 deletions test/parallel/test-https-truncate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
'use strict';
const common = require('../common');
const assert = require('assert');

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}

const assert = require('assert');
const https = require('https');

const fs = require('fs');
Expand All @@ -14,7 +15,7 @@ const key = fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem');
const cert = fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem');

// number of bytes discovered empirically to trigger the bug
const data = Buffer.allocUnsafe(1024 * 32 + 1);
const data = Buffer.alloc(1024 * 32 + 1);
Copy link
Member

Choose a reason for hiding this comment

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

May I ask why alloc is preferred over allocUnsafe in a test?

Copy link
Member Author

@Trott Trott Dec 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

May I ask why alloc is preferred over allocUnsafe in a test?

I don't feel strongly about it, but here's the thinking:

  • If we use Buffer.alloc(), then we get a buffer with predictable contents. While the contents don't play a role in the test here, all things being equal, I prefer having repeatable, predictable tests over tests with unpredictable inputs involved (unless, of course, the purpose is to do something like fuzz testing).

  • This is not performance-critical code, so the (small, in this case) performance hit of zero-filling is not a concern.

  • The very name of Buffer.allocUnsafe() seems to suggest "Don't use it unless you need to and know what you're doing." We don't need it here, so...

Again, I don't feel particularly strongly about it, but it does seem to me that if we can use either alloc() or allocUnsafe() in a test without any real difference, then we should default to alloc(). (The situation is different in lib where it stands to reason that tiny performance hits can add up to significant performance issues for some use cases. I'm only talking about test here.)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm fine with that, was just curious.


httpsTest();

Expand All @@ -36,19 +37,18 @@ function httpsTest() {
}


function test(res) {
res.on('end', function() {
const test = common.mustCall(function(res) {
res.on('end', common.mustCall(function() {
assert.strictEqual(res._readableState.length, 0);
assert.strictEqual(bytes, data.length);
console.log('ok');
});
}));

// Pause and then resume on each chunk, to ensure that there will be
// a lone byte hanging out at the very end.
let bytes = 0;
res.on('data', function(chunk) {
bytes += chunk.length;
this.pause();
setTimeout(this.resume.bind(this));
setTimeout(this.resume.bind(this), 1);
});
}
});