add --jsontrace flag for state tests #194
Conversation
| vm.on('step', function (e) { | ||
| let hexStack = [] | ||
| hexStack = e.stack.map(item => { | ||
| return '0x' + new BN(item).toString(16, 0) |
There was a problem hiding this comment.
That was from your suggestion in #170 . But it doesn't look toString takes a second argument so I will remove it.
There was a problem hiding this comment.
Oh sorry, looked it up now. The second parameter is for padding (defaults to 1) where 0 means disable padding, i.e. it will not add a second nibble if the number of nibbles is odd.
e.g. 0x0 is not presented as 0x01
Not sure which one is needed according the tracing specs.
There was a problem hiding this comment.
Ah I see. Looks like there is no padding according to the example trace here: ethereum/tests#249
lib/opcodes.js
Outdated
| @@ -175,5 +175,5 @@ module.exports = function (op, full) { | |||
| } | |||
| } | |||
|
|
|||
| return {name: opcode, fee: code[1], in: code[2], out: code[3], dynamic: code[4], async: code[5]} | |||
| return {name: opcode, rawOp: op, fee: code[1], in: code[2], out: code[3], dynamic: code[4], async: code[5]} | |||
There was a problem hiding this comment.
Agreed. I think that is a better name.
tests/GeneralStateTestsRunner.js
Outdated
| @@ -54,6 +77,9 @@ function runTestCase (testData, t, cb) { | |||
| } | |||
| }, | |||
| function (done) { | |||
| if (options.jsontrace) { | |||
| console.log('{ "steps": [' + steps + ']}') | |||
There was a problem hiding this comment.
I think you'd need to use JSON.stringify(steps) here too? Perhaps:
JSON.stringify({ "steps": steps })
I am also wondering which is faster / uses less memory (not sure which we are optimising for), keeping the array elements stringified or as a map?
There was a problem hiding this comment.
Agreed. That is preferable. I do seem to remember discovering test cases which produce enough steps to crash ethereumjs-vm when trying to print so many steps in one statement.
To use less memory, it might be good to loop through each element in steps and print individually.
However, I think this suggestion is good for now until I can identify which tests causes the VM to crash with tracing on. I think it's actually just preferable to print steps in place and not collect them into an array.
|
also it turns out some of this already in the Metro/byz working branch.. c4cb02b |
|
Yes, added this some time ago for debugging, but not with standard trace in mind and not on such a high-level. Just ignore for the moment, will be removed on next rebase when this PR is merged. |
tests/GeneralStateTestsRunner.js
Outdated
| @@ -77,9 +76,6 @@ function runTestCase (options, testData, t, cb) { | |||
| } | |||
| }, | |||
| function (done) { | |||
| if (options.jsontrace) { | |||
| console.log('{ "steps": [' + steps + ']}') | |||
There was a problem hiding this comment.
If you print it in place, you need to print { "steps":[ here, the comma in place and ]} at the end.
There was a problem hiding this comment.
I made the changes to make the output conform to the example here: ethereum/tests#249 .
There are no commas between steps. Each step is logged on its own line.
There was a problem hiding this comment.
Is this PR still far away (sorry, didn't really follow discussion)? Would be really useful, I always monkey-path this stuff together again and again...
There was a problem hiding this comment.
@jwasinger ah, that original issue isn't really clear on a spec.
There was a problem hiding this comment.
@holgerd77 This is ready to merge.
There was a problem hiding this comment.
Probably @cdetrio should review this, since he has started the initiative. Casey?
|
@jwasinger @holgerd77 can you please avoid the "sync" button on the purge request because that creates a merge from master and then a merge into master. Just ignore it if you are certain or rebase it properly otherwise. |
|
I did not notice that until now. Thanks for informing me @axic |
squashed #170 with authorship retained.