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

Update testing to use geth and parity clients #66

Closed
wants to merge 10 commits into from

Conversation

cpurta
Copy link
Contributor

@cpurta cpurta commented Jan 10, 2018

Allow for coverage tests to use either a running geth instance or parity instance. This allows for more robust testing against private development networks.

Will resolve #64 when completed.

All apps are currently configured to use the development network. Based on the truffle-config.js in the aragon-core repo it will cause tests to use the ganache-cli testrpc. We can then later update that config to have the development network point to a specific instance of geth or parity preferably locally.

@cpurta
Copy link
Contributor Author

cpurta commented Jan 10, 2018

Just as a note seems like it may be worth adding in account creation and password file creation if geth/parity have no accounts set up. Default dev account passwords would just be empty (or default to "password") to allow for simple password file creation.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.575% when pulling 17eafe3 on cpurta:geth-parity-testing into 0df6ab0 on aragon:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.575% when pulling f9fc16d on cpurta:geth-parity-testing into 0df6ab0 on aragon:master.

@izqui izqui self-requested a review January 10, 2018 20:04
@izqui
Copy link
Contributor

izqui commented Jan 10, 2018

Woah! This looks really promising! Thanks so much for taking the time :)

  • Regarding only using the first account, I think there are some tests that require multiple accounts mostly to test access control.
  • I think it would be important to make this tests be run in the CI, for that we need to either install both geth and parity directly or use Docker images for them so we can properly pin the versions.

@cpurta
Copy link
Contributor Author

cpurta commented Jan 10, 2018

Yeah that would make sense. I agree that it would be really important to add this to CI. Was thinking along the lines of use docker since it would be much easier to configure at run time. Going to be looking at some published images or perhaps building some and pushing those to docker hub.

@izqui
Copy link
Contributor

izqui commented Jan 10, 2018

I have used official parity’s images in the past and work great. I think geth has images too.

Regarding the CI, I don’t think we need to run coverage on parity and geth. We can do just a test with parity and geth and then coverage on the forked version of testrpc of solidity coverage.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.575% when pulling 573e343 on cpurta:geth-parity-testing into 0c7f8b4 on aragon:master.

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 looks so good! Let me know when it can be tested :)

@@ -3,7 +3,9 @@
"version": "1.0.0",
"main": "index.js",
"scripts": {
"test": "truffle test",
"test": "truffle test --network development",
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to run tests on Fundraising for now

docker_start_parity() {
check_docker
# pull the most stable release of parity
docker pull parity/parity:stable-release --chain dev
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 we should pin to specific geth and parity versions!

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. Are you thinking about just using the most recent versions for right now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of this is to avoid random CI failures because a geth or parity update happened.

@cpurta
Copy link
Contributor Author

cpurta commented Jan 11, 2018

The current geth image that I am using only has a latest tag. Checking some other images for geth seems to have "outdated" tags arachnid/geth has v1.4.20 as the only tagged version. Also based on the recent push it looks like it's being built along side the other tags. Most other images are a minor version back <=1.6.*. Are we ok with keeping the latest tag for geth?

Also is docker enabled on travis CI? If so I will update the CI file to use the dockerized tests and see if everything has worked.

@cpurta cpurta mentioned this pull request Jan 11, 2018
@izqui
Copy link
Contributor

izqui commented Jan 12, 2018

The .travis.yml file needs that to enable docker:

sudo: required
services:
  - docker

Chris Purta added 3 commits January 12, 2018 12:17
Added the dockerized tests to the travis CI file (More than likely
going to fail all tests due to gas limit on dev net). Pinned the docker
clients to use specific versions to limit the breaking of pipelines when
the images are updated.
@cpurta cpurta changed the title WIP: Update testing to use geth and parity clients Update testing to use geth and parity clients Jan 22, 2018
@sohkai sohkai removed this from the 0.5 code freeze milestone Aug 30, 2018
@sohkai sohkai self-assigned this Aug 30, 2018
bingen pushed a commit to bingen/aragon-apps that referenced this pull request Dec 17, 2018
@izqui
Copy link
Contributor

izqui commented Feb 8, 2019

I think we should really do this at some point @sohkai @bingen

@Evalir
Copy link
Contributor

Evalir commented Mar 17, 2020

Should we aim to do this or just deprioritize and close? @sohkai @bingen

@sohkai
Copy link
Contributor

sohkai commented Mar 17, 2020

@cpurta Sorry for dragging this on forever; we never could find time to prioritize this given other efforts.

We're consolidating our test tooling and etc. to be more robust against real/ganache chains, so hopefully adding tests against a real node in the future will be much, much easier than where we were in 2018. For now though, let's consider these contracts to be well tested given their reliability on mainnet over the last year and a half :).

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run tests in geth and parity
5 participants