From 7462d377ab9ed24430af1861f9ad459b2b4445fe Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 31 Dec 2019 14:20:48 -0700 Subject: [PATCH 1/5] Extract the temp file operations into a separate class. --- src/client/common/platform/fileSystem.ts | 13 +-- src/client/common/platform/fs-temp.ts | 40 +++++++++ .../platform/filesystem.functional.test.ts | 62 +------------- .../platform/fs-temp.functional.test.ts | 85 +++++++++++++++++++ 4 files changed, 130 insertions(+), 70 deletions(-) create mode 100644 src/client/common/platform/fs-temp.ts create mode 100644 src/test/common/platform/fs-temp.functional.test.ts diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 462c6f737fa1..04c9e8b212cc 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -7,11 +7,11 @@ import * as fileSystem from 'fs'; import * as fs from 'fs-extra'; import * as glob from 'glob'; import { inject, injectable } from 'inversify'; -import * as tmp from 'tmp'; import { promisify } from 'util'; import { createDeferred } from '../utils/async'; import { noop } from '../utils/misc'; import { FileSystemPaths, FileSystemPathUtils } from './fs-paths'; +import { TemporaryFileSystem } from './fs-temp'; // prettier-ignore import { FileStat, FileType, @@ -85,6 +85,7 @@ export function convertStat(old: fs.Stats, filetype: FileType): FileStat { export class FileSystem implements IFileSystem { private readonly paths: IFileSystemPaths; private readonly pathUtils: FileSystemPathUtils; + private readonly tmp: TemporaryFileSystem; // prettier-ignore constructor( @inject(IPlatformService) platformService: IPlatformService @@ -94,6 +95,7 @@ export class FileSystem implements IFileSystem { platformService.isWindows ); this.pathUtils = FileSystemPathUtils.withDefaults(this.paths); + this.tmp = TemporaryFileSystem.withDefaults(); } //================================= @@ -307,14 +309,7 @@ export class FileSystem implements IFileSystem { } public createTemporaryFile(extension: string): Promise { - return new Promise((resolve, reject) => { - tmp.file({ postfix: extension }, (err, tmpFile, _, cleanupCallback) => { - if (err) { - return reject(err); - } - resolve({ filePath: tmpFile, dispose: cleanupCallback }); - }); - }); + return this.tmp.createFile(extension); } public async isDirReadonly(dirname: string): Promise { diff --git a/src/client/common/platform/fs-temp.ts b/src/client/common/platform/fs-temp.ts new file mode 100644 index 000000000000..332bdf8cc8fe --- /dev/null +++ b/src/client/common/platform/fs-temp.ts @@ -0,0 +1,40 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as tmp from 'tmp'; +import { TemporaryFile } from './types'; + +interface IRawTempFS { + // tslint:disable-next-line:no-any + file(config: tmp.Options, callback?: (err: any, path: string, fd: number, cleanupCallback: () => void) => void): void; +} + +export class TemporaryFileSystem { + // prettier-ignore + constructor( + private readonly raw: IRawTempFS + ) { } + public static withDefaults(): TemporaryFileSystem { + // prettier-ignore + return new TemporaryFileSystem( + tmp + ); + } + + public createFile(suffix: string): Promise { + const opts = { + postfix: suffix + }; + return new Promise((resolve, reject) => { + this.raw.file(opts, (err, filename, _fd, cleanUp) => { + if (err) { + return reject(err); + } + resolve({ + filePath: filename, + dispose: cleanUp + }); + }); + }); + } +} diff --git a/src/test/common/platform/filesystem.functional.test.ts b/src/test/common/platform/filesystem.functional.test.ts index bb8a07a141c7..5cb89b4731aa 100644 --- a/src/test/common/platform/filesystem.functional.test.ts +++ b/src/test/common/platform/filesystem.functional.test.ts @@ -9,7 +9,7 @@ import * as path from 'path'; import { convertStat, FileSystem } from '../../../client/common/platform/fileSystem'; import { FileSystemPaths, FileSystemPathUtils } from '../../../client/common/platform/fs-paths'; import { PlatformService } from '../../../client/common/platform/platformService'; -import { FileType, TemporaryFile } from '../../../client/common/platform/types'; +import { FileType } from '../../../client/common/platform/types'; import { sleep } from '../../../client/common/utils/async'; // prettier-ignore import { @@ -1127,66 +1127,6 @@ suite('FileSystem', () => { }); }); - suite('createTemporaryFile', () => { - async function createTemporaryFile(suffix: string): Promise { - const tempfile = await fileSystem.createTemporaryFile(suffix); - fix.addFSCleanup(tempfile.filePath, tempfile.dispose); - return tempfile; - } - - test('TemporaryFile is created properly', async () => { - const tempfile = await fileSystem.createTemporaryFile('.tmp'); - fix.addFSCleanup(tempfile.filePath, tempfile.dispose); - await assertExists(tempfile.filePath); - - expect(tempfile.filePath.endsWith('.tmp')).to.equal(true, `bad suffix on ${tempfile.filePath}`); - }); - - test('TemporaryFile is disposed properly', async () => { - const tempfile = await createTemporaryFile('.tmp'); - await assertExists(tempfile.filePath); - - tempfile.dispose(); - - await assertDoesNotExist(tempfile.filePath); - }); - - test('Ensure creating a temporary file results in a unique temp file path', async () => { - const tempFile = await createTemporaryFile('.tmp'); - const tempFile2 = await createTemporaryFile('.tmp'); - - const filename1 = tempFile.filePath; - const filename2 = tempFile2.filePath; - - expect(filename1).to.not.equal(filename2); - }); - - test('Ensure writing to a temp file is supported via file stream', async function() { - if (WINDOWS) { - // tslint:disable-next-line:no-invalid-this - this.skip(); - } - const tempfile = await createTemporaryFile('.tmp'); - const stream = fileSystem.createWriteStream(tempfile.filePath); - fix.addCleanup(() => stream.destroy()); - const data = '...'; - - stream.write(data, 'utf8'); - - const actual = await fs.readFile(tempfile.filePath, 'utf8'); - expect(actual).to.equal(data); - }); - - test('Ensure chmod works against a temporary file', async () => { - // Note that on Windows chmod is a noop. - const tempfile = await createTemporaryFile('.tmp'); - - const promise = fs.chmod(tempfile.filePath, '7777'); - - await expect(promise).to.not.eventually.be.rejected; - }); - }); - suite('isDirReadonly', () => { suite('non-Windows', () => { suiteSetup(function() { diff --git a/src/test/common/platform/fs-temp.functional.test.ts b/src/test/common/platform/fs-temp.functional.test.ts new file mode 100644 index 000000000000..c29a5461d2f4 --- /dev/null +++ b/src/test/common/platform/fs-temp.functional.test.ts @@ -0,0 +1,85 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +// tslint:disable:max-func-body-length chai-vague-errors + +import { expect } from 'chai'; +import * as fs from 'fs-extra'; +import { TemporaryFileSystem } from '../../../client/common/platform/fs-temp'; +import { TemporaryFile } from '../../../client/common/platform/types'; +// prettier-ignore +import { + assertDoesNotExist, assertExists, FSFixture, WINDOWS +} from './utils'; + +suite('FileSystem - TemporaryFileSystem', () => { + let tmpfs: TemporaryFileSystem; + let fix: FSFixture; + setup(async () => { + tmpfs = TemporaryFileSystem.withDefaults(); + fix = new FSFixture(); + }); + teardown(async () => { + await fix.cleanUp(); + }); + + suite('createFile', () => { + async function createFile(suffix: string): Promise { + const tempfile = await tmpfs.createFile(suffix); + fix.addFSCleanup(tempfile.filePath, tempfile.dispose); + return tempfile; + } + + test('TemporaryFile is created properly', async () => { + const tempfile = await tmpfs.createFile('.tmp'); + fix.addFSCleanup(tempfile.filePath, tempfile.dispose); + await assertExists(tempfile.filePath); + + expect(tempfile.filePath.endsWith('.tmp')).to.equal(true, `bad suffix on ${tempfile.filePath}`); + }); + + test('TemporaryFile is disposed properly', async () => { + const tempfile = await createFile('.tmp'); + await assertExists(tempfile.filePath); + + tempfile.dispose(); + + await assertDoesNotExist(tempfile.filePath); + }); + + test('Ensure creating a temporary file results in a unique temp file path', async () => { + const tempFile = await createFile('.tmp'); + const tempFile2 = await createFile('.tmp'); + + const filename1 = tempFile.filePath; + const filename2 = tempFile2.filePath; + + expect(filename1).to.not.equal(filename2); + }); + + test('Ensure writing to a temp file is supported via file stream', async function() { + if (WINDOWS) { + // tslint:disable-next-line:no-invalid-this + this.skip(); + } + const tempfile = await createFile('.tmp'); + const stream = fs.createWriteStream(tempfile.filePath); + fix.addCleanup(() => stream.destroy()); + const data = '...'; + + stream.write(data, 'utf8'); + + const actual = await fs.readFile(tempfile.filePath, 'utf8'); + expect(actual).to.equal(data); + }); + + test('Ensure chmod works against a temporary file', async () => { + // Note that on Windows chmod is a noop. + const tempfile = await createFile('.tmp'); + + const promise = fs.chmod(tempfile.filePath, '7777'); + + await expect(promise).to.not.eventually.be.rejected; + }); + }); +}); From 5aa1c54972f0450850be6e5d90cc0124d8067384 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 31 Dec 2019 15:23:14 -0700 Subject: [PATCH 2/5] Add missing doc comments. --- src/client/common/platform/fs-temp.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/client/common/platform/fs-temp.ts b/src/client/common/platform/fs-temp.ts index 332bdf8cc8fe..c1b8e9081f64 100644 --- a/src/client/common/platform/fs-temp.ts +++ b/src/client/common/platform/fs-temp.ts @@ -9,6 +9,7 @@ interface IRawTempFS { file(config: tmp.Options, callback?: (err: any, path: string, fd: number, cleanupCallback: () => void) => void): void; } +// Operations related to temporary files and directories. export class TemporaryFileSystem { // prettier-ignore constructor( @@ -21,6 +22,7 @@ export class TemporaryFileSystem { ); } + // Create a new temp file with the given filename suffix. public createFile(suffix: string): Promise { const opts = { postfix: suffix From 2256097b94582ee48f538ddf755acc5cb7bb8947 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 31 Dec 2019 15:23:53 -0700 Subject: [PATCH 3/5] Ensure full coverage on FileSystem. --- .../common/platform/filesystem.functional.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/common/platform/filesystem.functional.test.ts b/src/test/common/platform/filesystem.functional.test.ts index 5cb89b4731aa..d964796562a6 100644 --- a/src/test/common/platform/filesystem.functional.test.ts +++ b/src/test/common/platform/filesystem.functional.test.ts @@ -1127,6 +1127,18 @@ suite('FileSystem', () => { }); }); + suite('createFile', () => { + // tested fully in the TemporaryFileSystem tests. + + test('calls wrapped object', async () => { + const tempfile = await fileSystem.createTemporaryFile('.tmp'); + fix.addFSCleanup(tempfile.filePath, tempfile.dispose); + await assertExists(tempfile.filePath); + + expect(tempfile.filePath.endsWith('.tmp')).to.equal(true); + }); + }); + suite('isDirReadonly', () => { suite('non-Windows', () => { suiteSetup(function() { From 59e665b8f8d3b63c0a4d290c6bbf29d4c931b372 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 9 Jan 2020 13:43:23 -0700 Subject: [PATCH 4/5] Add ITempFileSystem. --- src/client/common/platform/fs-temp.ts | 4 ++-- src/client/common/platform/types.ts | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/client/common/platform/fs-temp.ts b/src/client/common/platform/fs-temp.ts index c1b8e9081f64..ed9b48e8214c 100644 --- a/src/client/common/platform/fs-temp.ts +++ b/src/client/common/platform/fs-temp.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. import * as tmp from 'tmp'; -import { TemporaryFile } from './types'; +import { ITempFileSystem, TemporaryFile } from './types'; interface IRawTempFS { // tslint:disable-next-line:no-any @@ -10,7 +10,7 @@ interface IRawTempFS { } // Operations related to temporary files and directories. -export class TemporaryFileSystem { +export class TemporaryFileSystem implements ITempFileSystem { // prettier-ignore constructor( private readonly raw: IRawTempFS diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index c0541fb8298c..bd3cbda012cc 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -47,6 +47,10 @@ export interface IPlatformService { export type TemporaryFile = { filePath: string } & vscode.Disposable; export type TemporaryDirectory = { path: string } & vscode.Disposable; +export interface ITempFileSystem { + createFile(suffix: string): Promise; +} + //=========================== // FS paths From 6d1bfeb14e8162496a3027c48f46037ee595ed11 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 14 Jan 2020 17:01:06 -0700 Subject: [PATCH 5/5] Add tests for the error cases. --- src/test/common/platform/fs-temp.unit.test.ts | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 src/test/common/platform/fs-temp.unit.test.ts diff --git a/src/test/common/platform/fs-temp.unit.test.ts b/src/test/common/platform/fs-temp.unit.test.ts new file mode 100644 index 000000000000..e687bae90e0d --- /dev/null +++ b/src/test/common/platform/fs-temp.unit.test.ts @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +// tslint:disable:max-func-body-length + +import { expect } from 'chai'; +import * as TypeMoq from 'typemoq'; +import { TemporaryFileSystem } from '../../../client/common/platform/fs-temp'; + +interface IDeps { + // tmp module + // tslint:disable-next-line:no-any + file(config: { postfix?: string }, callback?: (err: any, path: string, fd: number, cleanupCallback: () => void) => void): void; +} + +suite('FileSystem - temp files', () => { + let deps: TypeMoq.IMock; + let temp: TemporaryFileSystem; + setup(() => { + deps = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); + temp = new TemporaryFileSystem(deps.object); + }); + function verifyAll() { + deps.verifyAll(); + } + + suite('createFile', () => { + test(`fails if the raw call fails`, async () => { + const failure = new Error('oops'); + // prettier-ignore + deps.setup(d => d.file({ postfix: '.tmp' }, TypeMoq.It.isAny())) + .throws(failure); + + const promise = temp.createFile('.tmp'); + + await expect(promise).to.eventually.be.rejected; + verifyAll(); + }); + + test(`fails if the raw call "returns" an error`, async () => { + const failure = new Error('oops'); + deps.setup(d => d.file({ postfix: '.tmp' }, TypeMoq.It.isAny())) + // tslint:disable-next-line:no-empty + .callback((_cfg, cb) => cb(failure, '...', -1, () => {})); + + const promise = temp.createFile('.tmp'); + + await expect(promise).to.eventually.be.rejected; + verifyAll(); + }); + }); +});