Skip to content
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

Jump to bottom

Async/await refactoring for tests/io folder - test.directory.js #1733

Merged
merged 5 commits into from Jan 17, 2018
Merged

Async/await refactoring for tests/io folder - test.directory.js #1733

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
752d813
refactors test.directory.js in tests/io folder
Jan 5, 2018
0d32e72
moves function readStringFromStream outside of the test function
Jan 9, 2018
294bcd3
changes try/catch block to rejects.toThrow
Jan 13, 2018
92059ba
adds readStringFromStream to helpers.js
Jan 13, 2018
ec796bc
adds resolves.toBe method
Jan 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
260 changes: 114 additions & 146 deletions tests/io/test.directory.js
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@ import { unexpectedSuccess } from '../helpers';


describe('Directory.getFiles()', () => {
it('should return cached data when available', () => {
it('should return cached data when available', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
const fakeFileMeta = {
size: 1,
Expand All @@ -18,41 +18,35 @@ describe('Directory.getFiles()', () => {

const fakeWalkPromise = sinon.stub();

return myDirectory.getFiles(fakeWalkPromise)
.then((files) => {
expect(files).toEqual(myDirectory.files);
expect(fakeWalkPromise.called).toBeFalsy();
});
const files = await myDirectory.getFiles(fakeWalkPromise);
expect(files).toEqual(myDirectory.files);
expect(fakeWalkPromise.called).toBeFalsy();
});

it('should return files from fixtures', () => {
it('should return files from fixtures', async () => {
const myDirectory = new Directory('tests/fixtures/io/');

return myDirectory.getFiles()
.then((files) => {
const fileNames = Object.keys(files);
expect(fileNames).toContain('dir1/file1.txt');
expect(fileNames).toContain('dir2/file2.txt');
expect(fileNames).toContain('dir2/dir3/file3.txt');
});
const files = await myDirectory.getFiles();
const fileNames = Object.keys(files);
expect(fileNames).toContain('dir1/file1.txt');
expect(fileNames).toContain('dir2/file2.txt');
expect(fileNames).toContain('dir2/dir3/file3.txt');
});

it('can be configured to not scan file paths', () => {
it('can be configured to not scan file paths', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
myDirectory.setScanFileCallback((filePath) => {
return !filePath.startsWith('dir2');
});

return myDirectory.getFiles()
.then((files) => {
const fileNames = Object.keys(files);
expect(fileNames).toContain('dir1/file1.txt');
expect(fileNames).not.toContain('dir2/file2.txt');
expect(fileNames).not.toContain('dir2/dir3/file3.txt');
});
const files = await myDirectory.getFiles();
const fileNames = Object.keys(files);
expect(fileNames).toContain('dir1/file1.txt');
expect(fileNames).not.toContain('dir2/file2.txt');
expect(fileNames).not.toContain('dir2/dir3/file3.txt');
});

it('can be configured to scan all dirs and to include a single file', () => {
it('can be configured to scan all dirs and to include a single file', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
myDirectory.setScanFileCallback((filePath, isDir) => {
if (isDir) {
Expand All @@ -61,85 +55,84 @@ describe('Directory.getFiles()', () => {
return filePath === 'dir2/dir3/file3.txt';
});

return myDirectory.getFiles()
.then((files) => {
const fileNames = Object.keys(files);
expect(fileNames).not.toContain('dir1/file1.txt');
expect(fileNames).not.toContain('dir2/file2.txt');
expect(fileNames).toContain('dir2/dir3/file3.txt');
});
const files = await myDirectory.getFiles();
const fileNames = Object.keys(files);
expect(fileNames).not.toContain('dir1/file1.txt');
expect(fileNames).not.toContain('dir2/file2.txt');
expect(fileNames).toContain('dir2/dir3/file3.txt');
});
});

describe('Directory._getPath()', () => {
it('should reject if not a file that exists', () => {
it('should reject if not a file that exists', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
return myDirectory.getFiles()
.then(() => {
return myDirectory.getPath('whatever')
.then(unexpectedSuccess)
.catch((err) => {
expect(err.message).toContain(
'"whatever" does not exist in this dir.'
);
});
});

await myDirectory.getFiles();
try {
await myDirectory.getPath('whatever');
unexpectedSuccess();
Copy link
Member

Choose a reason for hiding this comment

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

it looks that jest with a version >= 20 supports a expect(...).rejects assertion that we could use in out tests instead of the try { ... } catch (...) { ... }:

e.g. this one would become something like:

    await expect(
      myDirectory.getPath('whatever')
    ).rejects.toThrow('"whatever" does not exist in this dir.');

and as a plus jest produce a very nice failure message when the rejected promise doesn't match the expected error message (or if it doesn't fail as expected).

There are a number of try { ... } catch (...) { ... } in this file that we can make shorter and cleaner applying this approach, and it could be worth given that we are already changing this test file.

} catch (err) {
expect(err.message).toContain(
'"whatever" does not exist in this dir.'
);
}
});

it('should reject if path does not start with base', () => {
it('should reject if path does not start with base', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
myDirectory.files = {
'../file1.txt': {},
};
return myDirectory.getPath('../file1.txt')
.then(unexpectedSuccess)
.catch((err) => {
expect(err.message).toContain('Path argument must be relative');
});

try {
await myDirectory.getPath('../file1.txt');
unexpectedSuccess();
} catch (err) {
expect(err.message).toContain('Path argument must be relative');
}
});

it("should reject if path starts with '/'", () => {
it("should reject if path starts with '/'", async () => {
const myDirectory = new Directory('tests/fixtures/io/');
myDirectory.files = {
'/file1.txt': {},
};
return myDirectory.getPath('/file1.txt')
.then(unexpectedSuccess)
.catch((err) => {
expect(err.message).toContain('Path argument must be relative');
});

try {
await myDirectory.getPath('/file1.txt');
unexpectedSuccess();
} catch (err) {
expect(err.message).toContain('Path argument must be relative');
}
});
});

function readStringFromStream(readStream, transform) {
Copy link
Member

Choose a reason for hiding this comment

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

This helpers seems to be useful enough to be something exported by the helpers.js module (from the tests/ directory), I would not be surprised if it could be helpful in the xpi and crx test files as well.

return new Promise((resolve, reject) => {
let content = '';
readStream.on('readable', () => {
let chunk;
// eslint-disable-next-line no-cond-assign
while ((chunk = readStream.read()) !== null) {
content += chunk.toString(transform);
}
});
readStream.on('end', () => {
resolve(content);
});
readStream.on('error', reject);
});
}

describe('Directory.getFileAsStream()', () => {
it('should return a stream', () => {
it('should return a stream', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
return myDirectory.getFiles()
.then(() => {
return myDirectory.getFileAsStream('dir2/dir3/file3.txt');
})
.then((readStream) => {
return new Promise((resolve, reject) => {
let content = '';
readStream
.on('readable', () => {
let chunk;
// eslint-disable-next-line no-cond-assign
while ((chunk = readStream.read()) !== null) {
content += chunk.toString();
}
})
.on('end', () => {
resolve(content);
})
.on('error', (err) => {
reject(err);
});
})
.then((content) => {
expect(content).toEqual('123\n');
});
});
await myDirectory.getFiles();

const readStream = await myDirectory.getFileAsStream('dir2/dir3/file3.txt');

const content = await readStringFromStream(readStream);
expect(content).toEqual('123\n');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpl thanks for the very helpful Jest link. Now I have a couple of suggestions about using Jest methods. For example, maybe here we can replace lines 106, 107 with
await expect(readStringFromStream(readStream).resolves.toBe('123\n');

Copy link
Member

Choose a reason for hiding this comment

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

@hameleonka 👍 sure! go for it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpl I've changed some Jest methods in this file

});

it('should not enforce utf-8 when encoding = null', async () => {
Expand All @@ -153,27 +146,8 @@ describe('Directory.getFileAsStream()', () => {
encoding: null,
});

const readStringFromStream = (readStream) => {
return new Promise((resolve, reject) => {
let content = '';
readStream.on('readable', () => {
let chunk;
// eslint-disable-next-line no-cond-assign
while ((chunk = readStream.read()) !== null) {
content += chunk.toString('binary');
}
});

readStream.on('end', () => {
resolve(content);
});

readStream.on('error', reject);
});
};

const stringFromEncodingDefault = await readStringFromStream(readStreamEncodingDefault);
const stringFromEncodingNull = await readStringFromStream(readStreamEncodingNull);
const stringFromEncodingDefault = await readStringFromStream(readStreamEncodingDefault, 'binary');
const stringFromEncodingNull = await readStringFromStream(readStreamEncodingNull, 'binary');

// Ensure that by setting the encoding to null, the utf-8 encoding is not enforced
// while reading binary data from the stream.
Expand All @@ -184,7 +158,7 @@ describe('Directory.getFileAsStream()', () => {
expect(stringFromEncodingDefault.slice(0, 8)).not.toEqual('\x89PNG\r\n\x1a\n');
});

it('should reject if file is too big', () => {
it('should reject if file is too big', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
const fakeFileMeta = {
size: 1024 * 1024 * 102,
Expand All @@ -194,39 +168,34 @@ describe('Directory.getFileAsStream()', () => {
'chrome.manifest': fakeFileMeta,
};

return myDirectory.getFileAsStream('manifest.json')
.then(unexpectedSuccess)
.catch((err) => {
expect(err.message).toContain('File "manifest.json" is too large');
});
try {
await myDirectory.getFileAsStream('manifest.json');
unexpectedSuccess();
} catch (err) {
expect(err.message).toContain('File "manifest.json" is too large');
}
});
});


describe('Directory.getFileAsString()', () => {
it('should strip a BOM', () => {
it('should strip a BOM', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
return myDirectory.getFiles()
.then(() => {
return myDirectory.getFileAsString('dir3/foo.txt');
})
.then((content) => {
expect(content.charCodeAt(0)).not.toEqual(0xFEFF);
});

await myDirectory.getFiles();
const content = await myDirectory.getFileAsString('dir3/foo.txt');
expect(content.charCodeAt(0)).not.toEqual(0xFEFF);
});

it('should return a string', () => {
it('should return a string', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
return myDirectory.getFiles()
.then(() => {
return myDirectory.getFileAsString('dir2/dir3/file3.txt');
})
.then((string) => {
expect(string).toEqual('123\n');
});

await myDirectory.getFiles();
const string = await myDirectory.getFileAsString('dir2/dir3/file3.txt');
expect(string).toEqual('123\n');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpl same here
await expect(myDirectory.getFileAsString('dir2/dir3/file3.txt').resolves.toBe('123\n');

Copy link
Member

Choose a reason for hiding this comment

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

@hameleonka 👍

});

it('should reject if stream emits error', () => {
it('should reject if stream emits error', async () => {
const fakeStreamEmitter = new EventEmitter();

const myDirectory = new Directory('tests/fixtures/io/');
Expand All @@ -242,14 +211,15 @@ describe('Directory.getFileAsString()', () => {
return Promise.resolve(fakeStreamEmitter);
};

return myDirectory.getFileAsString('manifest.json')
.then(unexpectedSuccess)
.catch((err) => {
expect(err.message).toContain('¡hola!');
});
try {
await myDirectory.getFileAsString('manifest.json');
unexpectedSuccess();
} catch (err) {
expect(err.message).toContain('¡hola!');
}
});

it('should reject if file is too big', () => {
it('should reject if file is too big', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
const fakeFileMeta = {
size: 1024 * 1024 * 102,
Expand All @@ -259,11 +229,12 @@ describe('Directory.getFileAsString()', () => {
'chrome.manifest': fakeFileMeta,
};

return myDirectory.getFileAsString('manifest.json')
.then(unexpectedSuccess)
.catch((err) => {
expect(err.message).toContain('File "manifest.json" is too large');
});
try {
await myDirectory.getFileAsString('manifest.json');
unexpectedSuccess();
} catch (err) {
expect(err.message).toContain('File "manifest.json" is too large');
}
});
});

Expand All @@ -275,17 +246,14 @@ describe('Directory.getFileAsString()', () => {
The location is not relevant, the file contents are.
*/
describe('Directory.getChunkAsBuffer()', () => {
it('should get a buffer', () => {
it('should get a buffer', async () => {
const myDirectory = new Directory('tests/fixtures/io/');
return myDirectory.getFiles()
.then(() => {
// Just grab the first two characters.
return myDirectory.getChunkAsBuffer('dir2/dir3/file3.txt', 2);
})
.then((buffer) => {
// The file contains: 123\n. This tests that we are getting just
// the first two characters in the buffer.
expect(buffer.toString()).toEqual('12');
});

await myDirectory.getFiles();
// Just grab the first two characters.
const buffer = await myDirectory.getChunkAsBuffer('dir2/dir3/file3.txt', 2);
// The file contains: 123\n. This tests that we are getting just
// the first two characters in the buffer.
expect(buffer.toString()).toEqual('12');
});
});