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

Implement testing in Geth and Parity #186

Closed
wants to merge 39 commits into from

Conversation

cpurta
Copy link
Contributor

@cpurta cpurta commented Jan 14, 2018

Add script to allow for dockerized and local testing using a private development networks. Allows for specific clients to be used.

Will resolve #171

Currently only the tests using the Geth clients are working. The parity tests are hanging on send_transaction due to the client not running with the --geth flag. In order to use that flag an account will have to be unlocked.

I am looking into how accounts can be created and unlocked with the dev chain in the docker image provided by parity.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.865% when pulling 1129d78 on cpurta:geth-parity-testing into a3166b0 on aragon:dev.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.865% when pulling 2ae1d5d on cpurta:geth-parity-testing into a3166b0 on aragon:dev.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.865% when pulling 2c81793 on cpurta:geth-parity-testing into a3166b0 on aragon:dev.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.865% when pulling 4ac08a0 on cpurta:geth-parity-testing into a3166b0 on aragon:dev.

@izqui izqui self-requested a review January 14, 2018 09:51
@izqui izqui added this to the 0.5 code freeze milestone Jan 14, 2018
@@ -3,15 +3,20 @@
"version": "2.0.1",
"description": "Core contracts for Aragon",
"scripts": {
"test": "npm run ganache-cli:dev && truffle test --network rpc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these renamed back to testrpc? I intentionally renamed them after their rename when updating the dependencies on #169

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I now understand the rationale behind it. IMO we should rename the script and references to something other than testrpc as it is easy to think only the testrpc client is run. My suggestions would be test-nodes.sh, start-nodes.sh or manage-nodes.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Wasn't quite sure what to name that back to. Seems like the same file name should be applied in the aragon apps as to avoid confusion there as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree

Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

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

ups

.gitignore Outdated
@@ -11,3 +11,7 @@ scTopics
functions.csv
artifacts/
allFiredEvents

# geth and parity specific files
password
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these files needed to run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not needed I was just using that file to store passwords for accounts for geth/parity. I just didn't want to be committing any password files.

@izqui
Copy link
Contributor

izqui commented Jan 14, 2018

This looks amazing!

I had to modify the truffle config gas limit for the rpc network so it would be below the block gas limit in geth. Anyway, blocks are not sealing for some reason and tests are not running locally.

Travis build is failing as well, but exiting without an error code because the return code is for the clean up command https://travis-ci.org/aragon/aragon-core/jobs/328620235 Maybe we could make this run in after_success: in travis-ci

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.765% when pulling 60d5e55 on cpurta:geth-parity-testing into 8523b9e on aragon:dev.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.765% when pulling 4a75b94 on cpurta:geth-parity-testing into 8523b9e on aragon:dev.

@cpurta
Copy link
Contributor Author

cpurta commented Jan 16, 2018

Found that for geth if I reduce the gas in the truffle config I no longer the the Error: exceeds block gas limit but it appears that truffle does not like to play nicely with the dev chain of geth. Found that in this issue: trufflesuite/truffle#624.

I believe that this is the same issue with parity as well. When running truffle test --verbose-rpc --network rpc it is constantly making the eth_getFilterChanges and waiting for changes. Found another issue for that: trufflesuite/truffle#389.

So that presents two options.

  1. Attempt to fix truffle to avoid the data race issue mentioned in Truffle doesn't seem to detect mined transactions if insta-mining trufflesuite/truffle#624
  2. Tests will have to use a private development network both locally and in docker.

I am not opposed to attempting option 1 but that requires learning the truffle codebase and having to go through the PR process (unsure on timeline). If the issue is not being worked on already (does not look like it). It would be nice for truffle to work on the dev chain though.

Having a private network for all testing does not seem like a bad route to go and it allows for mocking test and main networks at some point without actually deploying to them. Setting up a private network for local test is a bit tedious to get all the nuances correct. Dockerizing a private network setup would more than likely mean having to manage and update the image(s) when new versions come out.

Thoughts on what is the best route to go would be helpful.

@izqui
Copy link
Contributor

izqui commented Jan 17, 2018

IMO it is not worth it to try to fix truffle, at least timeline wise. I would say let's go for route 2, either by configuring a testnet ourselves (tried before and never got the correct mining and gas settings to work, but definitely doable).

We can also try to modify geth slightly as @karalabe suggests in the truffle issue and slightly delay block sealing on dev mode. Unsure if there is a CLI setting we can tweak, or we can just modify the source code in geth and make that the docker image we depend on. I feel way more comfortable hacking around geth than truffle ;)

Chris Purta added 3 commits January 17, 2018 14:10
Added in generated account keys that use an empty password so that geth
can unlock those via a js script that is run. This script will unlock
all the accounts and start the mining process so that geth will be able
to accept transactions from truffle. Updated docker testing to use an
image that has the same accounts unlocked at run time and is mining. It
may be worth only allowing tests to be ran through docker.
@izqui
Copy link
Contributor

izqui commented Feb 22, 2018

Looking good. I think we should try to make the instant seal otherwise the CI is going to take forever.

Looks like the geth test is exceeding the maximum time limit for jobs.

