Testing improvements (#513)

* test: add tests for non-existent executors * test: add explicit lifecycle tests for duplicate initializations * test: fail solidity tests if they made no assertions
aragon · Apr 23, 2019 · 635dd96 · 635dd96
1 parent 1b67d10
commit 635dd96
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 11 deletions.
diff --git a/contracts/test/TestDelegateProxy.sol b/contracts/test/TestDelegateProxy.sol
@@ -69,6 +69,9 @@ contract TestDelegateProxy is DelegateProxy {
 
     // keep as last test as it will kill this contract
     function testDieIfMinReturn0() public {
+        Assert.isTrue(true, ''); // Make at least one assertion to satisfy the runner
+
         delegatedFwd(target, target.die.selector.toBytes());
+        Assert.fail('should be dead');
     }
 }
diff --git a/test/evm_script.js b/test/evm_script.js
@@ -160,16 +160,35 @@ contract('EVM Script', accounts => {
         await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_ENABLED)
       })
 
+      it('fails to disable an already disabled executor', async () => {
+        await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss })
+        await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss })
+
+        await assertRevert(evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_DISABLED)
+      })
+    })
+
+    context('> Non-existing executor', () => {
       it('fails to enable a non-existent executor', async () => {
         await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss })
-        await assertRevert(evmScriptReg.enableScriptExecutor(installedExecutorId + 1, { from: boss }), reverts.EVMREG_INEXISTENT_EXECUTOR)
+
+        // 0 is reserved
+        await assertRevert(evmScriptReg.enableScriptExecutor(0, { from: boss }), reverts.EVMREG_INEXISTENT_EXECUTOR)
+
+        // No executors should be installed yet
+        assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(1)), ZERO_ADDR, 'No executors should be installed yet')
+        await assertRevert(evmScriptReg.enableScriptExecutor(1, { from: boss }), reverts.EVMREG_INEXISTENT_EXECUTOR)
       })
 
-      it('fails to disable an already disabled executor', async () => {
+      it('fails to disable a non-existent executor', async () => {
         await acl.createPermission(boss, evmScriptReg.address, REGISTRY_MANAGER_ROLE, boss, { from: boss })
-        await evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss })
 
-        await assertRevert(evmScriptReg.disableScriptExecutor(installedExecutorId, { from: boss }), reverts.EVMREG_EXECUTOR_DISABLED)
+        assert.equal(await evmScriptReg.getScriptExecutor(createExecutorId(1)), ZERO_ADDR, 'No executors should be installed yet')
+        await assertRevert(
+          evmScriptReg.disableScriptExecutor(1, { from: boss }),
+          // On disable only an enable check is performed as it doubles as an existence check
+          reverts.EVMREG_EXECUTOR_DISABLED
+        )
       })
     })
   })

diff --git a/test/helpers/runSolidityTest.js b/test/helpers/runSolidityTest.js
@@ -27,7 +27,7 @@ const HOOKS_MAP = {
   afterAll: 'afterAll',
 }
 
-const processResult = txReceipt => {
+const processResult = (txReceipt, mustAssert) => {
   if (!txReceipt || !txReceipt.receipt) {
     return
   }
@@ -37,6 +37,9 @@ const processResult = txReceipt => {
       throw new Error(log.args.message)
     }
   })
+  if (mustAssert && !decodedLogs.length) {
+    throw new Error('No assertions made')
+  }
 }
 
 /*
@@ -81,13 +84,15 @@ function runSolidityTest(c, mochaContext) {
         if (interface.type === 'function') {
           if (['beforeAll', 'beforeEach', 'afterEach', 'afterAll'].includes(interface.name)) {
             // Set up hooks
-            global[HOOKS_MAP[interface.name]](() => {
-              return deployed[interface.name]().then(processResult)
-            })
+            global[HOOKS_MAP[interface.name]](() =>
+              deployed[interface.name]()
+                .then(receipt => processResult(receipt, false))
+            )
           } else if (interface.name.startsWith('test')) {
-            it(interface.name, () => {
-              return deployed[interface.name]().then(processResult)
-            })
+            it(interface.name, () =>
+              deployed[interface.name]()
+                .then(receipt => processResult(receipt, true))
+            )
           }
         }
       })

diff --git a/test/lifecycle.js b/test/lifecycle.js
@@ -1,5 +1,6 @@
 const { assertRevert } = require('./helpers/assertThrow')
 const { getBlockNumber } = require('./helpers/web3')
+const reverts = require('./helpers/revertStrings')
 
 // Mocks
 const LifecycleMock = artifacts.require('LifecycleMock')
@@ -35,6 +36,14 @@ contract('Lifecycle', accounts => {
     it('has correct initialization block', async () => {
       assert.equal(await lifecycle.getInitializationBlock(), await getBlockNumber(), 'initialization block should be correct')
     })
+
+    it('cannot be re-initialized', async () => {
+      assertRevert(lifecycle.initializeMock(), reverts.INIT_ALREADY_INITIALIZED)
+    })
+
+    it('cannot be petrified', async () => {
+      assertRevert(lifecycle.petrifyMock(), reverts.INIT_ALREADY_INITIALIZED)
+    })
   })
 
   context('> Petrified', () => {
@@ -50,6 +59,10 @@ contract('Lifecycle', accounts => {
       assert.isTrue(await lifecycle.isPetrified(), 'should be petrified')
     })
 
+    it('cannot be petrified again', async () => {
+      assertRevert(lifecycle.petrifyMock(), reverts.INIT_ALREADY_INITIALIZED)
+    })
+
     it('has initialization block in the future', async () => {
       const petrifiedBlock = await lifecycle.getInitializationBlock()
       const blockNumber = await getBlockNumber()