Skip to content

Commit 47f4e55

Browse files
ericsnowcurrentlyericsnowcurrently
committed
Work around upstream bugs in the new "fs" API.
1 parent e324a75 commit 47f4e55

File tree

3 files changed

+84
-34
lines changed

3 files changed

+84
-34
lines changed

‎src/client/common/platform/fileSystem.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
} from './types';
2222

2323
// tslint:disable:max-classes-per-file
24+
// tslint:disable:no-suspicious-comment
2425

2526
const ENCODING: string = 'utf8';
2627

@@ -203,6 +204,11 @@ export class RawFileSystem implements IRawFileSystem {
203204

204205
public async rmtree(dirname: string): Promise<void> {
205206
const uri = vscode.Uri.file(dirname);
207+
// TODO (https://github.com/microsoft/vscode/issues/84177)
208+
// The docs say "throws - FileNotFound when uri doesn't exist".
209+
// However, it happily does nothing (at least for remote-over-SSH).
210+
// So we have to manually stat, just to be sure.
211+
await this.newapi.stat(uri);
206212
return this.newapi.delete(uri, {
207213
recursive: true,
208214
useTrash: false
@@ -228,6 +234,9 @@ export class RawFileSystem implements IRawFileSystem {
228234
}
229235

230236
public async mkdirp(dirname: string): Promise<void> {
237+
// TODO https://github.com/microsoft/vscode/issues/84175
238+
// Hopefully VS Code provides this method in their API
239+
// so we don't have to roll our own.
231240
const stack = [dirname];
232241
while (stack.length > 0) {
233242
const current = stack.pop() || '';
@@ -261,6 +270,12 @@ export class RawFileSystem implements IRawFileSystem {
261270
public async copyFile(src: string, dest: string): Promise<void> {
262271
const srcURI = vscode.Uri.file(src);
263272
const destURI = vscode.Uri.file(dest);
273+
// TODO (https://github.com/microsoft/vscode/issues/84177)
274+
// The docs say "throws - FileNotFound when parent of
275+
// destination doesn't exist". However, it happily creates
276+
// the parent directory (at least for remote-over-SSH).
277+
// So we have to manually stat, just to be sure.
278+
await this.newapi.stat(vscode.Uri.file(this.path.dirname(dest)));
264279
await this.newapi.copy(srcURI, destURI, {
265280
overwrite: true
266281
});
@@ -270,11 +285,14 @@ export class RawFileSystem implements IRawFileSystem {
270285
// fs-extra
271286

272287
public async chmod(filename: string, mode: string | number): Promise<void> {
288+
// TODO https://github.com/microsoft/vscode/issues/84175
289+
// This functionality has been requested for the VS Code API.
273290
return this.fsExtra.chmod(filename, mode);
274291
}
275292

276293
public async lstat(filename: string): Promise<FileStat> {
277-
// At the moment the new VS Code API does not seem to support lstat...
294+
// TODO https://github.com/microsoft/vscode/issues/84175
295+
// This functionality has been requested for the VS Code API.
278296
const stat = await this.fsExtra.lstat(filename);
279297
return convertFileStat(stat);
280298
}
@@ -283,18 +301,24 @@ export class RawFileSystem implements IRawFileSystem {
283301
// non-async (fs-extra)
284302

285303
public statSync(filename: string): FileStat {
304+
// TODO https://github.com/microsoft/vscode/issues/84175
305+
// This functionality has been requested for the VS Code API.
286306
const stat = this.fsExtra.statSync(filename);
287307
return convertFileStat(stat);
288308
}
289309

290310
public readTextSync(filename: string): string {
311+
// TODO https://github.com/microsoft/vscode/issues/84175
312+
// This functionality has been requested for the VS Code API.
291313
return this.fsExtra.readFileSync(filename, ENCODING);
292314
}
293315

294316
//****************************
295317
// non-async (fs)
296318

297319
public createWriteStream(filename: string): WriteStream {
320+
// TODO https://github.com/microsoft/vscode/issues/84175
321+
// This functionality has been requested for the VS Code API.
298322
return this.nodefs.createWriteStream(filename);
299323
}
300324
}

‎src/test/common/platform/filesystem.test.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
// are found in filesystem.functional.test.ts.
2020

2121
// tslint:disable:max-func-body-length chai-vague-errors
22+
// tslint:disable:no-suspicious-comment
2223

2324
suite('Raw FileSystem', () => {
2425
let filesystem: IRawFileSystem;
@@ -148,9 +149,11 @@ suite('Raw FileSystem', () => {
148149
return {
149150
type: filetype,
150151
size: old.size,
151-
// XXX Apparently VS Code's new "fs" has granularity of seconds.
152-
// So we round to the nearest integer.
153-
// XXX ctime is showing up as 0.
152+
// TODO (https://github.com/microsoft/vscode/issues/84177)
153+
// FileStat.ctime and FileStat.mtime only have 1-second resolution.
154+
// So for now we round to the nearest integer.
155+
// TODO (https://github.com/microsoft/vscode/issues/84177)
156+
// FileStat.ctime is consistently 0 instead of the actual ctime.
154157
ctime: 0,
155158
//ctime: Math.round(old.ctimeMs),
156159
mtime: Math.round(old.mtimeMs)

‎src/test/common/platform/filesystem.unit.test.ts

Lines changed: 53 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,28 @@ import {
1717

1818
// tslint:disable:max-func-body-length chai-vague-errors
1919

20+
function createMockStat(): TypeMoq.IMock<FileStat> {
21+
const stat = TypeMoq.Mock.ofType<FileStat>(undefined, TypeMoq.MockBehavior.Strict);
22+
// This is necessary because passing "mock.object" to
23+
// Promise.resolve() triggers the lookup.
24+
//tslint:disable-next-line:no-any
25+
stat.setup((s: any) => s.then)
26+
.returns(() => undefined)
27+
.verifiable(TypeMoq.Times.atLeast(0));
28+
return stat;
29+
}
30+
31+
function createMockLegacyStat(): TypeMoq.IMock<fsextra.Stats> {
32+
const stat = TypeMoq.Mock.ofType<fsextra.Stats>(undefined, TypeMoq.MockBehavior.Strict);
33+
// This is necessary because passing "mock.object" to
34+
// Promise.resolve() triggers the lookup.
35+
//tslint:disable-next-line:no-any
36+
stat.setup((s: any) => s.then)
37+
.returns(() => undefined)
38+
.verifiable(TypeMoq.Times.atLeast(0));
39+
return stat;
40+
}
41+
2042
//tslint:disable-next-line:no-any
2143
type TempCallback = (err: any, path: string, fd: number, cleanupCallback: () => void) => void;
2244
interface IRawFS {
@@ -251,48 +273,30 @@ suite('FileSystem paths', () => {
251273

252274
suite('Raw FileSystem', () => {
253275
let raw: TypeMoq.IMock<IRawFS>;
254-
//let stat: TypeMoq.IMock<FileStat>;
255276
let oldStat: TypeMoq.IMock<fsextra.Stats>;
256277
let filesystem: IRawFileSystem;
257278
setup(() => {
258279
raw = TypeMoq.Mock.ofType<IRawFS>(undefined, TypeMoq.MockBehavior.Strict);
259-
//stat = TypeMoq.Mock.ofType<FileStat>(undefined, TypeMoq.MockBehavior.Strict);
260-
oldStat = TypeMoq.Mock.ofType<fsextra.Stats>(undefined, TypeMoq.MockBehavior.Strict);
280+
oldStat = createMockLegacyStat();
261281
filesystem = new RawFileSystem(
262-
//raw.object,
263282
raw.object,
264283
raw.object,
265284
raw.object,
266285
raw.object
267286
);
268-
269-
// This is necessary because passing "stat.object" to
270-
// Promise.resolve() triggers the lookup.
271-
//tslint:disable-next-line:no-any
272-
oldStat.setup((s: any) => s.then)
273-
.returns(() => undefined)
274-
.verifiable(TypeMoq.Times.atLeast(0));
275287
});
276288
function verifyAll() {
277289
raw.verifyAll();
278-
//stat.verifyAll();
279290
oldStat.verifyAll();
280291
}
281292

282293
function setupOldStat(stat: FileStat, old?: TypeMoq.IMock<fsextra.Stats> | null) {
283294
if (old === undefined) {
284295
old = oldStat;
285296
} else if (old === null) {
286-
old = TypeMoq.Mock.ofType<fsextra.Stats>(undefined, TypeMoq.MockBehavior.Strict);
297+
old = createMockLegacyStat();
287298
}
288299

289-
// This is necessary because passing "stat.object" to
290-
// Promise.resolve() triggers the lookup.
291-
//tslint:disable-next-line:no-any
292-
old!.setup((s: any) => s.then)
293-
.returns(() => undefined)
294-
.verifiable(TypeMoq.Times.atLeast(0));
295-
296300
if (stat.type === FileType.File) {
297301
old!.setup(s => s.isFile())
298302
.returns(() => true);
@@ -419,22 +423,28 @@ suite('Raw FileSystem', () => {
419423
suite('rmtree', () => {
420424
test('wraps the low-level function', async () => {
421425
const dirname = 'x/y/z/spam';
422-
raw.setup(r => r.delete(Uri.file(dirname), { recursive: true, useTrash: false }))
426+
const uri = Uri.file(dirname);
427+
const stat = createMockStat();
428+
raw.setup(r => r.stat(uri))
429+
.returns(() => Promise.resolve(stat.object));
430+
raw.setup(r => r.delete(uri, { recursive: true, useTrash: false }))
423431
.returns(() => Promise.resolve());
424432

425433
await filesystem.rmtree(dirname);
426434

427435
verifyAll();
436+
stat.verifyAll();
428437
});
429438

430439
test('fails if the directory does not exist', async () => {
431440
const dirname = 'x/y/z/spam';
432-
raw.setup(r => r.delete(Uri.file(dirname), { recursive: true, useTrash: false }))
441+
raw.setup(r => r.stat(Uri.file(dirname)))
433442
.throws(new Error('file not found'));
434443

435444
const promise = filesystem.rmtree(dirname);
436445

437446
await expect(promise).to.eventually.be.rejected;
447+
verifyAll();
438448
});
439449
});
440450

@@ -545,12 +555,32 @@ suite('Raw FileSystem', () => {
545555
test('read/write streams are used properly', async () => {
546556
const src = 'x/y/z/spam.py';
547557
const dest = 'x/y/z/spam.py.bak';
558+
raw.setup(r => r.dirname(dest))
559+
.returns(() => 'x/y/z');
560+
const stat = createMockStat();
561+
raw.setup(r => r.stat(Uri.file('x/y/z')))
562+
.returns(() => Promise.resolve(stat.object));
548563
raw.setup(r => r.copy(Uri.file(src), Uri.file(dest), { overwrite: true }))
549564
.returns(() => Promise.resolve());
550565

551566
await filesystem.copyFile(src, dest);
552567

553568
verifyAll();
569+
stat.verifyAll();
570+
});
571+
572+
test('fails if the parent directory does not exist', async () => {
573+
const src = '/tmp/spam.py';
574+
const dest = '/tmp/__does_not_exist__/spam.py';
575+
raw.setup(r => r.dirname(dest))
576+
.returns(() => '/tmp/__does_not_exist__');
577+
raw.setup(r => r.stat(Uri.file('/tmp/__does_not_exist__')))
578+
.throws(new Error('file not found'));
579+
580+
const promise = filesystem.copyFile(src, dest);
581+
582+
await expect(promise).to.eventually.be.rejected;
583+
verifyAll();
554584
});
555585
});
556586

@@ -630,7 +660,7 @@ suite('FileSystem Utils', () => {
630660
let deps: TypeMoq.IMock<IDeps>;
631661
let utils: IFileSystemUtils;
632662
setup(() => {
633-
stat = TypeMoq.Mock.ofType<FileStat>(undefined, TypeMoq.MockBehavior.Strict);
663+
stat = createMockStat();
634664
filesystem = TypeMoq.Mock.ofType<IRawFileSystem>(undefined, TypeMoq.MockBehavior.Strict);
635665
path = TypeMoq.Mock.ofType<IFileSystemPaths>(undefined, TypeMoq.MockBehavior.Strict);
636666
tmp = TypeMoq.Mock.ofType<ITempFileSystem>(undefined, TypeMoq.MockBehavior.Strict);
@@ -642,13 +672,6 @@ suite('FileSystem Utils', () => {
642672
((data: string) => deps.object.getHashString(data)),
643673
((p: string) => deps.object.glob(p))
644674
);
645-
646-
// This is necessary because passing "stat.object" to
647-
// Promise.resolve() triggers the lookup.
648-
//tslint:disable-next-line:no-any
649-
stat.setup((s: any) => s.then)
650-
.returns(() => undefined)
651-
.verifiable(TypeMoq.Times.atLeast(0));
652675
});
653676
function verifyAll() {
654677
filesystem.verifyAll();

0 commit comments

Comments
 (0)