Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Upgrade solidity-coverage to 0.7.0-beta #716

Closed

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Sep 23, 2019

This is a preliminary PR documenting some of the changes that would be necessary to get the new version of solidity-coverage running here. Includes:

  • solidity-coverage bumped to 0.7.0-beta.2
  • truffle to 5.0.37
  • some small changes I will walk through in the diff below :)

0.7.0 ships as a plugin and its design requires that it launch ganache itself. You can run any ganache version by specifying it as a client option in solcover.js (this PR uses the default one bundled with the latest Truffle.) ex:

client: require('ganache-cli')

NB: my initial effort to get this passing timed out on the Reputation Mining portion of the tests. (It didn't run out of memory though.) Zeppelin's tests are ~25% slower with coverage than without - interested to see if that holds true here as well.

On a side note - was looking at aragon-court's test suite last week and they have a comparably industrial test suite that they've recently decided to break up into separate CI jobs. You can specify a subset of tests with the new coverage by writing something like:

truffle run coverage file="test/extensions/*.js

...and there's a way of aggregating the coverage reports at Coveralls using their parallel builds service

[EDIT - is it possible CI needs to be enabled for forks?]

const log = console.log;

// Copies pre-built token artifacts to .coverage_artifacts/contracts
function provisionTokenContracts(config){
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This token provisioning is now done in a post-compilation hook executed by the Truffle plugin. You're also usually compiling the DappSys contracts during this step but Truffle kept reporting there was nothing to compile there, so I just skipped that. But if it's necessary it would go in this function...

testCommand: '../node_modules/.bin/truffle test --network coverage',
testrpcOptions: `--port 8555 -i 1999 --acctKeys="./coverageEnv/ganache-accounts.json" --noVMErrorsOnRPCResponse --accounts 12 --allowUnlimitedContractSize`
};
providerOptions: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all the flags passed to testrc-sc, translated to ganache-core options and passed to ganache.

@@ -39,7 +39,7 @@ export async function setupEtherRouter(interfaceContract, deployedImplementation
const fName = value.name;
const fType = value.type;
// These are from DSAuth, and so are on EtherRouter itself without any more help.
if (fName !== "authority" && fName !== "owner") {
if (fName !== "authority" && fName !== "owner" && !fName.includes("coverage_")) {
Copy link
Collaborator Author

@cgewecke cgewecke Sep 23, 2019

Choose a reason for hiding this comment

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

Instrumentation is now done using an injected public function (instead of an event) and it needs to be excluded here.

@@ -30,7 +30,7 @@
"test:contracts:upgrade:2to3": "npm run start:blockchain:client:2to3 & truffle test ./test-upgrade/2to3/2to3.js",
"test:contracts:gasCosts": "npm run start:blockchain:client & truffle migrate --reset --compile-all && truffle test test-gas-costs/gasCosts.js --network development",
"test:contracts:patricia": "npm run start:blockchain:client parity & truffle migrate --reset --compile-all && truffle test packages/reputation-miner/patricia-test.js --network development",
"test:contracts:coverage": "SOLIDITY_COVERAGE=1 solidity-coverage && istanbul check-coverage --statements 99 --branches 94 --functions 99 --lines 99",
"test:contracts:coverage": "SOLIDITY_COVERAGE=1 truffle run coverage --network coverage --temp build && istanbul check-coverage --statements 99 --branches 94 --functions 99 --lines 99",
Copy link
Collaborator Author

@cgewecke cgewecke Sep 23, 2019

Choose a reason for hiding this comment

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

There something important here: --temp build means: "write the temporary coverage compilation artifacts to build". It treats that folder as disposable and will delete it after coverage runs. Normally we would use a folder called .coverage_artifacts but there are many places in the test setup scripts where the artifacts are explicitly requested from build.

In <= 0.6.x the whole project gets copied into a sub-folder. That works well here but has been the source of quite a bit of weirdness elsewhere :) In the plugin version, the contracts/artifacts are written to temp folders at the root and truffle's path variables are re-targeted to use them as sources for compilation and testing.

@@ -92,7 +92,6 @@ export async function setupUpgradableColonyNetwork(
deployedImplementations.ContractRecovery = contractRecovery.address;

await setupEtherRouter("IColonyNetwork", deployedImplementations, resolver);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a mistake.

@kronosapiens
Copy link
Member

Thank you @cgewecke !! We'll take a look in more detail soon :)

@cgewecke
Copy link
Collaborator Author

cgewecke commented Sep 26, 2019

Just leaving an additional note here re: parallelization. It looks like CircleCI supports this within individual job as well. See Running Tests In Parallel.

I would like (at some point) to have an example/recipe for that in the solidity-coverage docs (e.g how to split a job up and then unify the output for coveralls). May investigate in the next couple weeks in my own fork and push results here if successful...

@area
Copy link
Member

area commented Oct 11, 2019

Moved to #726

@area area closed this Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants