From 0a999c0f1b67353089f1e891b2f5a3f198836cd5 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sat, 13 Jul 2019 10:48:43 +0200 Subject: [PATCH 1/4] chore: upgrade solidity-coverage to v0.6+ --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index ca57aac09..c498a52f4 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "ethereumjs-abi": "^0.6.5", "ganache-cli": "^6.4.2", "mocha-lcov-reporter": "^1.3.0", - "solidity-coverage": "0.5.8", + "solidity-coverage": "^0.6.1", "solium": "^1.2.3", "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1", From 27e4cdaf6ef7bd2c4917878e2aaa4f2b9b5b12f8 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sat, 13 Jul 2019 13:18:22 +0200 Subject: [PATCH 2/4] Enhance coverage configuration due to upgrades to coverage parser and chain --- .solcover.js | 1 - test/helpers/assertThrow.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.solcover.js b/.solcover.js index 38a69357e..706360613 100644 --- a/.solcover.js +++ b/.solcover.js @@ -3,7 +3,6 @@ const skipFiles = [ 'test', 'acl/ACLSyntaxSugar.sol', 'common/DepositableStorage.sol', // Used in tests that send ETH - 'common/SafeERC20.sol', // solidity-coverage fails on assembly if (https://github.com/sc-forks/solidity-coverage/issues/287) 'common/UnstructuredStorage.sol' // Used in tests that send ETH ] diff --git a/test/helpers/assertThrow.js b/test/helpers/assertThrow.js index c15565328..e70f6c26e 100644 --- a/test/helpers/assertThrow.js +++ b/test/helpers/assertThrow.js @@ -31,7 +31,7 @@ module.exports = { error.reason = error.message.replace(errorPrefix, '').trim() } - if (process.env.SOLIDITY_COVERAGE !== 'true' && reason) { + if (reason) { assert.equal(error.reason, reason, `Expected revert reason "${reason}" but failed with "${error.reason || 'no reason'}" instead.`) } }, From 25a646c278a5aa07f269951bf3bb6bd2beccb34f Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sat, 13 Jul 2019 13:18:45 +0200 Subject: [PATCH 3/4] Fix coverage for ETH tests --- test/contracts/apps/app_funds.js | 20 ++++++++--- test/contracts/apps/recovery_to_vault.js | 43 +++++++++++------------- test/contracts/kernel/kernel_funds.js | 17 +++++++--- 3 files changed, 47 insertions(+), 33 deletions(-) diff --git a/test/contracts/apps/app_funds.js b/test/contracts/apps/app_funds.js index 623736272..3fa3f30cb 100644 --- a/test/contracts/apps/app_funds.js +++ b/test/contracts/apps/app_funds.js @@ -1,7 +1,8 @@ const { hash } = require('eth-ens-namehash') const { onlyIf } = require('../../helpers/onlyIf') -const { getBalance } = require('../../helpers/web3') const { assertRevert } = require('../../helpers/assertThrow') +const { skipCoverage } = require('../../helpers/coverage') +const { getBalance } = require('../../helpers/web3') const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') @@ -48,6 +49,15 @@ contract('App funds', ([permissionsRoot]) => { // Test the app itself and when it's behind the proxies to make sure their behaviours are the same const appProxyTypes = ['AppProxyUpgradeable', 'AppProxyPinned'] for (const appType of ['App', ...appProxyTypes]) { + const skipCoverageIfProxy = test => { + // The AppStubs aren't instrumented during coverage, but the AppProxys are, and so + // attempting to use the fallback fails when we're testing with the proxy. + // Native transfers (either .send() or .transfer()) fail under coverage because they're + // limited to 2.3k gas, and the injected instrumentation from coverage makes these + // operations cost more than that limit. + return appType === 'App' ? test : skipCoverage(test) + } + context(`> ${appType}`, () => { let appBase, app @@ -85,11 +95,11 @@ contract('App funds', ([permissionsRoot]) => { await app.initialize() }) - it('cannot receive ETH', async () => { + it('cannot receive ETH', skipCoverageIfProxy(async () => { assert.isTrue(await app.hasInitialized(), 'should have been initialized') await assertRevert(app.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - }) + })) onlyAppStubDepositable(() => { it('does not have depositing enabled by default', async () => { @@ -97,7 +107,7 @@ contract('App funds', ([permissionsRoot]) => { assert.isFalse(await app.isDepositable(), 'should not be depositable') }) - it('can receive ETH after being set to depositable', async () => { + it('can receive ETH after being set to depositable', skipCoverageIfProxy(async () => { const amount = 1 const initialBalance = await getBalance(app.address) @@ -106,7 +116,7 @@ contract('App funds', ([permissionsRoot]) => { await app.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(app.address)).valueOf(), initialBalance.plus(amount)) - }) + })) }) }) } diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index 6f5e2010c..cf1995354 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/test/contracts/apps/recovery_to_vault.js @@ -1,9 +1,9 @@ const { hash } = require('eth-ens-namehash') -const { getBalance } = require('../../helpers/web3') -const { skipCoverage } = require('../../helpers/coverage') +const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3) const { assertRevert } = require('../../helpers/assertThrow') +const { skipCoverage } = require('../../helpers/coverage') const { getNewProxyAddress } = require('../../helpers/events') -const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3) +const { getBalance } = require('../../helpers/web3') const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') @@ -160,15 +160,6 @@ contract('Recovery to vault', ([permissionsRoot]) => { // Test both the Vault itself and when it's behind a proxy to make sure their recovery behaviours are the same for (const vaultType of ['Vault', 'VaultProxy']) { - const skipCoverageIfVaultProxy = test => { - // The VaultMock isn't instrumented during coverage, but the AppProxy is, and so - // transferring to the fallback fails when we're testing with the proxy. - // Native transfers (either .send() or .transfer()) fail under coverage because they're - // limited to 2.3k gas, and the injected instrumentation from coverage makes these - // operations cost more than that limit. - return vaultType === 'VaultProxy' ? skipCoverage(test) : test - } - context(`> ${vaultType}`, () => { let target, vault @@ -190,9 +181,9 @@ contract('Recovery to vault', ([permissionsRoot]) => { await kernel.setRecoveryVaultAppId(vaultId) }) - it('kernel cannot receive ETH', async () => + it('kernel cannot receive ETH', skipCoverage(async () => await assertRevert(kernel.sendTransaction({ value: 1, gas: 31000 })) - ) + )) for (const { title, tokenContract } of tokenTestGroups) { it(`kernel recovers ${title}`, async () => { @@ -218,7 +209,7 @@ contract('Recovery to vault', ([permissionsRoot]) => { await target.enableDeposits() }) - it('does not recover ETH', skipCoverageIfVaultProxy(async () => + it('does not recover ETH', skipCoverage(async () => await recoverEth({ target, vault, shouldFail: true }) )) @@ -233,6 +224,12 @@ contract('Recovery to vault', ([permissionsRoot]) => { }) context('> Proxied app with kernel', () => { + // Any of these tests involving an ETH transfer are skipped in coverage because the + // target is an AppProxy, which gets instrumented by solidity-coverage. + // Native transfers (either .send() or .transfer()) fail under coverage because they're + // limited to 2.3k gas, and the injected instrumentation makes these operations cost + // more than that limit. + beforeEach(async () => { // Setup app const receipt = await kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false) @@ -243,15 +240,15 @@ contract('Recovery to vault', ([permissionsRoot]) => { target = app }) - it('cannot send 0 ETH to proxy', async () => { + it('cannot send 0 ETH to proxy', skipCoverage(async () => { await assertRevert(target.sendTransaction({ value: 0, gas: SEND_ETH_GAS })) - }) + })) - it('cannot send ETH with data to proxy', async () => { + it('cannot send ETH with data to proxy', skipCoverage(async () => { await assertRevert(target.sendTransaction({ value: 1, data: '0x01', gas: SEND_ETH_GAS })) - }) + })) - it('recovers ETH', skipCoverageIfVaultProxy(async () => + it('recovers ETH', skipCoverage(async () => await recoverEth({ target, vault }) )) @@ -273,9 +270,9 @@ contract('Recovery to vault', ([permissionsRoot]) => { }) } - it('fails if vault is not contract', async () => { + it('fails if vault is not contract', skipCoverage(async () => { await failWithoutVault(target, kernel) - }) + })) }) context('> Conditional fund recovery', () => { @@ -289,7 +286,7 @@ contract('Recovery to vault', ([permissionsRoot]) => { target = app }) - it('does not allow recovering ETH', skipCoverageIfVaultProxy(async () => + it('does not allow recovering ETH', skipCoverage(async () => // Conditional stub doesnt allow eth recoveries await recoverEth({ target, vault, shouldFail: true }) )) diff --git a/test/contracts/kernel/kernel_funds.js b/test/contracts/kernel/kernel_funds.js index 8a6c044a1..e66dee08b 100644 --- a/test/contracts/kernel/kernel_funds.js +++ b/test/contracts/kernel/kernel_funds.js @@ -1,6 +1,7 @@ +const { assertRevert } = require('../../helpers/assertThrow') +const { skipCoverage } = require('../../helpers/coverage') const { onlyIf } = require('../../helpers/onlyIf') const { getBalance } = require('../../helpers/web3') -const { assertRevert } = require('../../helpers/assertThrow') const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') @@ -12,6 +13,12 @@ const KernelDepositableMock = artifacts.require('KernelDepositableMock') const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies contract('Kernel funds', ([permissionsRoot]) => { + // Any of these tests involving an ETH transfer are skipped in coverage because the + // target is a Kernel or KernelProxy, which gets instrumented by solidity-coverage. + // Native transfers (either .send() or .transfer()) fail under coverage because they're + // limited to 2.3k gas, and the injected instrumentation makes these operations cost + // more than that limit. + let aclBase // Initial setup @@ -43,7 +50,7 @@ contract('Kernel funds', ([permissionsRoot]) => { } }) - it('cannot receive ETH', async () => { + it('cannot receive ETH', skipCoverage(async () => { // Before initialization assert.isFalse(await kernel.hasInitialized(), 'should not have been initialized') @@ -54,7 +61,7 @@ contract('Kernel funds', ([permissionsRoot]) => { assert.isTrue(await kernel.hasInitialized(), 'should have been initialized') await assertRevert(kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - }) + })) onlyKernelDepositable(() => { it('does not have depositing enabled by default', async () => { @@ -68,7 +75,7 @@ contract('Kernel funds', ([permissionsRoot]) => { assert.isFalse(await kernel.isDepositable(), 'should not be depositable') }) - it('can receive ETH after being enabled', async () => { + it('can receive ETH after being enabled', skipCoverage(async () => { const amount = 1 const initialBalance = await getBalance(kernel.address) @@ -79,7 +86,7 @@ contract('Kernel funds', ([permissionsRoot]) => { await kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(kernel.address)).valueOf(), initialBalance.plus(amount)) - }) + })) }) }) } From 57063351088a4311af5bba8f8c8bf1539971596e Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Sun, 14 Jul 2019 13:19:37 +0200 Subject: [PATCH 4/4] coverage: upgrade solidity-coverage and avoid skipping ETH transfer tests in coverage --- package.json | 2 +- test/contracts/apps/app_funds.js | 18 +++-------- test/contracts/apps/recovery_to_vault.js | 39 ++++++++++-------------- test/contracts/kernel/kernel_funds.js | 15 +++------ test/helpers/coverage.js | 14 --------- 5 files changed, 25 insertions(+), 63 deletions(-) delete mode 100644 test/helpers/coverage.js diff --git a/package.json b/package.json index c498a52f4..5bea6fdc1 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "ethereumjs-abi": "^0.6.5", "ganache-cli": "^6.4.2", "mocha-lcov-reporter": "^1.3.0", - "solidity-coverage": "^0.6.1", + "solidity-coverage": "^0.6.2", "solium": "^1.2.3", "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1", diff --git a/test/contracts/apps/app_funds.js b/test/contracts/apps/app_funds.js index 3fa3f30cb..f94433287 100644 --- a/test/contracts/apps/app_funds.js +++ b/test/contracts/apps/app_funds.js @@ -1,7 +1,6 @@ const { hash } = require('eth-ens-namehash') const { onlyIf } = require('../../helpers/onlyIf') const { assertRevert } = require('../../helpers/assertThrow') -const { skipCoverage } = require('../../helpers/coverage') const { getBalance } = require('../../helpers/web3') const ACL = artifacts.require('ACL') @@ -49,15 +48,6 @@ contract('App funds', ([permissionsRoot]) => { // Test the app itself and when it's behind the proxies to make sure their behaviours are the same const appProxyTypes = ['AppProxyUpgradeable', 'AppProxyPinned'] for (const appType of ['App', ...appProxyTypes]) { - const skipCoverageIfProxy = test => { - // The AppStubs aren't instrumented during coverage, but the AppProxys are, and so - // attempting to use the fallback fails when we're testing with the proxy. - // Native transfers (either .send() or .transfer()) fail under coverage because they're - // limited to 2.3k gas, and the injected instrumentation from coverage makes these - // operations cost more than that limit. - return appType === 'App' ? test : skipCoverage(test) - } - context(`> ${appType}`, () => { let appBase, app @@ -95,11 +85,11 @@ contract('App funds', ([permissionsRoot]) => { await app.initialize() }) - it('cannot receive ETH', skipCoverageIfProxy(async () => { + it('cannot receive ETH', async () => { assert.isTrue(await app.hasInitialized(), 'should have been initialized') await assertRevert(app.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - })) + }) onlyAppStubDepositable(() => { it('does not have depositing enabled by default', async () => { @@ -107,7 +97,7 @@ contract('App funds', ([permissionsRoot]) => { assert.isFalse(await app.isDepositable(), 'should not be depositable') }) - it('can receive ETH after being set to depositable', skipCoverageIfProxy(async () => { + it('can receive ETH after being set to depositable', async () => { const amount = 1 const initialBalance = await getBalance(app.address) @@ -116,7 +106,7 @@ contract('App funds', ([permissionsRoot]) => { await app.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(app.address)).valueOf(), initialBalance.plus(amount)) - })) + }) }) }) } diff --git a/test/contracts/apps/recovery_to_vault.js b/test/contracts/apps/recovery_to_vault.js index cf1995354..626bc67de 100644 --- a/test/contracts/apps/recovery_to_vault.js +++ b/test/contracts/apps/recovery_to_vault.js @@ -1,7 +1,6 @@ const { hash } = require('eth-ens-namehash') const { assertAmountOfEvents, assertEvent } = require('../../helpers/assertEvent')(web3) const { assertRevert } = require('../../helpers/assertThrow') -const { skipCoverage } = require('../../helpers/coverage') const { getNewProxyAddress } = require('../../helpers/events') const { getBalance } = require('../../helpers/web3') @@ -181,9 +180,9 @@ contract('Recovery to vault', ([permissionsRoot]) => { await kernel.setRecoveryVaultAppId(vaultId) }) - it('kernel cannot receive ETH', skipCoverage(async () => + it('kernel cannot receive ETH', async () => await assertRevert(kernel.sendTransaction({ value: 1, gas: 31000 })) - )) + ) for (const { title, tokenContract } of tokenTestGroups) { it(`kernel recovers ${title}`, async () => { @@ -209,9 +208,9 @@ contract('Recovery to vault', ([permissionsRoot]) => { await target.enableDeposits() }) - it('does not recover ETH', skipCoverage(async () => + it('does not recover ETH', async () => await recoverEth({ target, vault, shouldFail: true }) - )) + ) it('does not recover tokens', async () => await recoverTokens({ @@ -224,12 +223,6 @@ contract('Recovery to vault', ([permissionsRoot]) => { }) context('> Proxied app with kernel', () => { - // Any of these tests involving an ETH transfer are skipped in coverage because the - // target is an AppProxy, which gets instrumented by solidity-coverage. - // Native transfers (either .send() or .transfer()) fail under coverage because they're - // limited to 2.3k gas, and the injected instrumentation makes these operations cost - // more than that limit. - beforeEach(async () => { // Setup app const receipt = await kernel.newAppInstance(APP_ID, appBase.address, EMPTY_BYTES, false) @@ -240,17 +233,17 @@ contract('Recovery to vault', ([permissionsRoot]) => { target = app }) - it('cannot send 0 ETH to proxy', skipCoverage(async () => { + it('cannot send 0 ETH to proxy', async () => { await assertRevert(target.sendTransaction({ value: 0, gas: SEND_ETH_GAS })) - })) + }) - it('cannot send ETH with data to proxy', skipCoverage(async () => { + it('cannot send ETH with data to proxy', async () => { await assertRevert(target.sendTransaction({ value: 1, data: '0x01', gas: SEND_ETH_GAS })) - })) + }) - it('recovers ETH', skipCoverage(async () => + it('recovers ETH', async () => await recoverEth({ target, vault }) - )) + ) for (const { title, tokenContract } of tokenTestGroups) { it(`recovers ${title}`, async () => { @@ -270,9 +263,9 @@ contract('Recovery to vault', ([permissionsRoot]) => { }) } - it('fails if vault is not contract', skipCoverage(async () => { + it('fails if vault is not contract', async () => { await failWithoutVault(target, kernel) - })) + }) }) context('> Conditional fund recovery', () => { @@ -286,10 +279,10 @@ contract('Recovery to vault', ([permissionsRoot]) => { target = app }) - it('does not allow recovering ETH', skipCoverage(async () => + it('does not allow recovering ETH', async () => // Conditional stub doesnt allow eth recoveries await recoverEth({ target, vault, shouldFail: true }) - )) + ) for (const { title, tokenContract } of tokenTestGroups) { it(`allows recovers ${title}`, async () => { @@ -341,8 +334,8 @@ contract('Recovery to vault', ([permissionsRoot]) => { await kernel.setRecoveryVaultAppId(vaultId) }) - it('recovers ETH from the kernel', skipCoverage(async () => { + it('recovers ETH from the kernel', async () => { await recoverEth({ target: kernel, vault }) - })) + }) }) }) diff --git a/test/contracts/kernel/kernel_funds.js b/test/contracts/kernel/kernel_funds.js index e66dee08b..de80d198e 100644 --- a/test/contracts/kernel/kernel_funds.js +++ b/test/contracts/kernel/kernel_funds.js @@ -1,5 +1,4 @@ const { assertRevert } = require('../../helpers/assertThrow') -const { skipCoverage } = require('../../helpers/coverage') const { onlyIf } = require('../../helpers/onlyIf') const { getBalance } = require('../../helpers/web3') @@ -13,12 +12,6 @@ const KernelDepositableMock = artifacts.require('KernelDepositableMock') const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies contract('Kernel funds', ([permissionsRoot]) => { - // Any of these tests involving an ETH transfer are skipped in coverage because the - // target is a Kernel or KernelProxy, which gets instrumented by solidity-coverage. - // Native transfers (either .send() or .transfer()) fail under coverage because they're - // limited to 2.3k gas, and the injected instrumentation makes these operations cost - // more than that limit. - let aclBase // Initial setup @@ -50,7 +43,7 @@ contract('Kernel funds', ([permissionsRoot]) => { } }) - it('cannot receive ETH', skipCoverage(async () => { + it('cannot receive ETH', async () => { // Before initialization assert.isFalse(await kernel.hasInitialized(), 'should not have been initialized') @@ -61,7 +54,7 @@ contract('Kernel funds', ([permissionsRoot]) => { assert.isTrue(await kernel.hasInitialized(), 'should have been initialized') await assertRevert(kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS })) - })) + }) onlyKernelDepositable(() => { it('does not have depositing enabled by default', async () => { @@ -75,7 +68,7 @@ contract('Kernel funds', ([permissionsRoot]) => { assert.isFalse(await kernel.isDepositable(), 'should not be depositable') }) - it('can receive ETH after being enabled', skipCoverage(async () => { + it('can receive ETH after being enabled', async () => { const amount = 1 const initialBalance = await getBalance(kernel.address) @@ -86,7 +79,7 @@ contract('Kernel funds', ([permissionsRoot]) => { await kernel.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) assert.equal((await getBalance(kernel.address)).valueOf(), initialBalance.plus(amount)) - })) + }) }) }) } diff --git a/test/helpers/coverage.js b/test/helpers/coverage.js deleted file mode 100644 index f5f313242..000000000 --- a/test/helpers/coverage.js +++ /dev/null @@ -1,14 +0,0 @@ -const skipCoverage = test => { - // Required dynamic this binding to attach onto the running test - return function skipCoverage() { - if (process.env.SOLIDITY_COVERAGE === 'true') { - this.skip() - } else { - return test() - } - } -} - -module.exports = { - skipCoverage -}