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

Add E2E tests for geth and ganache dev clients #3122

Merged
merged 8 commits into from
Oct 15, 2019
Merged

Add E2E tests for geth and ganache dev clients #3122

merged 8 commits into from
Oct 15, 2019

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Oct 11, 2019

#2682 (original) and part of #3098 (E2E Proposal)

  • Adds shell scripts for testing with geth and ganache development nodes (locally & CI)
  • Combines coverage/coveralls measurement for E2E and unit tests in the Node 10 job.
  • Adds 25-30 tests covering simple deployments, promievents and transaction signing
  • Adds some small mock contracts and their compilation artifacts in test/sources

The tests run across these combinations.....

Client instamine automine 2s
geth --dev (http) (http, ws)
ganache-cli (http, ws)

Things discovered by the test suite:

  • confirmation handler over ws for ganache on deployment does not fire.
    • [Edit: ... was a bug in the tests].
  • confirmation handler over ws fires each second for geth automine
    • [Edit: ... now believe this works correctly, have added assertions to test]
  • ws for geth instamine is very flaky - possible race condition somewhere.

Tried to make this as simple as possible. I think the CI part will fit into 2.x without too many problems. It's just some small shell scripts.

I used this geth-dev-assistant wrapper to launch geth - it's small, all the APIs are exposed and you can pull recent images with it. I would be into publishing something like this in the web3-js org so anyone with permissions there can edit it as they wish and make necessary improvements.

To do in future....

  • Will add Parity in a subsequent PR after this is reviewed and any required changes are made.
  • More tests...it would be nice to get coverage up to 90% in 1.x ( like in 2.x )

.travis.yml Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 11, 2019

Coverage Status

Coverage increased (+1.7%) to 84.651% when pulling ca45a8f on cgewecke:tests/e2e-nodes into 4178371 on ethereum:1.x.

@nivida nivida added 1.x 1.0 related issues Enhancement Includes improvements or optimizations labels Oct 11, 2019
@nivida nivida self-requested a review October 11, 2019 06:44
Copy link
Contributor

@nivida nivida left a comment

Choose a reason for hiding this comment

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

Looks good to me! I only dropped some comments to have a more consistency code style.

test/e2e.method.signing.js Outdated Show resolved Hide resolved
test/e2e.method.signing.js Show resolved Hide resolved
test/e2e.method.signing.js Show resolved Hide resolved
test/e2e.method.signing.js Outdated Show resolved Hide resolved
test/e2e.method.send.js Show resolved Hide resolved
test/e2e.contract.deploy.js Outdated Show resolved Hide resolved
test/e2e.contract.deploy.js Outdated Show resolved Hide resolved
test/e2e.contract.deploy.js Outdated Show resolved Hide resolved
test/e2e.contract.deploy.js Outdated Show resolved Hide resolved
test/e2e.method.send.js Show resolved Hide resolved
@nivida nivida added the In Progress Currently being worked on label Oct 13, 2019
@holgerd77
Copy link
Collaborator

This looks really like a great start, cool! 😄 How long is the additional test/CI run time with this (just out of interest)?

@cgewecke
Copy link
Collaborator Author

@holgerd77 The existing (non-e2e) tests are really fast.. >2500 tests in 40s.

E2E time for only ~30 tests looks like this (including client launch):

Client Time
geth instamine (w/ skipped ws tests) 26s
ganache instamine 13s
geth automine 2s block interval 1m50s

So...it's slower.

To me it seems like the tests worth writing for E2E are mostly about checking that this complex transaction sending logic executes correctly for both instamine and longer duration blocks. And maybe also that the various client responses are formatted as expected.

@holgerd77
Copy link
Collaborator

@cgewecke Ah, thanks, that all sounds pretty fast to me, I am more used to count test time in minutes than in seconds, lol. 😛

@cgewecke cgewecke assigned cgewecke and unassigned cgewecke Oct 14, 2019
test/e2e.contract.deploy.js Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Added some simple events tests

test/e2e.method.send.js Show resolved Hide resolved
@cgewecke
Copy link
Collaborator Author

@nivida Have updated - this ready for another look-over...

(Rebased against 1.x to fix a package-lock.json merge conflict)

@nivida nivida removed the In Progress Currently being worked on label Oct 15, 2019
@nivida nivida merged commit 571f374 into web3:1.x Oct 15, 2019
nachomazzara pushed a commit to nachomazzara/web3.js that referenced this pull request Jun 4, 2020
* Add E2E tests for geth and ganache dev clients

* Replace missing run build step in CI

* Fix bug in deploy confirmation test (ganache)

* Add assertion to confirmation handler tests

* Fix formatting / remove ethereumjs-tx test

* Add simple events tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Enhancement Includes improvements or optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants