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-fs-read-stream-inherit #13618

Closed
wants to merge 1 commit into from
Closed

test: refactor test-fs-read-stream-inherit #13618

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
115 changes: 59 additions & 56 deletions test/parallel/test-fs-read-stream-inherit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const path = require('path');
const assert = require('assert');
const fs = require('fs');
const path = require('path');

const fn = path.join(common.fixturesDir, 'elipses.txt');
const rangeFile = path.join(common.fixturesDir, 'x.txt');

let paused = false;

{
let paused = false;

const file = fs.ReadStream(fn);

file.on('open', common.mustCall(function(fd) {
Expand Down Expand Up @@ -48,61 +49,61 @@ let paused = false;
}

{
const file3 = fs.createReadStream(fn, Object.create({encoding: 'utf8'}));
file3.length = 0;
file3.on('data', function(data) {
const file = fs.createReadStream(fn, Object.create({encoding: 'utf8'}));
file.length = 0;
file.on('data', function(data) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any preferences whether to use lambdas or good old function declarations when they are interchangeable?

Copy link
Member

Choose a reason for hiding this comment

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

By lambdas you mean () => {} (fat arrow) functions? I don't think we have an official policy (although I personally prefer fat arrow functions because they seem simpler).

Copy link
Member

Choose a reason for hiding this comment

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

@gibfahn Exactly, that would be my preference as well.

assert.strictEqual(typeof data, 'string');
file3.length += data.length;
file.length += data.length;

for (let i = 0; i < data.length; i++) {
// http://www.fileformat.info/info/unicode/char/2026/index.htm
assert.strictEqual(data[i], '\u2026');
}
});

file3.on('close', common.mustCall(function() {
assert.strictEqual(file3.length, 10000);
file.on('close', common.mustCall(function() {
assert.strictEqual(file.length, 10000);
}));
}

{
const options = Object.create({bufferSize: 1, start: 1, end: 2});
const file4 = fs.createReadStream(rangeFile, options);
assert.strictEqual(file4.start, 1);
assert.strictEqual(file4.end, 2);
const file = fs.createReadStream(rangeFile, options);
assert.strictEqual(file.start, 1);
assert.strictEqual(file.end, 2);
let contentRead = '';
file4.on('data', function(data) {
file.on('data', function(data) {
contentRead += data.toString('utf-8');
});
file4.on('end', common.mustCall(function() {
file.on('end', common.mustCall(function() {
assert.strictEqual(contentRead, 'yz');
}));
}

{
const options = Object.create({bufferSize: 1, start: 1});
const file5 = fs.createReadStream(rangeFile, options);
assert.strictEqual(file5.start, 1);
file5.data = '';
file5.on('data', function(data) {
file5.data += data.toString('utf-8');
const file = fs.createReadStream(rangeFile, options);
assert.strictEqual(file.start, 1);
file.data = '';
file.on('data', function(data) {
file.data += data.toString('utf-8');
});
file5.on('end', common.mustCall(function() {
assert.strictEqual(file5.data, 'yz\n');
file.on('end', common.mustCall(function() {
assert.strictEqual(file.data, 'yz\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're here could you parameterize the 'yz\n', here and in L106

}));
}

// https://github.com/joyent/node/issues/2320
{
const options = Object.create({bufferSize: 1.23, start: 1});
const file6 = fs.createReadStream(rangeFile, options);
assert.strictEqual(file6.start, 1);
file6.data = '';
file6.on('data', function(data) {
file6.data += data.toString('utf-8');
const file = fs.createReadStream(rangeFile, options);
assert.strictEqual(file.start, 1);
file.data = '';
file.on('data', function(data) {
file.data += data.toString('utf-8');
});
file6.on('end', common.mustCall(function() {
assert.strictEqual(file6.data, 'yz\n');
file.on('end', common.mustCall(function() {
assert.strictEqual(file.data, 'yz\n');
}));
}

Expand Down Expand Up @@ -136,56 +137,58 @@ let paused = false;
}

{
let file7 =
let data = '';
let file =
fs.createReadStream(rangeFile, Object.create({autoClose: false }));
assert.strictEqual(file7.autoClose, false);
file7.on('data', common.noop);
file7.on('end', common.mustCall(function() {
assert.strictEqual(file.autoClose, false);
file.on('data', (chunk) => { data += chunk; });
file.on('end', common.mustCall(function() {
process.nextTick(common.mustCall(function() {
assert(!file7.closed);
assert(!file7.destroyed);
file7Next();
assert(!file.closed);
assert(!file.destroyed);
assert.strictEqual(data, 'xyz\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

fileNext();
}));
}));

function file7Next() {
function fileNext() {
// This will tell us if the fd is usable again or not.
file7 = fs.createReadStream(null, Object.create({fd: file7.fd, start: 0 }));
file7.data = '';
file7.on('data', function(data) {
file7.data += data;
file = fs.createReadStream(null, Object.create({fd: file.fd, start: 0 }));
Copy link
Contributor

Choose a reason for hiding this comment

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

this is just weird, shadowing file but AFAICT only affects L147-8 , so there it asserts the state of this file 🤔
IMHO it's wrong, but maybe not for this PR...

file.data = '';
file.on('data', function(data) {
file.data += data;
});
file7.on('end', common.mustCall(function() {
assert.strictEqual(file7.data, 'xyz\n');
file.on('end', common.mustCall(function() {
assert.strictEqual(file.data, 'xyz\n');
}));
}
process.on('exit', function() {
assert(file7.closed);
assert(file7.destroyed);
assert(file.closed);
assert(file.destroyed);
});
}

// Just to make sure autoClose won't close the stream because of error.
{
const options = Object.create({fd: 13337, autoClose: false});
const file8 = fs.createReadStream(null, options);
file8.on('data', common.noop);
file8.on('error', common.mustCall());
const file = fs.createReadStream(null, options);
file.on('data', common.mustNotCall());
file.on('error', common.mustCall());
process.on('exit', function() {
assert(!file8.closed);
assert(!file8.destroyed);
assert(file8.fd);
assert(!file.closed);
assert(!file.destroyed);
assert(file.fd);
});
}

// Make sure stream is destroyed when file does not exist.
{
const file9 = fs.createReadStream('/path/to/file/that/does/not/exist');
file9.on('data', common.noop);
file9.on('error', common.mustCall());
const file = fs.createReadStream('/path/to/file/that/does/not/exist');
file.on('data', common.mustNotCall());
file.on('error', common.mustCall());

process.on('exit', function() {
assert(!file9.closed);
assert(file9.destroyed);
assert(!file.closed);
assert(file.destroyed);
});
}