Change stateManager.trie to stateManager._trie in GeneralStateTestsRunner #438
Conversation
tests/BlockchainTestsRunner.js
Outdated
| @@ -91,6 +91,9 @@ module.exports = function runBlockchainTest (options, testData, t, cb) { | |||
| cb(err) | |||
| }) | |||
| } catch (err) { | |||
| if (err) { | |||
| console.log(err) | |||
There was a problem hiding this comment.
Did you encounter errors here when testing? In that case, maybe explicitly failing with t.fail(err) would help find the errors.
There was a problem hiding this comment.
Update: using t.fail will cause some invalid test cases to fail. We can just remove the console.log here.
There was a problem hiding this comment.
Good call! I updated the PR.
There was a problem hiding this comment.
Ah you changed it to t.fail :) sorry, my first comment was mistaken, I think we can ignore the error here, no need for any handling.
There was a problem hiding this comment.
I kinda like having the test fail here when it has err, otherwise it fails silently and make it a tad bit more difficult to debug. What do you think @s1na ?
There was a problem hiding this comment.
Sorry, my comment was meant for the other one (GeneralStateTestsRunner), see below.
tests/GeneralStateTestsRunner.js
Outdated
| @@ -100,6 +100,9 @@ function runTestCase (options, testData, t, cb) { | |||
| tx: tx, | |||
| block: block | |||
| }, function (err, r) { | |||
| if (err) { | |||
| console.log(err) | |||
There was a problem hiding this comment.
I definitely agree with you that its better to have tests fail explicitly. In this case however, tests are correct: they are testing that the VM functions correctly when input is invalid.
E.g. current state root is 0xdeadbeef, when the VM is given an invalid tx (sender doesn't have enough gas), runTx throws an error. The tests later check that the state root shouldn't have changed for an invalid tx.
There was a problem hiding this comment.
make sense - updated!
|
Thanks again for the PR and sorry for the nitpicks :) Update: Branch was out-of-date, merged master. |
Fix #390
stateManager.trieshould bestateManager._trieinstead. I have also included a change to surface error message in test, as the testrunner was swallowing the error message, making it difficult to debug failing test.