From b17b57dc77855d275ac11b70a9b5d8fac7f782d8 Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Wed, 17 Jul 2019 14:48:26 -0300 Subject: [PATCH] Use require.resolve to lookup contracts in deps (#1110) * Use chai expect throw syntax in Dependency tests * Use require.resolve to lookup contracts in deps Change lookup in both Dependency and Contracts classes. Instead of looking for just node_modules in the current folder, it uses require.resolve (relative to the current workdir) to look for the dependency. Fixes #1076 * Fix tests --- package.json | 3 +- packages/cli/package-lock.json | 2 +- .../cli/src/models/dependency/Dependency.ts | 39 +++-- packages/cli/test/models/Dependency.test.js | 154 ++++++++---------- packages/cli/test/scripts/unlink.test.js | 2 +- packages/lib/src/artifacts/Contracts.ts | 14 +- packages/lib/src/project/ProxyAdminProject.ts | 8 +- packages/lib/src/project/SimpleProject.ts | 8 +- packages/lib/src/utils/Logger.ts | 13 +- .../lib/test/src/artifacts/Contracts.test.js | 6 + 10 files changed, 133 insertions(+), 116 deletions(-) diff --git a/package.json b/package.json index 5c1d94344..e76b4377e 100644 --- a/package.json +++ b/package.json @@ -6,6 +6,7 @@ }, "devDependencies": { "lerna": "~3.13.1", - "typescript": "^3.2.2" + "typescript": "^3.2.2", + "mock-stdlib-root": "file:./packages/cli/test/mocks/mock-stdlib" } } diff --git a/packages/cli/package-lock.json b/packages/cli/package-lock.json index f2f269524..e485ae872 100644 --- a/packages/cli/package-lock.json +++ b/packages/cli/package-lock.json @@ -1,6 +1,6 @@ { "name": "zos", - "version": "2.4.0", + "version": "2.4.1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/packages/cli/src/models/dependency/Dependency.ts b/packages/cli/src/models/dependency/Dependency.ts index 6d4002746..6836b6463 100644 --- a/packages/cli/src/models/dependency/Dependency.ts +++ b/packages/cli/src/models/dependency/Dependency.ts @@ -57,13 +57,21 @@ export default class Dependency { } } - public static hasDependenciesForDeploy(network: string): boolean { - const dependencies = ZosPackageFile.getLinkedDependencies() || []; + public static hasDependenciesForDeploy( + network: string, + packageFilename = 'zos.json', + networkFilename: string = undefined, + ): boolean { + const dependencies = + ZosPackageFile.getLinkedDependencies(packageFilename) || []; const networkDependencies = - ZosNetworkFile.getDependencies(`zos.${network}.json`) || {}; + ZosNetworkFile.getDependencies( + networkFilename || `zos.${network}.json`, + ) || {}; const hasDependenciesForDeploy = dependencies.find(depNameAndVersion => { const [name, version] = depNameAndVersion.split('@'); - const networkFilePath = `node_modules/${name}/zos.${network}.json`; + const dependency = new Dependency(name); + const networkFilePath = dependency._getNetworkFilePath(network); const projectDependency = networkDependencies[name]; const satisfiesVersion = projectDependency && @@ -151,15 +159,16 @@ export default class Dependency { public getPackageFile(): ZosPackageFile | never { if (!this._packageFile) { - const filename = `node_modules/${this.name}/zos.json`; - if (!fs.exists(filename)) { + try { + const filename = require.resolve(`${this.name}/zos.json`, { + paths: [process.cwd()], + }); + this._packageFile = new ZosPackageFile(filename); + } catch (err) { throw Error( - `Could not find a zos.json file for '${ - this.name - }'. Make sure it is provided by the npm package.`, + `Could not find a zos.json file for '${this.name}'. (${err.message})`, ); } - this._packageFile = new ZosPackageFile(filename); } return this._packageFile; } @@ -171,7 +180,7 @@ export default class Dependency { throw Error( `Could not find a zos file for network '${network}' for '${ this.name - }'`, + }'.`, ); } @@ -195,7 +204,13 @@ export default class Dependency { } private _getNetworkFilePath(network: string): string { - return `node_modules/${this.name}/zos.${network}.json`; + try { + return require.resolve(`${this.name}/zos.${network}.json`, { + paths: [process.cwd()], + }); + } catch (err) { + return null; + } } private _validateSatisfiesVersion( diff --git a/packages/cli/test/models/Dependency.test.js b/packages/cli/test/models/Dependency.test.js index d7f9083c6..078bb8ae7 100644 --- a/packages/cli/test/models/Dependency.test.js +++ b/packages/cli/test/models/Dependency.test.js @@ -4,18 +4,9 @@ require('../setup'); import sinon from 'sinon'; import npm from 'npm-programmatic'; -import { FileSystem as fs } from 'zos-lib'; import Dependency from '../../src/models/dependency/Dependency'; -contract('Dependency', function([_, from]) { - const assertErrorMessage = (fn, errorMessage) => { - try { - fn(); - } catch (error) { - error.message.should.match(errorMessage); - } - }; - +contract.only('Dependency', function([_, from]) { describe('static methods', function() { describe('#satisfiesVersion', function() { it('verifies if requirement satisfies version', function() { @@ -27,10 +18,9 @@ contract('Dependency', function([_, from]) { describe('#fromNameAndVersion', function() { describe('with invalid nameAndVersion', function() { it('throws error', function() { - assertErrorMessage( - () => Dependency.fromNameWithVersion('bildts-kcom'), - /Could not find a zos.json file/, - ); + expect( + () => Dependency.fromNameWithVersion('bildts-kcom') + ).to.throw(/Cannot find module/); }); }); @@ -62,41 +52,21 @@ contract('Dependency', function([_, from]) { context('when there are dependencies to deploy', function() { it('returns true', function() { - const projectPackageFile = fs.parseJsonIfExists( + Dependency.hasDependenciesForDeploy( + 'test', 'test/mocks/packages/package-with-multiple-stdlibs.zos.json', - ); - const projectNetworkFile = fs.parseJsonIfExists( - 'test/mocks/networks/network-with-stdlibs.zos.test.json', - ); - const stubbedParseJsonIfExists = sinon.stub(fs, 'parseJsonIfExists'); - stubbedParseJsonIfExists - .withArgs('zos.json') - .returns(projectPackageFile); - stubbedParseJsonIfExists - .withArgs('zos.test.json') - .returns(projectNetworkFile); - - Dependency.hasDependenciesForDeploy('test').should.be.true; + 'test/mocks/networks/network-with-stdlibs.zos.test.json' + ).should.be.true; }); }); context('when all dependencies are already deployed', function() { it('returns false', function() { - const projectPackageFile = fs.parseJsonIfExists( + Dependency.hasDependenciesForDeploy( + 'test', 'test/mocks/packages/package-with-stdlib.zos.json', - ); - const projectNetworkFile = fs.parseJsonIfExists( - 'test/mocks/networks/network-with-stdlibs.zos.test.json', - ); - const stubbedParseJsonIfExists = sinon.stub(fs, 'parseJsonIfExists'); - stubbedParseJsonIfExists - .withArgs('zos.json') - .returns(projectPackageFile); - stubbedParseJsonIfExists - .withArgs('zos.test.json') - .returns(projectNetworkFile); - - Dependency.hasDependenciesForDeploy('test').should.be.false; + 'test/mocks/networks/network-with-stdlibs.zos.test.json' + ).should.be.false; }); }); }); @@ -105,19 +75,25 @@ contract('Dependency', function([_, from]) { describe('#constructor', function() { context('with invalid version', function() { it('throws an error', function() { - assertErrorMessage( - () => new Dependency('mock-stdlib', '1.2.0'), - /does not match version/, - ); + expect( + () => new Dependency('mock-stdlib', '1.2.0') + ).to.throw(/does not match version/); }); }); context('with non-existent dependency name', function() { it('throws an error', function() { - assertErrorMessage( - () => new Dependency('bildts-kcom', '1.1.0'), - /Could not find a zos.json file/, - ); + expect( + () => new Dependency('bildts-kcom', '1.1.0') + ).to.throw(/Cannot find module/); + }); + }); + + context('with non-ethereum package', function() { + it('throws an error', function() { + expect( + () => new Dependency('chai') + ).to.throw(/Cannot find module/); }); }); @@ -138,52 +114,54 @@ contract('Dependency', function([_, from]) { }); }); - describe('instance methods', function() { - beforeEach(function() { - this.dependency = new Dependency('mock-stdlib', '1.1.0'); - this.txParams = {}; - this.addresses = {}; - delete this.dependency._packageFile; - }); - - describe('#deploy', function() { - it('deploys a dependency', async function() { - const project = await this.dependency.deploy({ from }); - const address = await project.getImplementation({ - contractName: 'Greeter', + function testInstanceMethodsFor(libname) { + describe(`instance methods for ${libname}`, function() { + beforeEach(function() { + this.dependency = new Dependency(libname, '1.1.0'); + this.txParams = {}; + this.addresses = {}; + delete this.dependency._projectFile; + }); + + describe('#deploy', function() { + it('deploys a dependency', async function() { + const project = await this.dependency.deploy({ from }); + const address = await project.getImplementation({ + contractName: 'Greeter', + }); + address.should.be.nonzeroAddress; }); - address.should.be.nonzeroAddress; }); - }); - describe('#getPackageFile', function() { - it('generates a package file', function() { - const packageFile = this.dependency.getPackageFile(); - packageFile.should.not.be.null; - packageFile.fileName.should.eq('node_modules/mock-stdlib/zos.json'); - packageFile.version.should.eq('1.1.0'); - packageFile.contracts.should.include({ Greeter: 'GreeterImpl' }); + describe('#projectFile', function() { + it('generates a package file', function() { + const projectFile = this.dependency.getPackageFile(); + projectFile.should.not.be.null; + projectFile.fileName.should.match(/mock-stdlib\/zos\.json$/); + projectFile.version.should.eq('1.1.0'); + projectFile.contracts.should.include({ Greeter: 'GreeterImpl' }); + }); }); - }); - describe('#getNetworkFile', function() { - context('for a non-existent network', function() { - it('throws an error', function() { - assertErrorMessage( - () => this.dependency.getNetworkFile('bildts-kcom'), - /Could not find a zos file for network/, - ); + describe('#getNetworkFile', function() { + context('for a non-existent network', function() { + it('throws an error', function() { + expect( + () => this.dependency.getNetworkFile('bildts-kcom') + ).to.throw(/Could not find a zos file/); + }); }); - }); - context('for an existent network', function() { - it('generates network file', function() { - const networkFile = this.dependency.getNetworkFile('test'); - networkFile.fileName.should.eq( - 'node_modules/mock-stdlib/zos.test.json', - ); + context('for an existent network', function() { + it('generates network file', function() { + const networkFile = this.dependency.getNetworkFile('test'); + networkFile.fileName.should.match(/mock-stdlib\/zos\.test\.json$/); + }); }); }); }); - }); + } + + testInstanceMethodsFor('mock-stdlib'); + testInstanceMethodsFor('mock-stdlib-root'); }); diff --git a/packages/cli/test/scripts/unlink.test.js b/packages/cli/test/scripts/unlink.test.js index 58c1127b2..432d6a9d4 100644 --- a/packages/cli/test/scripts/unlink.test.js +++ b/packages/cli/test/scripts/unlink.test.js @@ -28,7 +28,7 @@ contract('unlink script', function() { dependencies: [dependencyName], packageFile: this.packageFile, }).should.be.rejectedWith( - `Could not find a zos.json file for '${dependencyName}'. Make sure it is provided by the npm package.`, + /Cannot find module/, ); }); }); diff --git a/packages/lib/src/artifacts/Contracts.ts b/packages/lib/src/artifacts/Contracts.ts index 46148f4d4..20019c893 100644 --- a/packages/lib/src/artifacts/Contracts.ts +++ b/packages/lib/src/artifacts/Contracts.ts @@ -12,6 +12,7 @@ export default class Contracts { private static timeout: number = Contracts.DEFAULT_SYNC_TIMEOUT; private static buildDir: string = Contracts.DEFAULT_BUILD_DIR; private static contractsDir: string = Contracts.DEFAULT_CONTRACTS_DIR; + private static projectRoot: string = null; private static artifactDefaults: any = {}; private static defaultFromAddress: string; @@ -29,6 +30,10 @@ export default class Contracts { ); } + public static getProjectRoot(): string { + return path.resolve(this.projectRoot || process.cwd()); + } + public static async getDefaultTxParams(): Promise { const defaults = { ...Contracts.getArtifactsDefaults() }; if (!defaults.from) defaults.from = await Contracts.getDefaultFromAddress(); @@ -54,7 +59,10 @@ export default class Contracts { dependency: string, contractName: string, ): string { - return `${process.cwd()}/node_modules/${dependency}/build/contracts/${contractName}.json`; + return require.resolve( + `${dependency}/build/contracts/${contractName}.json`, + { paths: [this.getProjectRoot()] }, + ); } public static getFromLocal(contractName: string): Contract { @@ -98,6 +106,10 @@ export default class Contracts { Contracts.contractsDir = dir; } + public static setProjectRoot(dir: string): void { + Contracts.projectRoot = dir; + } + public static setArtifactsDefaults(defaults: any): void { Contracts.artifactDefaults = { ...Contracts.getArtifactsDefaults(), diff --git a/packages/lib/src/project/ProxyAdminProject.ts b/packages/lib/src/project/ProxyAdminProject.ts index 87876bc7c..1f59ea85d 100644 --- a/packages/lib/src/project/ProxyAdminProject.ts +++ b/packages/lib/src/project/ProxyAdminProject.ts @@ -65,10 +65,10 @@ class BaseProxyAdminProject extends BaseSimpleProject { initCallData, } = await this._setUpgradeParams(proxyAddress, contract, contractParams); Loggy.spin( - __filename, - 'upgradeProxy', - `action-proxy-${pAddress}`, - `Upgrading instance at ${pAddress}` + __filename, + 'upgradeProxy', + `action-proxy-${pAddress}`, + `Upgrading instance at ${pAddress}`, ); await this.proxyAdmin.upgradeProxy( pAddress, diff --git a/packages/lib/src/project/SimpleProject.ts b/packages/lib/src/project/SimpleProject.ts index f49e4e80d..86fd44786 100644 --- a/packages/lib/src/project/SimpleProject.ts +++ b/packages/lib/src/project/SimpleProject.ts @@ -27,10 +27,10 @@ export default class SimpleProject extends BaseSimpleProject { initCallData, } = await this._setUpgradeParams(proxyAddress, contract, contractParams); Loggy.spin( - __filename, - 'upgradeProxy', - `action-proxy-${pAddress}`, - `Upgrading instance at ${pAddress}` + __filename, + 'upgradeProxy', + `action-proxy-${pAddress}`, + `Upgrading instance at ${pAddress}`, ); const proxy = Proxy.at(pAddress, this.txParams); await proxy.upgradeTo(implementationAddress, initCallData); diff --git a/packages/lib/src/utils/Logger.ts b/packages/lib/src/utils/Logger.ts index adc2f3551..8e51c531a 100644 --- a/packages/lib/src/utils/Logger.ts +++ b/packages/lib/src/utils/Logger.ts @@ -124,9 +124,14 @@ export const Loggy: { [key: string]: any } = { _log(reference: string): void { try { if (this.isSilent) return; - const { file, fnName, text, spinnerAction, logLevel, logType } = this.logs[ - reference - ]; + const { + file, + fnName, + text, + spinnerAction, + logLevel, + logType, + } = this.logs[reference]; const color = this._getColorFor(logType); if (this.isVerbose || this.isTesting) { const location = `${path.basename(file)}#${fnName}`; @@ -134,7 +139,7 @@ export const Loggy: { [key: string]: any } = { spinnerAction, )}> ${text}`; const coloredMessage = color ? chalk.keyword(color)(message) : message; - console.error(coloredMessage); + if (!this.isTesting) console.error(coloredMessage); } else if (logLevel === LogLevel.Normal) { const options = color ? { text, status: spinnerAction, color } diff --git a/packages/lib/test/src/artifacts/Contracts.test.js b/packages/lib/test/src/artifacts/Contracts.test.js index 0be689291..97a55b363 100644 --- a/packages/lib/test/src/artifacts/Contracts.test.js +++ b/packages/lib/test/src/artifacts/Contracts.test.js @@ -22,6 +22,12 @@ contract('Contracts', function() { instance.address.should.not.be.null; }); + it('can lookup contracts from hoisted node modules', async function() { + const GreeterLib = Contracts.getFromNodeModules('mock-stdlib-root', 'GreeterLib'); + const instance = await GreeterLib.new(); + instance.address.should.not.be.null; + }); + describe('configuration', function() { it('has some default configuration', function() { Contracts.getSyncTimeout().should.be.eq(240000);