How much time is taking? I run into this issue running coverage on Travis but solved it changing some variable in the .travis.yml, you can check travis docs for that.

The revert issue I hadn't thought of that, but that's a good way to test it, more similar to how the system will behave in production

@karalabe
Copy link

Why don't you run Geth in instant sealing mode? (--dev)

@cpurta
Copy link
Contributor Author

cpurta commented Feb 22, 2018

@karalabe I have done that and I run into the issue of there only being one account which then causes many test to fail due to Invalid number of arguments. I am looking into adding accounts on the dev chain and just transferring ether and unlocking them so that they can perform transactions. Do you happen to know of an easy way to do that?

@karalabe
Copy link

karalabe commented Feb 22, 2018

Something like?

acc := personal.newAccount("");
personal.unlockAccount(acc, "");
eth.sendTransaction({from: eth.accounts[0], to: acc, value: web3.toWei(1000, "ether")});

@cpurta
Copy link
Contributor Author

cpurta commented Feb 23, 2018

@karalabe that worked. Have all tests passing now.

@izqui
Copy link
Contributor

izqui commented Feb 23, 2018

That's amazing, thanks so much @cpurta and @karalabe

Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

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

This is absolutely fantastic. Tested locally and it is working perfectly. Just a couple of details and we will proceed to merge!

# pull the latest image using the dev test network
docker pull purta/geth-devnet:$GETH_VERSION
# run the geth dev network container
docker run -d -p 8545:8545 purta/geth-devnet:$GETH_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you have to create your own images? I think we should eventually move them under aragon's docker account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to make my own images so that all the accounts needed for testing could be set up correctly. For geth I ended up having to have a way to build images for a PoA network and the instant seal dev network with accounts being set up on the fly.

I agree that they should be moved eventually. The code for the geth images are here and the parity images are here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect. We should create a separate issue for this, as IMO is not a blocker at this point

Copy link
Contributor

Choose a reason for hiding this comment

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

Just published it to docker.io/aragon/geth-devnet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to use the new image using the aragon namespace, however I see that there is only a latest tag on docker hub. Do you want to use that tag or versioned tag like the one that is already being used (v1.7.3)?

