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.

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

Improve Code Coverage #562

Merged
merged 12 commits into from
Feb 5, 2018
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
"posthtml-include": "^1.1.0",
"prettier": "^1.9.1",
"rimraf": "^2.6.1",
"sinon": "^4.2.2",
"sourcemap-validator": "^1.0.6",
"stylus": "^0.54.5",
"typescript": "^2.6.2"
Expand Down
6 changes: 5 additions & 1 deletion src/Logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class Logger {
this.lines += message.split('\n').length;
}

console.log(message);
this._log(message);
}

log(message) {
Expand Down Expand Up @@ -119,6 +119,10 @@ class Logger {
handleMessage(options) {
this[options.method](...options.args);
}

_log(message) {
console.log(message);
}
}

// If we are in a worker, make a proxy class which will
Expand Down
4 changes: 2 additions & 2 deletions src/assets/WebManifestAsset.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ class WebManifestAsset extends Asset {
icon.src = this.addURLDependency(icon.src);
}
}

if (Array.isArray(this.ast.screenshots)) {
for (let shot of this.ast.screenshots) {
shot.src = this.addURLDependency(shot.src);
}
}

if (this.ast.serviceworker && this.ast.serviceworker.src) {
this.ast.serviceworker.src = this.addURLDependency(
this.ast.serviceworker.src
Expand Down
14 changes: 8 additions & 6 deletions src/utils/customErrors.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
let serverErrorList = {
const serverErrorList = {
EACCES: "You don't have access to bind the server to port {port}.",
EADDRINUSE: 'There is already a process listening on port {port}.'
};

function serverErrors(err, port) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code was previously built to handle unknown errors if (!desc) ..., but an exception on the .replace call would prevent this code path from ever executing.

Exposed by tests, fixed.

let desc = serverErrorList[err.code].replace(/{port}/g, port);
if (!desc) {
desc = `Error: ${
err.code
} occurred while setting up server on port ${port}.`;
let desc = `Error: ${
err.code
} occurred while setting up server on port ${port}.`;

if (serverErrorList[err.code]) {
desc = serverErrorList[err.code].replace(/{port}/g, port);
}

return desc;
}

Expand Down
1 change: 1 addition & 0 deletions src/utils/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ exports.readFile = promisify(fs.readFile);
exports.writeFile = promisify(fs.writeFile);
exports.stat = promisify(fs.stat);
exports.readdir = promisify(fs.readdir);
exports.unlink = promisify(fs.unlink);

exports.exists = function(filename) {
return new Promise(resolve => {
Expand Down
37 changes: 30 additions & 7 deletions test/asset.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,24 @@
const {strictEqual} = require('assert');
const assert = require('assert');
const Asset = require('../src/Asset');

describe('Asset', () => {
it('should include default implementations', async () => {
const a = new Asset(__filename, undefined, {rootDir: '/root/dir'});
Object.assign(a, {
type: 'type',
contents: 'contents'
});

const err = new Error();

assert(a.shouldInvalidate() === false);
assert(a.mightHaveDependencies());
assert.deepEqual(await a.generate(), {
type: 'contents'
});
assert.equal(a.generateErrorMessage(err), err);
});

describe('addURLDependency', () => {
const bundleName = 'xyz';
const options = {
Expand All @@ -18,21 +35,27 @@ describe('Asset', () => {

it('should ignore urls', () => {
const url = 'https://parceljs.org/assets.html';
strictEqual(asset.addURLDependency(url), url);
assert.strictEqual(asset.addURLDependency(url), url);
});

it('should ignore empty string', () => {
strictEqual(asset.addURLDependency(''), '');
assert.strictEqual(asset.addURLDependency(''), '');
});

it('should generate bundle name', () => {
strictEqual(asset.addURLDependency('foo'), bundleName);
assert.strictEqual(asset.addURLDependency('foo'), bundleName);
});

it('should preserve query and hash', () => {
strictEqual(asset.addURLDependency('foo#bar'), `${bundleName}#bar`);
strictEqual(asset.addURLDependency('foo?bar'), `${bundleName}?bar`);
strictEqual(
assert.strictEqual(
asset.addURLDependency('foo#bar'),
`${bundleName}#bar`
);
assert.strictEqual(
asset.addURLDependency('foo?bar'),
`${bundleName}?bar`
);
assert.strictEqual(
asset.addURLDependency('foo?bar#baz'),
`${bundleName}?bar#baz`
);
Expand Down
40 changes: 40 additions & 0 deletions test/bundler.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const assert = require('assert');
const sinon = require('sinon');
const {bundler, nextBundle} = require('./utils');

describe('bundler', function() {
Expand All @@ -9,4 +10,43 @@ describe('bundler', function() {
await nextBundle(b);
assert(b.mainAsset);
});

it('should defer bundling if a bundle is pending', async () => {
const b = bundler(__dirname + '/integration/html/index.html');
b.pending = true; // bundle in progress
const spy = sinon.spy(b, 'bundle');

// first bundle, with existing bundle pending
const bundlePromise = b.bundle();

// simulate bundle finished
b.pending = false;
b.emit('buildEnd');

// wait for bundle to complete
await bundlePromise;

assert(spy.calledTwice);
});

it('should enforce asset type path to be a string', () => {
const b = bundler(__dirname + '/integration/html/index.html');

assert.throws(() => {
b.addAssetType('.ext', {});
}, 'should be a module path');
});

it('should enforce setup before bundling', () => {
const b = bundler(__dirname + '/integration/html/index.html');
b.farm = true; // truthy

assert.throws(() => {
b.addAssetType('.ext', __filename);
}, 'before bundling');

assert.throws(() => {
b.addPackager('type', 'packager');
}, 'before bundling');
});
});
29 changes: 29 additions & 0 deletions test/customErrors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
const assert = require('assert');
const customErrors = require('../src/utils/customErrors');

const port = 1234;

const EACCES = new Error();
EACCES.code = 'EACCES';
const EADDRINUSE = new Error();
EADDRINUSE.code = 'EADDRINUSE';

describe('customErrors', () => {
it('should include port in server errors', () => {
const msg = customErrors.serverErrors(EACCES, port);
assert(msg.includes(port));
});

it('should handle known server errors', () => {
let msg = customErrors.serverErrors(EACCES, port);
assert(msg.includes(`don't have access`));

msg = customErrors.serverErrors(EADDRINUSE, port);
assert(msg.includes('already'));
});

it('should handled unknown server errors', () => {
let msg = customErrors.serverErrors(new Error(), port);
assert(msg.includes(port));
});
});
142 changes: 142 additions & 0 deletions test/fs-cache.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
const assert = require('assert');
const path = require('path');
const rimraf = require('rimraf');
const fs = require('../src/utils/fs');
const promisify = require('../src/utils/promisify');
const {sleep} = require('./utils');
const ncp = promisify(require('ncp'));
const FSCache = require('../src/FSCache');

const cachePath = path.join(__dirname, '.cache');
const inputPath = path.join(__dirname, '/input');

const getMTime = async file => {
const stat = await fs.stat(file);
const mtime = stat.mtime.getTime();
return mtime;
};

describe('FSCache', () => {
beforeEach(() => {
rimraf.sync(cachePath);
rimraf.sync(inputPath);
});

it('should create directory on ensureDirExists', async () => {
let exists = await fs.exists(cachePath);
assert(!exists);

const cache = new FSCache({cacheDir: cachePath});
await cache.ensureDirExists();

exists = await fs.exists(cachePath);
assert(exists);
});

it('should cache resources', async () => {
const cache = new FSCache({cacheDir: cachePath});
await cache.write(__filename, {a: 'test', b: 1, dependencies: []});

let cached = await cache.read(__filename);
assert.equal(cached.a, 'test');
assert.equal(cached.b, 1);
});

it('should return null for invalidated resources', async () => {
const cache = new FSCache({cacheDir: cachePath});
cache.invalidate(__filename);

let cached = await cache.read(__filename);
assert.equal(cached, null);
});

it('should remove file on delete', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cache.delete was not working due to fs.unlink missing from the fs utility. The implementation catches the error and fails silently. Does not appear to be used today, but is now working correctly.

Exposed by tests, fixed.

let cache = new FSCache({cacheDir: cachePath});
await cache.write(__filename, {a: 'test', b: 1, dependencies: []});
await cache.delete(__filename);

let cached = await cache.read(__filename);
assert.equal(cached, null);
});

it('should remove from invalidated on write', async () => {
const cache = new FSCache({cacheDir: cachePath});
cache.invalidate(__filename);

assert(cache.invalidated.has(__filename));

await cache.write(__filename, {a: 'test', b: 1, dependencies: []});

assert(!cache.invalidated.has(__filename));
});

it('should include mtime for dependencies included in parent', async () => {
const cache = new FSCache({cacheDir: cachePath});
const mtime = await getMTime(__filename);

await cache.write(__filename, {
a: 'test',
b: 1,
dependencies: [
{
includedInParent: true,
name: __filename
},
{
name: __filename
}
]
});

const cached = await cache.read(__filename);
assert.equal(cached.dependencies[0].mtime, mtime);
assert.equal(cached.dependencies[1].mtime, undefined);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for #514.

it('should invalidate when dependency included in parent changes', async () => {
const cache = new FSCache({cacheDir: cachePath});
await ncp(__dirname + '/integration/fs', inputPath);
const filePath = path.join(inputPath, 'test.txt');

await cache.write(__filename, {
dependencies: [
{
includedInParent: true,
name: filePath
}
]
});

// delay and update dependency
await sleep(50);
await fs.writeFile(filePath, 'world');

const cached = await cache.read(__filename);
assert.equal(cached, null);
});

it('should return null on read error', async () => {
const cache = new FSCache({cacheDir: cachePath});
const cached = await cache.read(
path.join(__dirname, '/does/not/exist.txt')
);

assert.equal(cached, null);
});

it('should continue without throwing on write error', async () => {
const cache = new FSCache({cacheDir: cachePath});
const filePath = path.join(__dirname, '/does/not/exist.txt');

assert.doesNotThrow(async () => {
await cache.write(__filename, {
dependencies: [
{
includedInParent: true,
name: filePath
}
]
});
});
});
});
Loading