Refactor: Use exvcr to record eth/rootchain network requests #1037

achiurizo · 2019-10-07T19:36:59Z

TODO

Fix skip test for has_token/2 due to ald contract function change
Fix skip test for get_deposits due to invalid output encoding
Fix CircleCI where passing in --umbrella breaks the exvcr usage.

Overview

This PR introduces exvcr to record the response we get from the Ethereum client for testing with OMG.Eth, OMG.Eth.RootChain and the rest of its ilk. This will record our tests and allows us to turn down the service and test against those responses instead of spinning up, and tearing down the geth process each time for each test case.

It also introduces another way for us to run our tests against an ethereum client by setting up a docker service to bring up the client instead of using DevHelpers to bring up and teardown the geth process.

Recording VCR Cassettes with Docker + ExVCR

Spin up the docker instance for ganache/plasma-contracts:

docker-compose -f docker-compose-fixtures.yml up ganache plasma-contracts

This will spin up ganache in a deterministic mode and deploy the ALD contracts from plasma_framework see here

In your test cases, you can specify the pre-determined addresses (see ganache output in docker) in the setup block. Then just wrap your network calls in use_cassette:

test "get_height/1 returns me a number", %{contract: contract} do
  use_cassette "get_height" do
    OMG.Eth.get_height(contract)
  end
end

Changes

introduce exvcr and record geth responses 9e6148f
Breaks out tests into separate files for each module, e.g eth_test.exs and root_chain_test.exs 9e6148f
reset Ethereuemex into request counter so that we can have a consistent recording. This changes the DevHelpers and test setup block to explicitly reset the counter to 0. 5d9d9f1 8e38bb9
adds a docker-compose-fixtures.yml to spin up ganache and plasma-contracts. Note, currently dumped the service here and can consolidate it after. 14d009f

Testing

mix test --only common test/omg_eth/eth_test.exs
mix test --only common test/omg_eth/root_chain_test.exs
# or do it all
mix test --only common

When we use the `@tag fixtures:` functionality, it calls out to `DevGeth` and etc ... which will send out a request from Ethereumex to ensure that geth is loaded. This whoever causes the request id to increment and causes all the cassettes to not much when you remove the fixtures call. We explicitly reset the counter before we start the actual interactions in the tests.

Need to reset the :rpc_counter on Ethereuemex so that we can have the exvcr cassettes record it in a predicatble manner(start with id 1)

skipped a few tests due to some issues see comments for more details: * `hasToken` not the ALD contract function anymore * `get_deposits/3` running into output encoding errors

achiurizo · 2019-10-07T19:38:55Z

I think the --umbrella flag in the common_coveralls_and_integration_tests is breaking the exvcr usage. 🤔

pdobacz

just some comments first to ramp up, as this is a Draft for now, but looks good 👍

My biggest concern for now is:

