Skip to content

Testing improvements#542

Open
dapplion wants to merge 9 commits into
aragon:nextfrom
dapplion:514_testing_improvements
Open

Testing improvements#542
dapplion wants to merge 9 commits into
aragon:nextfrom
dapplion:514_testing_improvements

Conversation

@dapplion
Copy link
Copy Markdown

@dapplion dapplion commented Jul 13, 2019 •
edited
Loading

Following the request on the "good first issue" #514, I am extending the tests covering the following items:

  • Explicitly test AppProxyFactory. newAppProxyPinned(IKernel, bytes32)
  • Explicitly test EVMScriptRunner.protectState modifier
  • Test view methods of SafeERC20.sol

NOTE: This PR is a WIP until further questions are resolved.

@welcome
Copy link
Copy Markdown

welcome Bot commented Jul 13, 2019

Thanks for opening this pull request! Someone will review it soon 🔍

@auto-assign auto-assign Bot requested review from facuspagnuolo, izqui and sohkai July 13, 2019 20:38
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jul 13, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 13, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.9%) to 100.0% when pulling 6d84a85 on dapplion:514_testing_improvements into 0fd1ff6 on aragon:next.

@dapplion dapplion force-pushed the 514_testing_improvements branch from ecc5354 to 0122188 Compare July 13, 2019 21:44
@dapplion dapplion mentioned this pull request Jul 13, 2019
Open
facuspagnuolo
facuspagnuolo reviewed Jul 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good @dapplion ! Looking forward for the other tests :)

@dapplion dapplion force-pushed the 514_testing_improvements branch from 4de1ea2 to 272ab16 Compare August 19, 2019 20:07
@dapplion dapplion force-pushed the 514_testing_improvements branch from 272ab16 to d520e20 Compare August 19, 2019 20:09
@dapplion dapplion force-pushed the 514_testing_improvements branch from 3a50d24 to 85fc5b5 Compare August 31, 2019 11:59
@dapplion
Copy link
Copy Markdown
Author

dapplion commented Aug 31, 2019

@sohkai The Travis build on my fork's branch completes the coverage step successfully https://travis-ci.com/dapplion/aragonOS/builds/125316450. However, on your repo it doesn't, where it errors with:

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? With this last commit, coverage gets to 100.00% :)

@dapplion
Copy link
Copy Markdown
Author

dapplion commented Sep 17, 2019

@cgewecke helped solve the problem and now of CI tasks complete successfully

@sohkai sohkai added this to the aragonOS 5.0 milestone Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@izqui izqui Awaiting requested review from izqui

@sohkai sohkai Awaiting requested review from sohkai

1 more reviewer

@facuspagnuolo facuspagnuolo facuspagnuolo left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

aragonOS 5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@dapplion @CLAassistant @coveralls @facuspagnuolo @sohkai @cgewecke