API Tests for runBlockchain

[s1na]

This is part of the PR series for adding API tests to the vm:

• Adds iterator to fakeBlockchain to facilitate testing
• Fixes a minor issue for propagating errors in runBlockchain
• Adds tests for runBlockchain, considering the new API with blockchain moving out of stateManager.

[holgerd77]

Hi Sina, can you execute runBlockchain() from within a VM context (so vm.runBlockchain()) here and not by a standalone require?

[s1na]

Hey Holger. Sure thing. I was trying to use mocks as much as possible in the individual modules, and then use real objects in the tests for index, like this.

[holgerd77]

Ah okay, if this follows some overall concept, it should be ok

[s1na]

Great, I rebased master into this branch to incorporate the recent changes.

[holgerd77]

Hi @s1na, sorry, I am a bit delayed on reviews due to personal reasons. If you are getting impatient or stuff, you can always drop me a note - maybe as a pm on Gitter - then I try to re-sort some priorities.

[s1na]

Hey @holgerd77, I totally understand and there's no hurry on my side ;)
P.S. Thanks a lot for taking the time to communicate such matters.

[holgerd77]

Hi @s1na, if I run the tests from this PR locally with node tests/api/runBlockchain.js, 2 out of 4 of the tests are failing:

TAP version 13
# runBlockchain
# should run without a blockchain parameter
# should run without blocks
# should run with genesis block
ok 1 genesis should be set for blockchain
# should run with valid and invalid blocks
ok 2 block3 should be the current head
not ok 3 should have returned error
  ---
    operator: fail
    at: Test.t.test (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/tests/api/runBlockchain.js:70:10)
  ...
not ok 4 test exited without ending
  ---
    operator: fail
  ...

1..4
# tests 4
# pass  2
# fail  2

Maybe it would be best to first update #327 to include the new tests in the tests and coverage suite (have a look at my comment over there if you can go along with the suggested changes) and rebase this on top. Then we have an explicit feedback here directly in the CircleCI test run if the new tests are passing.

[s1na]

@holgerd77 I think the failing tests might be due to ethereumjs/ethereumjs-blockchain#60. They were passing for me locally due to my mistake of modifying ethereumjs-blockchain in node_modules and then forgetting to revert the changes...sorry.
When I tried to update ethereumjs-blockchain manually to 3.2.0, another issue resurfaced. I created a PR for it. Hopefully afterwards, the runBlockchain tests should pass.

[holgerd77]

Hi Sina, I've now added the API tests to the CircleCI tests (we somewhat forgot about that) with this #347 PR.

I also released your fix on the blockchain library as v3.2.1.

Would be good if you could now rebase this on top of this, then we can see more transparently what is going on and if this can be merged.

[holgerd77]

Ah, sorry, my fault. The VM actually generally is not yet updated to the ethereumjs-blockchain3.x series, this is actually in the working on #325. This should be merged soon.

[holgerd77]

It would be probably good if you undo the latest commit and then rebase again once the PR mentioned above is merged. //cc @vpulim

[holgerd77]

Did the respective changes in the PR from above.

[s1na]

From Restore node_modules cache:

No cache is found for key: v1-node-pull/336-mbSU64mYTrWWbsyhoNqPw4O1kp7BMwKRG7Xhh_pobE0=
Found a cache from build 1631 at v1-node-pull/336-

It could be that circleci is falling back to a previous node_modules cache, and is not actually using the most recent dependencies to run the tests.

[holgerd77]

I just removed and fetched your updated branch and tested locally, this has the same 4 failing tests as shown in the CircleCI log (you have access to that, or haven't you?), so this should be relatively certain something in the tests or the tests discovering an implementation issue.

[holgerd77]

Ah interesting. I am not understanding deeply enough what is going on, do you think we should permanently keep the cache changes from your temporary commit?

[s1na]

@holgerd77 I'm also not sure, but I think the worst that can happen is that circleci will take longer in some cases (like this PR), because it will need to re-install dependencies.

[holgerd77]

Do you know if this cache is also applying for the test runs itself (so that e.g. state tests are not re-run)? This would eventually be a pity, since we could loose a lot of test performance here.

Is there also a possible key combination like v1-node-{{ checksum "package.json" }} without the branch referenced? Intuitively I would assume that this might be some middle ground to not waste too much cache performance gains.

[s1na]

This cache is only for node_modules I think.

That's a good point, as long as the checksum of package.json is the same, it shouldn't make a difference which branch it is, right? So are you suggesting replacing this key, with what you mentioned, or having that as fallback?

keys:
    - v1-node-{{ .Branch }}-{{ checksum "package.json" }}
    - v1-node-{{ checksum "package.json" }}

or only the second key?

[holgerd77]

At a first look at https://circleci.com/docs/2.0/caching/, the keys seems pretty flexible to compose. Yes, can you remove your last commit and try the two-key-version, with a commit message without the "tmp" part, then we can maybe directly take this if it turns out to work.

[holgerd77]

Hi Sina, I will have more time to reviewing more stuff in the coming 1-2 weeks, thanks for keeping on with this for so long. We should nevertheless get this settled now timely and come to the Gitcoin-bounty payout point, this is already going on for too long (due to our fault).

[s1na]

Hi Holger, for me the bounty payout is marginal incentive, and the important thing was getting the chance to work on a specific task, with your help, and learn the inner workings of the vm, which happens during these reviews. So no hurries on that account.

With that said, I would also like to see this through, if only to start the next task, and will be available during these 2 weeks.

Please let me know, if this PR requires modifications. In the meanwhile I'll get started on the next PR.

[holgerd77]

Hi @s1na, this looks good, thanks for the PR. 😄