@@ -23,10 +23,10 @@ const mocha = process.env.GAS_REPORTER ? mochaGasSettings : {}
module.exports = {
networks: {
rpc: {
network_id: 15,
network_id: 454545,
Copy link
Contributor

Choose a reason for hiding this comment

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

why this network id?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is still 15 on manage-nodes.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that was left over from me making a private PoA network in geth. I think that it can just stay at 15. Will revert that change.

return assertRevert(async () => {
await registry.newRepo('', repoDev, { from: apmOwner })
})
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we still run all of this inside a function like assertRevert(...) does?

We can expect that the function provided returns a Promise to a receipt, and after that we can check it:

async function assertThrows(codeBlock, message, errorCode) {
    try {
        const result = await codeBlock()
        assert.equal(result.receipt.status, 0, 'should have failed status')
    } catch (e) {
        return assertError(e, errorCode, message)
    }
    assert.fail('should have thrown before')
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that at first but the result that was returned was always undefined. That's why I went to using the try-catch blocks whenever the function was called in the tests. I will mess around with trying to see if I can get that logic reduced into a function.

@CLAassistant
Copy link

CLAassistant commented Feb 23, 2018

CLA assistant check
All committers have signed the CLA.

@cpurta
Copy link
Contributor Author

cpurta commented Feb 23, 2018

Addressed the comments, looks like CI failed the coverage test due to testrpc dying in the middle of the test.

@izqui
Copy link
Contributor

izqui commented Mar 1, 2018

This looks great.

@izqui izqui requested a review from sohkai March 1, 2018 14:32
# pull the latest image using the dev test network
docker pull purta/geth-devnet:$GETH_VERSION
# run the geth dev network container
docker run -d -p 8545:8545 purta/geth-devnet:$GETH_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Just published it to docker.io/aragon/geth-devnet

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Super late to this, but this is looking awesome @cpurta ⚡️!

A few notes below, mostly on smoothing the testing quirks from geth / parity.

"test": "npm run ganache-cli:dev && truffle test --network rpc",
"test": "npm run manage-nodes:dev && truffle test --network rpc",
"test:geth": "export GETH_CLIENT=geth; npm run manage-nodes:dev && truffle test --network rpc",
"test:parity": "export GETH_CLIENT=parity; npm run manage-nodes:dev && truffle test --network rpc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than export GETH_CLIENT=...; npm run ..., you could set the env var for the local command by GETH_CLIENT=... npm run ....

Might it also make sense to rename GETH_CLIENT to ETH_CLIENT?

if [ "$SOLIDITY_COVERAGE" = true ]; then
GETH_PORT=8555
else
GETH_PORT=8545
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also rename this to ETH_PORT?

# pull the most stable release of parity
docker pull purta/parity-instantseal:$PARITY_VERSION
# run the container in detached mode
docker run -d -p 8545:8545 purta/parity-instantseal:$PARITY_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the -p also use GETH_PORT?

@@ -62,19 +61,19 @@ contract('APMRegistry', accounts => {

it('fails to create empty repo name', async () => {
return assertRevert(async () => {
await registry.newRepo('', repoDev, { from: apmOwner })
return await registry.newRepo('', repoDev, { from: apmOwner })
Copy link
Contributor

@sohkai sohkai Mar 26, 2018

Choose a reason for hiding this comment

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

Since all of our assert* helpers are async themselves, these inner functions can just return the promise (if we're still using the return values, see below), e.g.

return assertRevert(() => {
  return registry.newRepo('', repoDev, { from: apmOwner })
})

Copy link
Contributor Author

@cpurta cpurta Apr 4, 2018

Choose a reason for hiding this comment

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

Seems like if we go this route then we will have to add in the logic to handle the 0 status everywhere that we call assertRevert. So all test that do not have to deal will odd truffle/geth test differences (base 16 number).

It seems like these tests would then have to use a wrapper to check that the receipt status is 0 and throw the error:

const checkRecieptStatus = code => {
    let result = await code()
    if (result.receipt.status == 0) { throw new Error('mock revert') }
}

Which makes the test look something like:

return assertRevert(async () => {
    checkRecieptStatus(() => {
        return await registry.newRepo('', repoDev, { from: apmOwner })
    })
})

} else {
return assert.isAbove(e.message.search('revert'), -1, 'should have failed with revert')
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat confused by this—geth and parity don't fail on this but rather, return values? The error that occurs (e.message.search('base 16 number')) is from web3 failing to convert the return value?

Sounds nuts. What repo version do they return?

In any case, it might be nice to wrap this so we can still use the generic assertRevert:

return assertRevert(async () => {
  try {
    await repo.getByVersionId(0)
  } catch (e) {
    if (e.message.search('base 16 number') > -1) {
      // geth and parity will return values for the repo version which web3 chokes on
      // and determines that they cannot be converted to uint16 so we need to catch this error
        throw new Error('mock revert')
      } else {
        throw e
      }
    }
  }
})

if (result.receipt) {
assert.equal(result.receipt.status, 0, 'should have failed status')
} else {
assert.isFalse(result, 'should not have permission')
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks nuts—geth or parity return a result here rather than reverting?

If so, it might also be nice to have a generic wrapper for this (I notice there's another instance of this later), using a mock revert error:

const assertRevertOrFalse = (code, falseMsg) => {
  return assertRevert(async () => {
    const result = await code()
    if (!result) {
      throw new Error('mock revert')
    }
  })
}

} catch (e) {
if (codeDeploymentError(e)) { return true }
assert.isAbove(e.message.search('revert'), -1, 'should have failed with revert')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to hoist this up as a assertCodeDeploymentErrorOrRevert:

const assertCodeDeploymentErrorOrRevert = code => {
  return assertRevert(async () => {
    try {
      await code()
    } catch (e) {
      if (codeDeploymentError(e)) { throw new Error('mock revert') }
      else { throw e }
    }
  })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By "hoist" do you mean add this as function in helpers/assertThrow.js?

@@ -4,7 +4,8 @@ function assertError(error, s, message) {

async function assertThrows(codeBlock, message, errorCode) {
try {
await codeBlock()
const result = await codeBlock()
return assert.equal(result.receipt.status, 0, 'should have failed status')
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure I like this; the assert.fail('should have thrown before') at the end should fail any test that doesn't throw an error.

Not having this bit also lets us not worry about what's being returned from the codeBlock().

@cpurta
Copy link
Contributor Author

cpurta commented Apr 12, 2018

@sohkai, replied to some of your comments regarding the error checking. Some feedback on that would be great to get this wrapped up.

@sohkai
Copy link
Contributor

sohkai commented Apr 18, 2018

Hey @cpurta, I've been meaning to get back to looking at this in the last week but have a few other tasks prioritized. I'm really sorry that I've left you hanging (and missed the comments two weeks ago); but know that this is nagging me too (and I hope to get back to it once those other things are done!).

@cpurta
Copy link
Contributor Author

cpurta commented Jun 11, 2018

@sohkai, would appreciate some feedback on the comments. Will have to do quite a bit of updating to this PR due to it being a bit outdated but looking to get this one closed soon.

@sohkai
Copy link
Contributor

sohkai commented Jul 10, 2018

@cpurta Really, really sorry about leaving you hanging. We've continually deprioritized this so I haven't had a chance to really dig back in :(.

Given the current priorities, I doubt we'll be able to look at this until at least the end of July... There's a lot of context required for this and I need some time to get up to speed with all the differences.

Something I noticed with ganache-cli a while back was the ability to disable its "easy" error reporting mode to be more similar to real clients (see the "Important note about contract runtime error reporting" section). Maybe if we enforce this in the testing script, we don't have to use separate logic between the different clients?

@sohkai sohkai modified the milestones: aragonOS 3.1, aragonOS 5.0 Aug 22, 2018
@sohkai sohkai self-assigned this Aug 30, 2018
@sohkai sohkai changed the base branch from dev to next July 11, 2019 09:30
@sohkai sohkai closed this Mar 17, 2020
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.

Run tests in geth and parity
7 participants