-
Notifications
You must be signed in to change notification settings - Fork 756
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
Conversation
There was a problem hiding this 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')) | ||
} | ||
|
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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" : [ |
There was a problem hiding this comment.
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() | ||
}) |
There was a problem hiding this comment.
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() | ||
}) |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() | ||
}) |
There was a problem hiding this comment.
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')) |
There was a problem hiding this comment.
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() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
827b1b0
to
3c33cff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
This PR is part of the series for adding API test coverage. It:
runBlock.js
lib/runBlock.js
to add two validation checks.