Testing improvements

[sohkai]

A few items left from the test suite advisory:

• Explicitly test AppProxyFactory. newAppProxyPinned(IKernel, bytes32)
• Explicitly test EVMScriptRunner.protectState modifier
• Explicitly test auth modifiers in apps (APMRegistry, Repo, EVMScriptRegistry)

[dapplion]

@sohkai I have reviewed the items that you proposed to test and I have a few doubts:

• Regarding AppProxyFactory.newPinnedAppProxy(IKernel, bytes32) are you referring to newAppProxyPinned?
  

  aragonOS/contracts/factory/AppProxyFactory.sol

  Line 38 in d10d195

  function newAppProxyPinned(IKernel _kernel, bytes32 _appId) public returns (AppProxyPinned) {

• Regarding EVMScriptRunner.protectState, since DelegateScript and DeployDelegateScript executors removed, is it possible to change the kernel address without a malicious executor?
• Could you provide an example of an existing test that explicitly tests a modifier as a reference?

[sohkai]

@dapplion

Regarding AppProxyFactory.newPinnedAppProxy(IKernel, bytes32) are you referring to newAppProxyPinned?

Oops, yes! I've edited the issue :).

Regarding EVMScriptRunner.protectState, since DelegateScript and DeployDelegateScript executors removed, is it possible to change the kernel address without a malicious executor?

Not with the current set of executors, but this is something that may be possible if alternative executors get installed.

Could you provide an example of an existing test that explicitly tests a modifier as a reference?

https://github.com/aragon/aragonOS/blob/dev/test/contracts/apps/app_acl.js is a generic test against the ACL's auth modifier, but we should also add explicit tests for those named AragonApps to make sure their authentication works as expected (especially with parameters).

[dapplion]

Thank you so much for your answer @sohkai. I have opened a PR #542 testing item 2 EVMScriptRunner.protectState. I still have doubts regarding the other items:

• AppProxyFactory.newPinnedAppProxy(IKernel, bytes32) is never called by the kernel or any contract, in contrast to AppProxyFactory.newAppProxyPinned(IKernel, bytes32, bytes)

  aragonOS/contracts/kernel/Kernel.sol

  Line 131 in c85d34e

  appProxy = newAppProxyPinned(this, _appId, _initializePayload);

  . The intended usage of this function is to be called directly by a user? If so, should the test just call it externally anyway without the extra steps done in the Kernel.newPinnedAppInstance method?
• The three mentioned contracts APMRegistry, Repo and EVMScriptRegistry permissions are tested extensively in their respective test suites. I don't fully understand if you mean that those test should be extended, or do another suite of test with stubs similar to the example you provided.

[sohkai]

First of all, thanks for contributing to the tests! ❤️

AppProxyFactory.newPinnedAppProxy(IKernel, bytes32) is never called by the kernel or any contract, in contrast to AppProxyFactory.newAppProxyPinned(IKernel, bytes32, bytes)

I'd say we could add a completely new test file, test/contracts/factory/app_proxy_factory.js, and explicitly test all functionality in AppProxyFactory by itself.

The fact that all of the functions are public is indeed a bit of an abstraction leak we didn't catch onto, and they should've been marked internal instead (too late now to change this, for aragonOS 4).

The three mentioned contracts APMRegistry, Repo and EVMScriptRegistry permissions are tested extensively in their respective test suites. I don't fully understand if you mean that those test should be extended, or do another suite of test with stubs similar to the example you provided.

They are meant to be extended to test the permissions parameters themselves, but setting this up is a bit difficult without better tooling for ACL params, so I wouldn't worry about this one too much :).

[dapplion]

@sohkai Thanks for the recommendation. I have tested

• newAppProxy(IKernel _kernel, bytes32 _appId) and
• newAppProxyPinned(IKernel _kernel, bytes32 _appId)

in a new file covering the last remaining untested line. Let me know if this issue would require further actions or we should skip the auth modifiers explicit tests for now.

[dapplion]

According to coveralls, the only file with remaining lines/branches to be tested is SafeERC20.sol I see you discussing coverage decrease in #541, may it be that SafeERC20.sol should be ignored for coverage?

[sohkai]

Ahhh good point! It was once ignored in coverage due to solidity-coverage not supporting some assembly opcodes, but this has been fixed and we should increase the coverage of SafeERC20 too!

[dapplion]

@sohkai I have written extra test to cover SafeERC20 and get coverage to 100.00%. However, the Travis build on my fork's branch completes the coverage step successfully https://travis-ci.com/dapplion/aragonOS/builds/125316450 but on Aragon's repo it doesn't. The error is:

Writing artifacts to ./build/contracts
Instrumenting  ./coverageEnv/contracts/acl/ACL.sol
Skipping instrumentation of  ./coverageEnv/contracts/acl/ACLSyntaxSugar.sol
Instrumenting  ./coverageEnv/contracts/acl/IACL.sol
Cleaning up...
There was a problem instrumenting ./coverageEnv/contracts/acl/IACL.sol: TypeError: Cannot read property 'stop' of null
Exiting without generating coverage...
npm ERR! code ELIFECYCLE

Do you have any clue on what might be the issue?

[cgewecke]

@dapplion It's a bug coming from solidity-parser-antlr which should be fixed in solidity-coverage 0.6.5. Maybe a lower version is cached here?

[dapplion]

@cgewecke Bumped the version of solidity-coverage to 0.6.7 and fixed the problem. Thank you for your insight!