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

Pipe test results using JSON #113

Merged
merged 4 commits into from
Nov 27, 2023
Merged

Pipe test results using JSON #113

merged 4 commits into from
Nov 27, 2023

Conversation

aborg-dev
Copy link

This PR is stacked on top of #112

This PR changes how run-tests.zkasm.js communicates results to the zkasm-result.py. Before, we used to parse log lines, now we pass a json object with a test result for each of the tests. This will allow storing more detailed information like performance counters for #90 and error messages.

@aborg-dev aborg-dev added the C-housekeeping Category: Refactoring, cleanups, code quality label Nov 27, 2023
@aborg-dev aborg-dev requested a review from MCJOHN974 November 27, 2023 13:05
@MCJOHN974
Copy link

MCJOHN974 commented Nov 27, 2023

Hmm, why tests was skipped? I guess this PR should change csv's, adding more columns, isn't it?

upd: oh, PR is not to master, I see

upd2: in general LGTM, but I'm not shure if csv asserts will be passed here

@aborg-dev
Copy link
Author

Hmm, why tests was skipped? I guess this PR should change csv's, adding more columns, isn't it?

upd: oh, PR is not to master, I see

upd2: in general LGTM, but I'm not shure if csv asserts will be passed here

Good call. I've tested locally that this code can both generate and validate CSVs, but I would expect our CI to run again when I rebase this on main branch.

Base automatically changed from unify_tests to main November 27, 2023 13:24
This PR changes how run-tests.zkasm.js communicates results to the zkasm-result.py.
Before, we used to parse log lines, now we pass a json object with a test result for each of the tests.
This will allow storing more detailed information like performance counters for #90 and error messages.
@aborg-dev
Copy link
Author

The test failed because the heuristic to skip the first 4 lines in the output did not work when we needed to recompile the Main PIL file.
I've dropped this heuristic and instead started to communicate between processes using a temporary file explicitly.
This also allows us to inspect this test file locally if we need to debug this part.

@MCJOHN974
Copy link

Can you also please return ability to just run one given file using ./ci/test-zk.sh please

@aborg-dev
Copy link
Author

Can you also please return ability to just run one given file using ./ci/test-zk.sh please

Ah, sorry for breaking this, will fix this in the follow-up PR.

Would something like this work for now?

npm test --prefix tests/zkasm "../../cranelift/zkasm_data/generated/add.zkasm"

@aborg-dev aborg-dev added this pull request to the merge queue Nov 27, 2023
@MCJOHN974
Copy link

npm test --prefix tests/zkasm "../../cranelift/zkasm_data/generated/add.zkasm"

Yes, it works

Merged via the queue into main with commit 2b7ffda Nov 27, 2023
21 checks passed
@aborg-dev aborg-dev deleted the json_export branch November 27, 2023 14:45
@aborg-dev aborg-dev added this to the ZK WASM: Stage 1 milestone Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants