From 7c96eb08192f631bb10ca9ca803f4f5de8ef0df7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=CC=81=20Cruz?= Date: Fri, 17 May 2013 11:47:00 +0100 Subject: [PATCH] Small performance improvement by accepting an optional package meta to avoid re-reading it. Fix tests if previous run left dummy directories. --- README.md | 8 +-- lib/core/resolvers/GitResolver.js | 2 +- lib/core/resolvers/Resolver.js | 18 +++--- lib/core/resolvers/UrlResolver.js | 2 +- test/core/resolverFactory.js | 3 +- test/core/resolvers/fsResolver.js | 13 +++- test/core/resolvers/gitResolver.js | 66 +++++++++++++++++++-- test/core/resolvers/resolver.js | 95 ++++++++++++++++++++++++++---- test/core/resolvers/urlResolver.js | 3 +- 9 files changed, 170 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index f90dd2d3..6e657bd4 100644 --- a/README.md +++ b/README.md @@ -274,14 +274,14 @@ Returns the target. Returns the local temporary folder into which the package is being fetched. The files will remain here until the folder is moved when installing. -`Resolver#hasNew(canonicalPkg)`: Promise +`Resolver#hasNew(canonicalPkg, pkgMeta)`: Promise Checks if there is a version more recent than the provided `canonicalPkg` (folder) that complies with the resolver target. The hasNew process is as follows: -- Reads the `package meta` from the `canonicalPkg` -- Resolves to `true if it doesn't exist -- Otherwise, calls _hasNew with the `package meta` and the `canonicalPkg` as arguments +- Reads the `package meta` from the `canonical package` if not supplied +- If there's an error while reading the `package meta`, it resolves to `true` because the package might be broken +- Otherwise, calls `_hasNew()` with the `canonical package` and `package meta` as arguments If the resolver is already working, either resolving or checking for a newer version, the promise is immediately rejected. diff --git a/lib/core/resolvers/GitResolver.js b/lib/core/resolvers/GitResolver.js index 9f4457c1..228515b7 100644 --- a/lib/core/resolvers/GitResolver.js +++ b/lib/core/resolvers/GitResolver.js @@ -17,7 +17,7 @@ mout.object.mixIn(GitResolver, Resolver); // ----------------- -GitResolver.prototype._hasNew = function (pkgMeta) { +GitResolver.prototype._hasNew = function (canonicalPkg, pkgMeta) { var oldResolution = pkgMeta._resolution || {}; return this._findResolution() diff --git a/lib/core/resolvers/Resolver.js b/lib/core/resolvers/Resolver.js index 0cc89957..ede8662b 100644 --- a/lib/core/resolvers/Resolver.js +++ b/lib/core/resolvers/Resolver.js @@ -38,15 +38,11 @@ Resolver.prototype.getTempDir = function () { return this._tempDir; }; -Resolver.prototype.hasNew = function (canonicalPkg) { +Resolver.prototype.hasNew = function (canonicalPkg, pkgMeta) { var promise; var metaFile; var that = this; - // TODO: Change arguments to canonicalPkg, pkgMeta - // where pkgMeta is optional - // Change _hasNew to the same - // If already working, error out if (this._working) { return Q.reject(createError('Already working', 'EWORKING')); @@ -54,18 +50,18 @@ Resolver.prototype.hasNew = function (canonicalPkg) { this._working = true; - // Avoid reading the package meta if _hasNew was not rewritten - if (this._hasNew === Resolver.prototype._hasNew) { - promise = this._hasNew(); + // Avoid reading the package meta if already given + if (pkgMeta) { + promise = this._hasNew(canonicalPkg, pkgMeta); // Otherwise call _hasNew with both the package meta and the canonical package } else { metaFile = path.join(canonicalPkg, '.bower.json'); promise = Q.nfcall(fs.readFile, metaFile) .then(function (contents) { var pkgMeta = JSON.parse(contents.toString()); - return that._hasNew(pkgMeta, canonicalPkg); + return that._hasNew(canonicalPkg, pkgMeta); }, function () { - return true; // Simply resolve to true if there was an error reading the meta + return true; // Simply resolve to true if there was an error reading the file }); } @@ -130,7 +126,7 @@ Resolver.prototype._resolve = function () { // ----------------- -Resolver.prototype._hasNew = function (pkgMeta) { +Resolver.prototype._hasNew = function (canonicalPkg, pkgMeta) { return Q.resolve(true); }; diff --git a/lib/core/resolvers/UrlResolver.js b/lib/core/resolvers/UrlResolver.js index b568c93c..5a421fed 100644 --- a/lib/core/resolvers/UrlResolver.js +++ b/lib/core/resolvers/UrlResolver.js @@ -35,7 +35,7 @@ util.inherits(UrlResolver, Resolver); // ----------------- -UrlResolver.prototype._hasNew = function (pkgMeta) { +UrlResolver.prototype._hasNew = function (canonicalPkg, pkgMeta) { var oldCacheHeaders = pkgMeta._cacheHeaders || {}; var reqHeaders = {}; diff --git a/test/core/resolverFactory.js b/test/core/resolverFactory.js index d829104b..09e97e6d 100644 --- a/test/core/resolverFactory.js +++ b/test/core/resolverFactory.js @@ -1,6 +1,7 @@ var expect = require('expect.js'); var fs = require('fs'); var path = require('path'); +var mkdirp = require('mkdirp'); var mout = require('mout'); var Q = require('q'); var rimraf = require('rimraf'); @@ -166,7 +167,7 @@ describe('resolverFactory', function () { var temp; tempSource = path.resolve(__dirname, '../assets/tmp'); - fs.mkdirSync(tempSource); + mkdirp.sync(tempSource); fs.writeFileSync(path.join(tempSource, '.git'), 'foo'); endpoints = {}; diff --git a/test/core/resolvers/fsResolver.js b/test/core/resolvers/fsResolver.js index 7bce1790..50ad7440 100644 --- a/test/core/resolvers/fsResolver.js +++ b/test/core/resolvers/fsResolver.js @@ -3,6 +3,7 @@ var path = require('path'); var fs = require('fs'); var path = require('path'); var rimraf = require('rimraf'); +var mkdirp = require('mkdirp'); var Q = require('q'); var cmd = require('../../../lib/util/cmd'); var copy = require('../../../lib/util/copy'); @@ -68,9 +69,15 @@ describe('FsResolver', function () { describe('.hasNew', function () { it('should resolve always to true (for now..)', function (next) { - var resolver = new FsResolver(path.relative(process.cwd(), testPackage)); + var resolver = new FsResolver(testPackage); - resolver.hasNew() + tempSource = path.resolve(__dirname, '../../assets/tmp'); + mkdirp.sync(tempSource); + fs.writeFileSync(path.join(tempSource, '.bower.json'), JSON.stringify({ + name: 'test' + })); + + resolver.hasNew(tempSource) .then(function (hasNew) { expect(hasNew).to.be(true); next(); @@ -145,7 +152,7 @@ describe('FsResolver', function () { tempSource = path.resolve(__dirname, '../../assets/tmp'); - fs.mkdirSync(tempSource); + mkdirp.sync(tempSource); resolver = new FsResolver(tempSource); copy.copyFile(path.join(testPackage, 'foo'), path.join(tempSource, 'foo')) diff --git a/test/core/resolvers/gitResolver.js b/test/core/resolvers/gitResolver.js index 459d9f4a..ceb125b9 100644 --- a/test/core/resolvers/gitResolver.js +++ b/test/core/resolvers/gitResolver.js @@ -4,6 +4,7 @@ var path = require('path'); var fs = require('fs'); var chmodr = require('chmodr'); var rimraf = require('rimraf'); +var mkdirp = require('mkdirp'); var Q = require('q'); var mout = require('mout'); var copy = require('../../../lib/util/copy'); @@ -20,7 +21,7 @@ describe('GitResolver', function () { describe('.hasNew', function () { before(function () { - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); }); afterEach(function (next) { @@ -625,7 +626,7 @@ describe('GitResolver', function () { describe('._cleanup', function () { beforeEach(function () { - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); }); afterEach(function (next) { @@ -704,7 +705,7 @@ describe('GitResolver', function () { describe('._savePkgMeta', function () { before(function () { - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); }); afterEach(function (next) { @@ -734,6 +735,63 @@ describe('GitResolver', function () { .done(); }); + it('should save the release (under _release)', function (next) { + var resolver = new GitResolver('foo'); + var metaFile = path.join(tempDir, '.bower.json'); + + // Test with type 'version' + resolver._resolution = { type: 'version', tag: '0.0.1', commit: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }; + resolver._tempDir = tempDir; + + resolver._savePkgMeta({ name: 'foo', version: '0.0.1' }) + .then(function () { + return Q.nfcall(fs.readFile, metaFile); + }) + .then(function (contents) { + var json = JSON.parse(contents.toString()); + expect(json._release).to.equal('0.0.1'); + }) + // Test with type 'tag' + .then(function () { + resolver._resolution = { type: 'tag', tag: '0.0.1', commit: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }; + return resolver._savePkgMeta({ name: 'foo' }); + }) + .then(function () { + return Q.nfcall(fs.readFile, metaFile); + }) + .then(function (contents) { + var json = JSON.parse(contents.toString()); + expect(json._release).to.equal('0.0.1'); + }) + // Test with type 'branch' + // In this case, it should be the commit + .then(function () { + resolver._resolution = { type: 'branch', commit: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }; + return resolver._savePkgMeta({ name: 'foo' }); + }) + .then(function () { + return Q.nfcall(fs.readFile, metaFile); + }) + .then(function (contents) { + var json = JSON.parse(contents.toString()); + expect(json._release).to.equal('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'); + }) + // Test with type 'commit' + .then(function () { + resolver._resolution = { type: 'commit', commit: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' }; + return resolver._savePkgMeta({ name: 'foo' }); + }) + .then(function () { + return Q.nfcall(fs.readFile, metaFile); + }) + .then(function (contents) { + var json = JSON.parse(contents.toString()); + expect(json._release).to.equal('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'); + next(); + }) + .done(); + }); + it('should add the version to the package meta if not present and resolution is a version', function (next) { var resolver = new GitResolver('foo'); @@ -819,8 +877,6 @@ describe('GitResolver', function () { }) .done(); }); - - it.skip('should save the release (under _release)'); }); describe('#fetchBranches', function () { diff --git a/test/core/resolvers/resolver.js b/test/core/resolvers/resolver.js index 4dde9d54..209ee139 100644 --- a/test/core/resolvers/resolver.js +++ b/test/core/resolvers/resolver.js @@ -3,7 +3,9 @@ var fs = require('fs'); var path = require('path'); var util = require('util'); var rimraf = require('rimraf'); +var mkdirp = require('mkdirp'); var tmp = require('tmp'); +var Q = require('q'); var cmd = require('../../../lib/util/cmd'); var copy = require('../../../lib/util/copy'); var Resolver = require('../../../lib/core/resolvers/Resolver'); @@ -55,6 +57,20 @@ describe('Resolver', function () { }); describe('.hasNew', function () { + before(function () { + mkdirp.sync(tempDir); + }); + + beforeEach(function () { + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ + name: 'test' + })); + }); + + after(function (next) { + rimraf(tempDir, next); + }); + it('should throw an error if already working (resolving)', function (next) { var resolver = new Resolver('foo'); var succeeded; @@ -71,7 +87,7 @@ describe('Resolver', function () { }) .done(); - resolver.hasNew() + resolver.hasNew(tempDir) .then(function () { succeeded = true; }, function (err) { @@ -85,17 +101,17 @@ describe('Resolver', function () { var resolver = new Resolver('foo'); var succeeded; - resolver.hasNew() + resolver.hasNew(tempDir) .then(function () { // Test if hasNew can be called again when done - resolver.hasNew() + resolver.hasNew(tempDir) .then(function () { next(succeeded ? new Error('Should have failed') : null); }); }) .done(); - resolver.hasNew() + resolver.hasNew(tempDir) .then(function () { succeeded = true; }, function (err) { @@ -108,13 +124,66 @@ describe('Resolver', function () { it('should resolve to true by default', function (next) { var resolver = new Resolver('foo'); - resolver.hasNew('.') + resolver.hasNew(tempDir) .then(function (hasNew) { expect(hasNew).to.equal(true); next(); }) .done(); }); + + it('should resolve to true if the there\'s an error reading the package meta', function (next) { + var resolver = new Resolver('foo'); + + rimraf.sync(path.join(tempDir, '.bower.json')); + resolver.hasNew(tempDir) + .then(function (hasNew) { + expect(hasNew).to.equal(true); + next(); + }) + .done(); + }); + + it('should call _hasNew with the canonical package and the package meta', function (next) { + var resolver = new Resolver('foo'); + var canonical; + var meta; + + resolver._hasNew = function (canonicalPkg, pkgMeta) { + canonical = canonicalPkg; + meta = pkgMeta; + return Q.resolve(true); + }; + + resolver.hasNew(tempDir) + .then(function () { + expect(canonical).to.equal(tempDir); + expect(meta).to.be.an('object'); + expect(meta.name).to.equal('test'); + next(); + }) + .done(); + }); + + it('should not read the package meta if already passed', function (next) { + var resolver = new Resolver('foo'); + var meta; + + resolver._hasNew = function (canonicalPkg, pkgMeta) { + meta = pkgMeta; + return Q.resolve(true); + }; + + resolver.hasNew(tempDir, { + name: 'foo' + }) + .then(function () { + expect(meta).to.be.an('object'); + expect(meta.name).to.equal('foo'); + next(); + }) + .done(); + }); }); describe('.resolve', function () { @@ -164,10 +233,10 @@ describe('Resolver', function () { resolver._resolve = function () {}; - resolver.hasNew() + resolver.hasNew(tempDir) .then(function () { // Test if hasNew can be called again when done - resolver.hasNew() + resolver.hasNew(tempDir) .then(function () { next(succeeded ? new Error('Should have failed') : null); }); @@ -355,7 +424,7 @@ describe('Resolver', function () { before(function () { var stat; - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); stat = fs.statSync(tempDir); dirMode0777 = stat.mode; }); @@ -456,7 +525,7 @@ describe('Resolver', function () { it('should read the bower.json file', function (next) { var resolver = new Resolver('foo'); - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); fs.writeFileSync(path.join(tempDir, 'bower.json'), JSON.stringify({ name: 'foo', version: '0.0.0' })); fs.writeFileSync(path.join(tempDir, 'component.json'), JSON.stringify({ name: 'bar', version: '0.0.0' })); @@ -474,7 +543,7 @@ describe('Resolver', function () { var resolver = new Resolver('foo'); var notified = false; - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); fs.writeFileSync(path.join(tempDir, 'component.json'), JSON.stringify({ name: 'bar', version: '0.0.0' })); resolver._readJson(tempDir) @@ -518,7 +587,7 @@ describe('Resolver', function () { var resolver = new Resolver('foo'); var meta = { name: 'foo' }; - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); resolver._tempDir = tempDir; resolver._applyPkgMeta(meta) @@ -540,7 +609,7 @@ describe('Resolver', function () { it('should remove files that match the ignore patterns', function (next) { var resolver = new Resolver('foo', { name: 'foo' }); - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); // Checkout test package version 0.2.1 which has a bower.json // with ignores @@ -585,7 +654,7 @@ describe('Resolver', function () { describe('._savePkgMeta', function () { before(function () { - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); }); afterEach(function (next) { diff --git a/test/core/resolvers/urlResolver.js b/test/core/resolvers/urlResolver.js index dc815a21..4ff9a9df 100644 --- a/test/core/resolvers/urlResolver.js +++ b/test/core/resolvers/urlResolver.js @@ -5,6 +5,7 @@ var path = require('path'); var nock = require('nock'); var Q = require('q'); var rimraf = require('rimraf'); +var mkdirp = require('mkdirp'); var cmd = require('../../../lib/util/cmd'); var UrlResolver = require('../../../lib/core/resolvers/UrlResolver'); @@ -64,7 +65,7 @@ describe('UrlResolver', function () { describe('.hasNew', function () { before(function () { - fs.mkdirSync(tempDir); + mkdirp.sync(tempDir); }); afterEach(function (next) {