Skip to content

Fix flakey file system tests #10541

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

Merged
merged 1 commit into from
Mar 13, 2020
Merged

Fix flakey file system tests #10541

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
30 changes: 18 additions & 12 deletions src/test/common/platform/filesystem.functional.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import * as fs from 'fs-extra';
import { convertStat, FileSystem, FileSystemUtils, RawFileSystem } from '../../../client/common/platform/fileSystem';
import { FileSystemPaths, FileSystemPathUtils } from '../../../client/common/platform/fs-paths';
import { FileType } from '../../../client/common/platform/types';
import { sleep } from '../../../client/common/utils/async';
import { createDeferred, sleep } from '../../../client/common/utils/async';
import { noop } from '../../../client/common/utils/misc';
import {
assertDoesNotExist,
assertFileText,
Expand Down Expand Up @@ -267,13 +268,24 @@ suite('FileSystem - raw', () => {
}
});

async function writeToStream(filename: string, write: (str: fs.WriteStream) => void) {
const closeDeferred = createDeferred();
const stream = fileSystem.createWriteStream(filename);
stream.on('close', () => closeDeferred.resolve());
Copy link

Choose a reason for hiding this comment

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

Isn't the requirement to check whether createWriteStream returns a writable stream.
At the end of the day the API is createWriteStream and that's what needs to be tested, not testing the functionality of stream object returned by nodejs.

There are other methods on streams that we're not testing here, pipe, event handlers, etc..
I think the test is testing the wrong thing. We're testing if node streams work.

I'd suggest removing all the tests and changing to:

const isStream = require('is-stream');
const stream = fileSystem.createWriteStream(filename);
assert.isOk(isStream.writable(stream))

Copy link

Choose a reason for hiding this comment

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

Also, listening to close isn't sufficient, we'd need to listen to flush event to know if data was flushed. But that's not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

There is no flush method on the write stream.

And I didn't want to change the tests here, just make them pass. Who ever owns these can make that change.

Copy link

Choose a reason for hiding this comment

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

  • Check for flush event
  • Call end method.

Who ever owns these can make that change.

@karthiknadig do we need such tests (see my comments).
Its just more unnecessary flaky tests. Also slower

Copy link
Member

Choose a reason for hiding this comment

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

I think testing that it returns a writable stream should be enough. If I remember correctly this particular API was not completely supported or we had some asks on VSCode. /cc @ericsnowcurrently for more info.

Copy link
Author

Choose a reason for hiding this comment

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

@ericsnowcurrently I can make the change suggested if you like.

Copy link
Author

Choose a reason for hiding this comment

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

I'm just going to submit. Eric can change the tests later if he likes.

Copy link

Choose a reason for hiding this comment

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

These functional tests verify the behavior that the extension relies on. This matters because the node and vscode APIs don't have exactly the same behavior. Other similar functional tests uncovered such cases, showing why it's important to verify the expected behavior (to an extent).

Copy link

Choose a reason for hiding this comment

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

@rchiodo, as to checking for flush and calling .end(), I don't see a problem offhand. I'd have to double-check the node docs, but honestly, I trust Don. 😄

Copy link

Choose a reason for hiding this comment

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

There are other methods on streams that we're not testing here, pipe, event handlers, etc..

True. And if the extension relies on those then we should consider testing them. Then again, it probably isn't worth the effort until the VS Code FS API provides some sort of WriteStream. My vote is to not add more tests. Regardless, it should be handled in a separate issue.

write(stream);
stream.end();
stream.close();
stream.destroy();
await closeDeferred.promise;
return stream;
}

test('returns the correct WriteStream', async () => {
const filename = await fix.resolve('x/y/z/spam.py');
const expected = fs.createWriteStream(filename);
expected.destroy();

const stream = fileSystem.createWriteStream(filename);
stream.destroy();
const stream = await writeToStream(filename, noop);

expect(stream.path).to.deep.equal(expected.path);
});
Expand All @@ -283,9 +295,7 @@ suite('FileSystem - raw', () => {
await assertDoesNotExist(filename);
const data = 'line1\nline2\n';

const stream = fileSystem.createWriteStream(filename);
stream.write(data);
stream.destroy();
await writeToStream(filename, s => s.write(data));
Copy link

Choose a reason for hiding this comment

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

FWIW, I'm a big believer of keeping as much of the test-targeted code in the test function as possible. The following variation on your solution would do that. It uses a sort of context-manager style that reduces the amount of code the test cares about without obscuring the code the test does care about:

Suggested change
await writeToStream(filename, s => s.write(data));
const stream = fileSystem.createWriteStream(filename);
await withClosing(stream, () => {
stream.write(data);
});

This would rely on the following helper, derived from writeToStream():

        async function withClosing(stream: WriteStream, test: () => void) {
            const closeDeferred = createDeferred();
            stream.on('close', () => closeDeferred.resolve());

            test()

            stream.end();
            stream.close();
            stream.destroy();
            await closeDeferred.promise;
        }

Other approaches (in order to most-to-least readable result):

  • for tests on operations on the stream (rather than its creation), we could use a helper to create the stream:
    • function createWriteStream(filename: string): [WriteStream, () => Promise<void>] { ... }
  • combine that with the context-manager style:
    • async function withStream(filename: string, (stream: WriteStream) => Promise<void>) { ... }
  • be more exliicit about it (1 helpers):
    • function getCloser(stream: WriteStream): () => Promise<void> { ... }
  • be more exliicit about it (2 helpers):
    • function prepForClose(stream: WriteStream): Promise<void> { ... }
    • function destroyStream(stream: WriteStream) { ... }

Copy link
Author

Choose a reason for hiding this comment

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

Sorry already submitted. Feel free to update if you like.

There is no flush method on a stream though, so don't add that:
https://nodejs.org/api/stream.html

Copy link
Author

Choose a reason for hiding this comment

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

There's actually a 'finish' event that fires after all data has been written. That's likely better than the close event.


await assertFileText(filename, data);
});
Expand All @@ -294,9 +304,7 @@ suite('FileSystem - raw', () => {
const filename = await fix.resolve('x/y/z/spam.py');
const data = '... 😁 ...';

const stream = fileSystem.createWriteStream(filename);
stream.write(data);
stream.destroy();
await writeToStream(filename, s => s.write(data));

await assertFileText(filename, data);
});
Expand All @@ -305,9 +313,7 @@ suite('FileSystem - raw', () => {
const filename = await fix.createFile('x/y/z/spam.py', '...');
const data = 'line1\nline2\n';

const stream = fileSystem.createWriteStream(filename);
stream.write(data);
stream.destroy();
await writeToStream(filename, s => s.write(data));

await assertFileText(filename, data);
});
Expand Down