how do I actually record the cassettes from scratch? There is a section in the PR description, but I'm not sure if this is complete. I'd assume one would need to bypass exvcr replaying of cassettes and instead have it record them? How are the json files updated in practice
related but a bit separate: how do I just run those tests with exvcr entirely bypassed, using geth, parity (or ganache, since it's supported now)?

pdobacz · 2019-10-08T09:12:34Z

apps/omg_eth/test/fixtures.exs

+    # The problem is the fixtures would send a first request out(this request). When you remove the fixtures,
+    # Ethereumex thinks we are sending the first request, missing the matching cassettes by request_body.
+    # So, we reset the counter so the cassettes can reply correctly without the fixtures:
+    :ets.insert(:rpc_requests_counter, {:rpc_counter, 0})


how about splitting this one-liner into its function (can be put under test/support) so that the docs can be put there too. Then it will make the docs find-able for anyone looking at any of the 6 occurrences of this insert

what do you think about this?

apps/omg_eth/test/fixtures/vcr_cassettes/root_chain/ganache/get_deposits.json

apps/omg_eth/test/omg_eth/root_chain_test.exs

pdobacz · 2019-10-08T09:53:33Z

also on this:

Fix CircleCI where passing in --umbrella breaks the exvcr usage.

I don't have any hints yet, why this would hurt coveralls. Are you able to run CircleCI's invocation locally (or in whichever setup where you have the mix test invocations pass)? That would be more or less these (note the different paths):

mix coveralls --umbrella --only common apps/omg_eth/test/omg_eth/eth_test.exs
mix coveralls --umbrella --only common apps/omg_eth/test/omg_eth/root_chain_test.exs
# or do it all
mix coveralls --umbrella --only common

Also to troubleshoot a bit more, can you run

mix coveralls --umbrella --only common apps/omg_eth/test/omg_eth/eth_test.exs

for elixir-omg/master and see what happens?

Ping me if you get stuck, I'll try to reproduce the problem on my end.

pdobacz · 2019-10-08T10:11:52Z

Ha, actually I played around with this and got something.

If you run the coveralls invocations, you'll see that exvcr attempts generate new cassettes under a parent dir (outside apps/omg_eth). This is because our coveralls --umbrella runs in the root dir, not in the app subdir :(.

Hence the econnrefused are coming from attempts to talk to ganache and record the cassettes. Not sure how to make exvcr choose the correct root path.

plasma-contract.Dockerfile

InoMurko · 2019-10-10T09:02:01Z

THIS IS MAJOR 👏 👏 👏 <3 <3 <3

coveralls · 2019-10-11T22:05:47Z

Coverage remained the same at 84.671% when pulling 2b10b91 on achiurizo/exvcr into e7cae6a on master.

This reverts commit e92e7a9.

achiurizo · 2019-10-15T11:12:14Z

@pdobacz

how do I actually record the cassettes from scratch? There is a section in the PR description, but I'm not sure if this is complete. I'd assume one would need to bypass exvcr replaying of cassettes and instead have it record them? How are the json files updated in practice

Wrapping the code executing the network request will update the json. Depending on your matching preference, it will automatically re-record if it does not have an existing record.

related but a bit separate: how do I just run those tests with exvcr entirely bypassed, using geth, parity (or ganache, since it's supported now)?

Haven't tried this but seems like the approach is to pass in another option like:

use ExVCR.Mock, adapter: ExVCR.Adapter.Hackney, options: [clear_mock: true]

apps/omg_eth/test/fixtures.exs

apps/omg_eth/test/omg_eth/eth_test.exs

InoMurko · 2019-10-15T11:45:53Z

docker-compose-fixtures.yml

@@ -0,0 +1,22 @@
+version: "3.4"


can we get some docs around this so that we know how to record for new "tests"?

pdobacz

3 minor follow-ups to the previous comments

apps/omg_eth/test/omg_eth/root_chain_test.exs

pdobacz · 2019-10-15T13:12:12Z

apps/omg_eth/test/fixtures.exs

+    # The problem is the fixtures would send a first request out(this request). When you remove the fixtures,
+    # Ethereumex thinks we are sending the first request, missing the matching cassettes by request_body.
+    # So, we reset the counter so the cassettes can reply correctly without the fixtures:
+    :ets.insert(:rpc_requests_counter, {:rpc_counter, 0})


what do you think about this?

achiurizo added 7 commits October 5, 2019 03:48

refactor: add exvcr, break out tests

9e6148f

reset rpc_counter after fixture, before tests

8e38bb9

Need to reset the :rpc_counter on Ethereuemex so that we can have the exvcr cassettes record it in a predicatble manner(start with id 1)

recored submit_block, get_child_chain

617cb9f

add get_deposits/3 vcr

e9ff288

use ganache+plasma contracts for vcr

14d009f

record ganache vcrs for root_chain

326432d

skipped a few tests due to some issues see comments for more details: * `hasToken` not the ALD contract function anymore * `get_deposits/3` running into output encoding errors

achiurizo requested review from InoMurko, pdobacz, T-Dnzt, chrishunt, pnowosie and jrhite October 7, 2019 19:37

pdobacz reviewed Oct 8, 2019

View reviewed changes

apps/omg_eth/test/omg_eth/root_chain_test.exs Outdated Show resolved Hide resolved

InoMurko reviewed Oct 10, 2019

View reviewed changes

plasma-contract.Dockerfile Show resolved Hide resolved

achiurizo added 5 commits October 11, 2019 21:29

set path to current dir

faf89f4

use Encoding.from_hex

2b6e468

fix test module name OMG.Eth.RootChainTest

52b6ae9

update trasnact_sync to single arg, not tuple

e92e7a9

lock plasma-contracts to SHA

f9435fa

achiurizo added 4 commits October 11, 2019 22:26

sort order (re: mix credo)

b181933

please work

bafed6f

first off why it 5433..

bc75c1c

add them new cassettes for ganache

cc0649c

achiurizo added 2 commits October 15, 2019 09:30

Revert "update trasnact_sync to single arg, not tuple"

44f8e3a

This reverts commit e92e7a9.

INO WAS RIGHT

dfcc77c

achiurizo force-pushed the achiurizo/exvcr branch from 7a793d9 to 34cc58d Compare October 15, 2019 11:01

remove my debugging and fix

e851855

achiurizo force-pushed the achiurizo/exvcr branch from 34cc58d to e851855 Compare October 15, 2019 11:06

achiurizo marked this pull request as ready for review October 15, 2019 11:12

achiurizo requested review from InoMurko and pdobacz October 15, 2019 11:13

Merge remote-tracking branch 'origin/master' into achiurizo/exvcr

970460c

InoMurko reviewed Oct 15, 2019

View reviewed changes

apps/omg_eth/test/fixtures.exs Show resolved Hide resolved

InoMurko reviewed Oct 15, 2019

View reviewed changes

apps/omg_eth/test/omg_eth/eth_test.exs Show resolved Hide resolved

InoMurko reviewed Oct 15, 2019

View reviewed changes

pdobacz reviewed Oct 15, 2019

View reviewed changes

achiurizo added 5 commits October 19, 2019 03:30

re-record the cassettes

562c401

Merge remote-tracking branch 'origin/master' into achiurizo/exvcr

1973fd5

move Eth.submit_block/5 to eth_test

6786408

sort alias order

beea1ca

update to plasma-deployer SHA

2b10b91

InoMurko approved these changes Oct 19, 2019

View reviewed changes

achiurizo merged commit 105a52d into master Oct 19, 2019

achiurizo deleted the achiurizo/exvcr branch October 19, 2019 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Use exvcr to record eth/rootchain network requests #1037

Refactor: Use exvcr to record eth/rootchain network requests #1037

achiurizo commented Oct 7, 2019 •

edited

Loading

achiurizo commented Oct 7, 2019

pdobacz left a comment

pdobacz Oct 8, 2019

pdobacz Oct 15, 2019

pdobacz commented Oct 8, 2019

pdobacz commented Oct 8, 2019 •

edited

Loading

InoMurko commented Oct 10, 2019

coveralls commented Oct 11, 2019 •

edited

Loading

achiurizo commented Oct 15, 2019

InoMurko Oct 15, 2019

pdobacz left a comment

pdobacz Oct 15, 2019

Refactor: Use exvcr to record eth/rootchain network requests #1037

Refactor: Use exvcr to record eth/rootchain network requests #1037

Conversation

achiurizo commented Oct 7, 2019 • edited Loading

Overview

Recording VCR Cassettes with Docker + ExVCR

Changes

Testing

achiurizo commented Oct 7, 2019

pdobacz left a comment

Choose a reason for hiding this comment

pdobacz Oct 8, 2019

Choose a reason for hiding this comment

pdobacz Oct 15, 2019

Choose a reason for hiding this comment

pdobacz commented Oct 8, 2019

pdobacz commented Oct 8, 2019 • edited Loading

InoMurko commented Oct 10, 2019

coveralls commented Oct 11, 2019 • edited Loading

achiurizo commented Oct 15, 2019

InoMurko Oct 15, 2019

Choose a reason for hiding this comment

pdobacz left a comment

Choose a reason for hiding this comment

pdobacz Oct 15, 2019

Choose a reason for hiding this comment

achiurizo commented Oct 7, 2019 •

edited

Loading

pdobacz commented Oct 8, 2019 •

edited

Loading

coveralls commented Oct 11, 2019 •

edited

Loading