[Filetests]: Add assert helper function #229
Conversation
MCJOHN974
force-pushed
the
viktar/custom-assert
branch
from
b5b508f
to
9473a1e
Compare
MCJOHN974
changed the title
I would lean towards keeping PRs smaller (so not changing filetests for now), but whatever is convenient to you works. |
In that case I'm ok to keep this one as is |
tests/zkasm/assert.js
Outdated
| // TODO: why arg* is list of two integers with last zero and first value in register??? | ||
| // Is it chunks? If yes they should be merged instead of ignoring second one |
There was a problem hiding this comment.
As I see it is really chunks and total number can be calculated as ctx[tag.params[idx].regName][0] + ctx[tag.params[idx].regName][1] * 2**32 But maybe it worth to ask Polygon team?
tests/zkasm/assert.js
Outdated
| } | ||
|
|
||
| /** Writes results to `config.outputFile`. */ | ||
| eval_assert_dump(ctx, tag) { |
There was a problem hiding this comment.
Do we need to call eval_assert_dump from zkAsm for some reason?
If not, we can make it a normal method of this helper class (without eval_ prefix) and call it in run-tests-zkasm.js directly.
This way we can avoid the use of fs in this class and the need to pass the outputFile in the constructor and instead do all of this in run-tests-zkasm.js. This will make this class simpler (less responsibilities) and will make it easier to write unit-tests for it.
tests/zkasm/assert.js
Outdated
| @@ -0,0 +1,68 @@ | |||
| const fs = require("fs"); | |||
|
|
|||
| class InstrumentInst { | |||
tests/zkasm/assert.js
Outdated
| * ${assert_neq(regname, regname, tag)}, for example: | ||
| * ${assert_neq(A, B, test_generated_from_line_3000)} | ||
| */ | ||
| eval_assert_neq(ctx, tag) { |
tests/zkasm/run-tests-zkasm.js
Outdated
| @@ -10,6 +10,7 @@ const { | |||
| newCommitPolsArray | |||
| } = require('pilcom'); | |||
| const buildPoseidon = require('@0xpolygonhermez/zkevm-commonjs').getPoseidon; | |||
| const assert_helper = require('./assert'); | |||
There was a problem hiding this comment.
Let's use AssertHelper name here instead of assert_helper to signal that this is a class. This will also make it easier to search the codebase as this name will match the one in the assert.js.
tests/zkasm/assert.js
Outdated
There was a problem hiding this comment.
Probably assert_helper.js would be a less ambiguous name.
There was a problem hiding this comment.
Since there will be more helpers (e.g. for benchmarking), I would propose to generate a directory like tests/zkasm/helpers and put all helpers there.
tests/zkasm/assert.js
Outdated
| @@ -0,0 +1,68 @@ | |||
| const fs = require("fs"); | |||
|
|
|||
| class InstrumentInst { | |||
tests/zkasm/assert.js
Outdated
| * Helper function must be used in format: | ||
| * ${assert_eq(regname, regname, tag)}, for example: | ||
| * ${assert_eq(A, B, test_generated_from_line_3000)} |
There was a problem hiding this comment.
Comments usually go before any @param in JSDoc (ref).
tests/zkasm/assert.js
Outdated
| // TODO: why arg* is list of two integers with last zero and first value in register??? | ||
| // Is it chunks? If yes they should be merged instead of ignoring second one |
There was a problem hiding this comment.
I feel like this TODO should be resolved before merging. A starting point to find out more about ctx and tag passed to helpers might be helper invocation in zkevm-proverjs.
tests/zkasm/assert.js
Outdated
| const arg1 = ctx[tag.params[1].regName][0]; | ||
| const testname = tag.params[2].varName; | ||
| this.results[testname] = arg0 === arg1 ? 'pass' : 'fail'; | ||
| return 0; |
There was a problem hiding this comment.
Is the return value supposed to be written into a register? If yes, why does it not depend on the assertion?
There was a problem hiding this comment.
Good find, thanks! It is because I used only one dollar and functions with one dollar expect output.
MCJOHN974
force-pushed
the
viktar/custom-assert
branch
from
df64dfd
to
577c41d
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Hm, seems one of the tests is failing due to update of some node packages |
I think that was a flaky run, rerunning the same test now passes: https://github.com/near/wasmtime/actions/runs/8009020692 |
This PR adds few helper functions:
assert_eq,assert_neq,assert_dump. Filetest infra not ready yet, so I didn't change old asserts to new ones. Should I do it before merge or it can be done after? Or I should add.zkasmfile somewhere to make testing this helper easy outside my machine?