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 runBlock API tests #360

Merged
merged 1 commit into from
Sep 27, 2018
Merged

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Sep 26, 2018

This PR is part of the series for adding API test coverage. It:

  • Adds tests for runBlock.js
  • Modifies lib/runBlock.js to add two validation checks.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Great, thanks for the PR, some smaller changes.

This also needs to be rebased on the way.

if (!opts.block) {
return cb(new Error('invalid input, block must be provided'))
}

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

populateCache: stateManager.warmCache.bind(stateManager),
runTx: (opts, cb) => cb(new Error('test')),
runCall: (opts, cb) => cb(new Error('test')),
_common: new Common('mainnet', 'byzantium')
Copy link
Member

Choose a reason for hiding this comment

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

Good that you instantiate explicitly with Byzantium here, so that we know the test data used is created in a Byzantium HF context.

@@ -0,0 +1,121 @@
{
"blocks" : [
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a "comment" field (shouldn't break anything, should it?) or similar here with a note, where you are taking the test data from (I assume from ethereum/tests together with a date and HF note?

.catch((e) => st.ok(e.message.includes('invalid input'), 'correct error'))

st.end()
})
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

.catch((e) => st.ok(e.message.includes('invalid input'), 'correct error'))

st.end()
})
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

.then(() => t.fail('should have returned error'))
.catch((e) => t.equal(e.message, 'test'))

st.end()
Copy link
Member

Choose a reason for hiding this comment

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

Not seeing this instantaneously, why is runTx failing here? Maybe this is more obvious then I get right now, but eventually make it a bit more explicit (by variable naming or comment adding) anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it wasn't very clear, because the suite setup is happening at the beginning of the file. Here and in the other test case runTx and runCall are failing because I've replaced them with a mock that simply calls the callback with an error.

I added comments to explain this in the source file as well.

.catch((e) => t.ok(e.message.includes('higher gas limit')))

t.end()
})
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

suite.vm.runTx = runTx
await suite.p.runBlock({ block, root: suite.vm.stateManager.trie.root })
.then(() => t.fail('should have returned error'))
.catch((e) => t.equal(e.message, 'test'))
Copy link
Member

Choose a reason for hiding this comment

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

Again here, a bit more explicit stating why runCall fails so that others can easily grab would be nice.

t.equal(res.results[0].gasUsed.toString('hex'), '5208', 'actual gas used should equal blockHeader gasUsed')

t.end()
})
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

Copy link
Member

@holgerd77 holgerd77 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, thanks!

@holgerd77 holgerd77 merged commit aedda26 into ethereumjs:master Sep 27, 2018
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.

3 participants