Skip to content

Commit

Permalink
Fix worker-test and fs.createWriteStream spec incompliance
Browse files Browse the repository at this point in the history
Summary:
The CircleCI build of metro is currently failing because `jest` fails to mock the `console` module. This is because the typeof console changed in the latest node version ([PR](nodejs/node#35399)). Meaning that the typeof `console` is `console` and Jest doesn't know how to mock a value of type `console`.

This diff manually mocks the `console` object to avoid this issue (verified with using Node 15 locally). However, the tests then started failing because the `close` event of streams was emitted more than once by our memory fs implementation.

The issue is that our `createWriteStream` implementetion does not specify the `emitClose: false` when creating the stream as required according to the [docs](https://nodejs.org/api/fs.html#fs_fs_createwritestream_path_options):

> By default, the stream will not emit a 'close' event after it has been destroyed. This is the opposite of the default for other Writable streams. Set the emitClose option to true to change this behavior.

I changed that in our `memory-fs` implementation and removed the manual emit of the `close` event. I then had to change the tests because the order of the events is `end`, `finish`, `close` and the tests asserted in the `end` event.

The tests then started passing on Node 15 but, surprise, they now failed on Node 12...

The reason is that the [`stream.Writeable`](https://nodejs.org/docs/latest-v13.x/api/stream.html#stream_constructor_new_stream_writable_options) option `autoDestroy` has changed with Node13 from default false to default true. Hardcoding `default: true` finally makes all tests passing on Node 12 - 15.

Reviewed By: cpojer

Differential Revision: D25495034

fbshipit-source-id: fdf871fa4dfbec079bc9ec1843fcff7facb3e0be
  • Loading branch information
Micha Reiser authored and facebook-github-bot committed Dec 12, 2020
1 parent 663621c commit 7667838
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 17 deletions.
2 changes: 1 addition & 1 deletion packages/buck-worker-tool/src/__tests__/worker-test.js
Expand Up @@ -124,7 +124,7 @@ describe('Buck worker:', () => {
fs.createWriteStream = (...args) => {
const writeStream = createWriteStreamImpl(...args);
++openedStreams;
writeStream.on('close', () => --openedStreams);
writeStream.on('finish', () => --openedStreams);
return writeStream;
};
});
Expand Down
16 changes: 5 additions & 11 deletions packages/metro-memory-fs/src/__tests__/index-test.js
Expand Up @@ -103,32 +103,32 @@ describe('posix support', () => {

describe('createWriteStream', () => {
it('writes a file', done => {
const st = fs.createWriteStream('/foo.txt');
const st = fs.createWriteStream('/foo.txt', {emitClose: true});
let opened = false;
let closed = false;
st.on('open', () => (opened = true));
st.on('close', () => (closed = true));
st.write('test');
st.write(' foo');
st.end(() => {
st.end();

st.on('close', () => {
expect(opened).toBe(true);
expect(closed).toBe(true);
expect(fs.readFileSync('/foo.txt', 'utf8')).toEqual('test foo');

done();
});
});

it('writes a file, as buffer', done => {
const st = fs.createWriteStream('/foo.txt');
let opened = false;
let closed = false;
st.on('open', () => (opened = true));
st.on('close', () => (closed = true));
st.write(Buffer.from('test'));
st.write(Buffer.from(' foo'));
st.end(() => {
expect(opened).toBe(true);
expect(closed).toBe(true);
expect(fs.readFileSync('/foo.txt', 'utf8')).toEqual('test foo');
done();
});
Expand All @@ -138,13 +138,10 @@ describe('posix support', () => {
fs.writeFileSync('/foo.txt', 'test bar');
const st = fs.createWriteStream('/foo.txt', {start: 5, flags: 'r+'});
let opened = false;
let closed = false;
st.on('open', () => (opened = true));
st.on('close', () => (closed = true));
st.write('beep');
st.end(() => {
expect(opened).toBe(true);
expect(closed).toBe(true);
expect(fs.readFileSync('/foo.txt', 'utf8')).toEqual('test beep');
done();
});
Expand All @@ -154,13 +151,10 @@ describe('posix support', () => {
const fd = fs.openSync('/bar.txt', 'w');
const st = fs.createWriteStream('/foo.txt', {fd});
let opened = false;
let closed = false;
st.on('open', () => (opened = true));
st.on('close', () => (closed = true));
st.write('beep boop');
st.end(() => {
expect(opened).toBe(false);
expect(closed).toBe(true);
expect(fs.readFileSync('/bar.txt', 'utf8')).toEqual('beep boop');
done();
});
Expand Down
18 changes: 13 additions & 5 deletions packages/metro-memory-fs/src/index.js
Expand Up @@ -1027,6 +1027,7 @@ class MemoryFs {
flags?: string,
mode?: number,
start?: number,
emitClose?: boolean,
...
}
| Encoding,
Expand All @@ -1037,30 +1038,36 @@ class MemoryFs {
autoClose?: boolean,
encoding?: Encoding,
fd?: ?number,
emitClose?: boolean,
flags?: string,
mode?: number,
start?: number,
...
}
| Encoding,
) => {
let autoClose, fd, flags, mode, start;
let autoClose, fd, flags, mode, start, emitClose;
if (typeof options !== 'string' && options != null) {
({autoClose, fd, flags, mode, start} = options);
({autoClose, fd, flags, mode, start, emitClose} = options);
}
let st = null;
if (fd == null) {
fd = this._open(pathStr(filePath), flags || 'w', mode);
process.nextTick(() => (st: any).emit('open', fd));
}
const ffd = fd;
const ropt = {fd, writeSync: this._write.bind(this), filePath, start};
const ropt = {
fd,
writeSync: this._write.bind(this),
filePath,
start,
emitClose: emitClose ?? false,
};
const rst = new WriteFileStream(ropt);
st = rst;
if (autoClose !== false) {
const doClose = () => {
this.closeSync(ffd);
rst.emit('close');
};
rst.on('finish', doClose);
rst.on('error', doClose);
Expand Down Expand Up @@ -1570,9 +1577,10 @@ class WriteFileStream extends stream.Writable {
filePath: FilePath,
writeSync: WriteSync,
start?: number,
emitClose?: boolean,
...
}) {
super();
super({emitClose: opts.emitClose, autoDestroy: true});
this.path = opts.filePath;
this.bytesWritten = 0;
this._fd = opts.fd;
Expand Down

0 comments on commit 7667838

Please sign in to comment.