Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Solidity test coverage #205

Closed
wants to merge 16 commits into from
Closed

Solidity test coverage #205

wants to merge 16 commits into from

Conversation

liamzebedee
Copy link
Contributor

@liamzebedee liamzebedee commented Jul 17, 2019

Summary. Generates test coverage report for our Solidity contracts, using solidity-coverage. Closes #272. Some screenshots below

Screen Shot 2019-07-17 at 2 28 30 pm

Screen Shot 2019-07-17 at 2 28 23 pm

Screen Shot 2019-07-17 at 2 28 19 pm

Instructions

  1. npm i
  2. npm run coverage
  3. open coverage/index.html

@@ -9,7 +9,9 @@
"js:lint": "eslint ${npm_package_config_eslintPaths}",
"js:lint:fix": "eslint --fix ${npm_package_config_eslintPaths}",
"sol:lint": "solium -d contracts/",
"sol:lint:fix": "solium -d contracts/ --fix"
"sol:lint:fix": "solium -d contracts/ --fix",
"remix": "remixd -s . --remix-ide https://remix.ethereum.org",
Copy link
Member

Choose a reason for hiding this comment

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

What's that? Is remixd installed as a dependency? Do we need this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh woops, will remove that.

@@ -52,6 +52,14 @@ module.exports = {
gasPrice: 1
},

coverage: {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add docs why do we need this in the truffle config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

@@ -43,6 +43,12 @@ jobs:
name: Run NPM tests
working_directory: ~/project/implementation/contracts
command: npm install && npm run setup && npm run test
- run:
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract it from test_solidity to a separate job? If we do we will also need to at it to a workflows at the bottom of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good idea!

@liamzebedee
Copy link
Contributor Author

Cool, coverage job is working. You can view an uploaded test report by navigating the artifacts tab. An example - https://3627-171933613-gh.circle-artifacts.com/0/home/circleci/project/implementation/coverage/index.html

@mhluongo
Copy link
Member

@liamzebedee can we either get this to not break the build and merge in master (ideal) or close?

You can likely have this generating the coverage artifact without less-than-full coverage failing the test

@liamzebedee
Copy link
Contributor Author

"less-than-full coverage" that's a good way of saying it's broken 😂

solidity-coverage appears to be broken for the interface contracts. Might be something to do with the upgrade to 0.5.0 (#213).

There was a problem instrumenting ./coverageEnv/contracts/interfaces/IKeep.sol: TypeError: Cannot read property 'stop' of null

Closing this for now until there's a better idea 😢

@liamzebedee liamzebedee closed this Sep 2, 2019
@prestwich
Copy link
Contributor

@liamzebedee what specifically was breaking? you can force solidity-coverage to ignore specific files, e.g. interfaces

@liamzebedee liamzebedee added the 🐛 bug Something isn't working label Sep 2, 2019
@liamzebedee
Copy link
Contributor Author

liamzebedee commented Sep 2, 2019

  • IERC721 but that's because it's duplicated (we've also got OpenZep versions)
  • IKeep
  • IPriceOracle

@liamzebedee liamzebedee reopened this Sep 2, 2019
@liamzebedee liamzebedee removed the 🐛 bug Something isn't working label Sep 2, 2019
@liamzebedee
Copy link
Contributor Author

Will re-open this, got a bit further just now with ignoring the files

@prestwich
Copy link
Contributor

.solcover.js

module.exports = {
    skipFiles: ['whatever here']
};

@liamzebedee
Copy link
Contributor Author

Yeah this is the current config I have:

module.exports = {
    skipFiles: ['interfaces/IKeep.sol','interfaces/IPriceOracle.sol','interfaces/IERC721.sol','interfaces/ITBTCSystem.sol','interfaces/KeepBridge.sol'],
    copyPackages: ['openzeppelin-solidity', 'bitcoin-spv'],
}

But now getting this error sc-forks/solidity-coverage#245 - for tomorrow.

@Shadowfiend
Copy link
Contributor

We need to capture this in an issue rather than pull a PR into the sprint board. Incidentally that issue can explain reasoning, describe how we might make it accessible, how it would fit into any workflow, etc.

Simply having test coverage available isn't really useful unless we know how it fits in with the workflow.

@liamzebedee
Copy link
Contributor Author

I have it running on my local, but only when installing deps using yarn. Since solidity-coverage only has a Yarn lockfile, and not package-lock, it's likely there's a subdep that has changed.

In either case, running it on CircleCI, and now there's a bizarre error about Node fibers. This is taking too long, gonna park it here for now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Solidity test coverage
5 participants