Fix flakey file system tests

[rchiodo]

File system tests seem to fail randomly. I believe it's because they're not waiting for the file write to finish.

Here's an example failure:
https://dev.azure.com/ms/vscode-python/_build/results?buildId=67769&view=ms.vss-test-web.build-test-results-tab&runId=2169996&resultId=100021&paneView=debug

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[codecov-io]

Codecov Report

Merging #10541 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10541      +/-   ##
==========================================
- Coverage   60.76%   60.74%   -0.03%     
==========================================
  Files         578      578              
  Lines       31355    31355              
  Branches     4463     4464       +1     
==========================================
- Hits        19053    19046       -7     
- Misses      11335    11339       +4     
- Partials      967      970       +3     

Impacted Files	Coverage Δ
src/datascience-ui/react-common/arePathsSame.ts	75.00% <0.00%> (-12.50%)	⬇️
src/client/common/utils/platform.ts	64.70% <0.00%> (-11.77%)	⬇️
src/client/linters/pydocstyle.ts	86.66% <0.00%> (-2.23%)	⬇️
src/client/datascience/debugLocationTracker.ts	76.56% <0.00%> (-1.57%)	⬇️
src/client/debugger/extension/adapter/factory.ts	94.28% <0.00%> (-1.43%)	⬇️
src/client/common/process/proc.ts	14.49% <0.00%> (-0.73%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b04ed7...1dd6e49. Read the comment docs.

[DonJayamanne]

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))

[DonJayamanne]

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.

[rchiodo]

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.

[DonJayamanne]

• 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

[karthiknadig]

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.

[rchiodo]

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

[rchiodo]

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

[ericsnowcurrently]

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).

[ericsnowcurrently]

@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. 😄

[ericsnowcurrently]

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.

[ericsnowcurrently]

LGTM

I just have one comment about a slightly different helper approach that keeps the code-under-test in the test function.

Also, the flush/end thing Don suggested (if you agree it makes sense).

[ericsnowcurrently]

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) { ... }

[rchiodo]

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

[rchiodo]

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