CI: spectests in CI #95

MCJOHN974 · 2023-11-21T15:39:34Z

Added script to generate .csv with spectest results and asserting commited result corresponds actual results in CI

Fixes #84

MCJOHN974 · 2023-11-21T15:52:19Z

Am I right that ci/run-tests.sh runs test_zkasm::tests::run_spectests?

nagisa · 2023-11-22T10:29:35Z

#bash ./ci/run-tests.sh --locked | grep test_zkasm
<snip>
test test_zkasm::tests::run_spectests ... ok
<snip>

nagisa · 2023-11-22T10:31:50Z

I’ll defer this to @Akashin since it was their request. I personally think it would be easier on the eyes if this file was post-processed with columns -t -s',' -o',' but I can see why we might not want to do that either (e.g. test additions will potentially cause the entire table to be reformatted which can then be hard to diff.)

MCJOHN974 · 2023-11-22T12:18:00Z

@nagisa can you take a look please, why it fails? As prints show python script goes to sys.exit(0) but fails... Locally whole pipe exits with 0. Pipe return status of last command so problem should be around python script..

aborg-dev · 2023-11-22T12:24:37Z

I’ll defer this to @Akashin since it was their request. I think it would be easier on the eyes if this file was post-processed with columns -t -s',' -o',' but I can see why we might not want to do that either (e.g. test additions will potentially cause the entire table to be reformatted which can then be hard to diff.)

I do agree that we would benefit from making this output easily readable by humans to make it easier to study this data. I think CSV is fine for the first version, but we should iterate on it in the future PR.

aborg-dev · 2023-11-22T12:02:22Z

.github/workflows/main.yml

@@ -50,6 +50,19 @@ jobs:
    - run: npm ci --prefix tests/zkasm
    - run: ./ci/test-zkasm.sh

+  spectest_zkasm:


I suggest we merge this with the previous test_zkasm job:

They are semantically in the same category

We will likely have more of these tests (for each of spec test files) and we wouldn't want a new job for each one of them

This will make sure we reuse NPM cache between jobs and save execution time as that is the dominating step for this job

Am I missing any reasons to split these into separate jobs?

aborg-dev · 2023-11-22T12:04:45Z

ci/zkasm-result.py

+        if file.endswith('.wat'):
+            test_name = os.path.splitext(file)[0]
+            zkasm_file = f'{test_name}.zkasm'
+            status_map[test_name] = 'compilation success' if zkasm_file in os.listdir(generated_dir) else 'compilation failed'


You can do os.path.exists(os.path.join(generated_dir, zkasm_file)) to be more specific

aborg-dev · 2023-11-22T12:05:41Z

ci/zkasm-result.py

+def check_compilation_status():
+    status_map = {}
+    for file in os.listdir(tests_dir):
+        if file.endswith('.wat'):


General idea to reduce code nesting, use continue:

if not file.endswith('.wat'): continue # Code below becomes less nested. ...

aborg-dev · 2023-11-22T12:09:27Z

ci/zkasm-result.py

+
+def update_status_from_stdin(status_map):
+    for line in sys.stdin:
+        if "--> fail" in line or "--> pass" in line:


That's the reason I originally suggested doing this in run-tests-zkasm.js where we have the information about test status programmatically and don't have to parse strings.

I'm fine with keeping this in Python for now, but if you agree moving this to JS will simplify code, feel free to do this.

This parsing takes just a few lines, and I found that wasmtime repo already contains some python files, so it won't be a big deviation from style of the repo, so I decided proceed with more convenient for me language

aborg-dev · 2023-11-22T12:12:02Z

ci/zkasm-result.py

+        csvwriter = csv.writer(csvfile)
+        csvwriter.writerow(['Test', 'Status'])
+        status_list = sorted(list(map(lambda x: (x, status_map[x]), status_map)))
+        for (test, status) in status_list:


You can also do csvwriter.writerows(status_list)

aborg-dev · 2023-11-22T12:12:53Z

ci/zkasm-result.py

+    with open(state_csv_path, 'w', newline='') as csvfile:
+        csvwriter = csv.writer(csvfile)
+        csvwriter.writerow(['Test', 'Status'])
+        status_list = sorted(list(map(lambda x: (x, status_map[x]), status_map)))


You can simplify this to sorted(status_map.items())

aborg-dev · 2023-11-22T12:16:03Z

ci/zkasm-result.py

+    for line in sys.stdin:
+        if "--> fail" in line or "--> pass" in line:
+            _, _, test_path = line.partition(' ')
+            test_name = os.path.basename(test_path).split('.')[0]


The canonical way to do this is:

filename, file_extension = os.path.splitext(os.path.basename('/path/to/somefile.ext')) # filename == "somefile"

aborg-dev · 2023-11-22T12:17:26Z

ci/zkasm-result.py

+    print("assert foo")
+    with open(state_csv_path, newline='') as csvfile:
+        csvreader = csv.reader(csvfile)
+        print("csvreader ok")


Leaving a comment not to forget to remove debug statements before submitting the PR.

aborg-dev · 2023-11-22T12:19:55Z

ci/zkasm-result.py

+        print('a')
+        for row in csvreader:
+            print('b')
+            if row[0] == "Test" or row[0] == "Total Passed" or row[0] == "Amount of Tests":


A way to simplify: if row[0] in ["Test", "Total Passed", "Amount of Tests"]:

aborg-dev · 2023-11-22T12:29:56Z

@nagisa can you take a look please, why it fails? As prints show python script goes to sys.exit(0) but fails... Locally whole pipe exits with 0. Pipe return status of last command so problem should be around python script..

I suspect this is because we have pipefail set and the first command npm test --prefix tests/zkasm ../../cranelift/zkasm_data/spectest/i64/generated fails

See https://gist.github.com/mohanpedala/1e2ff5661761d3abd0385e8223e16425#set--o-pipefail

aborg-dev

Let's ship it!

MCJOHN974 · 2023-11-22T14:34:39Z

Thanks for review, Andrei!

aborg-dev suggested changes Nov 22, 2023

View reviewed changes

CI: spectests in CI

e204a9e

MCJOHN974 force-pushed the viktar/spectests branch from da32ac4 to e204a9e Compare November 22, 2023 13:12

MCJOHN974 requested a review from aborg-dev November 22, 2023 13:37

aborg-dev approved these changes Nov 22, 2023

View reviewed changes

MCJOHN974 added this pull request to the merge queue Nov 22, 2023

Merged via the queue into main with commit 4e7366b Nov 22, 2023
21 checks passed

MCJOHN974 deleted the viktar/spectests branch November 22, 2023 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: spectests in CI #95

CI: spectests in CI #95

MCJOHN974 commented Nov 21, 2023 •

edited

Loading

MCJOHN974 commented Nov 21, 2023

nagisa commented Nov 22, 2023

nagisa commented Nov 22, 2023

MCJOHN974 commented Nov 22, 2023

aborg-dev commented Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

MCJOHN974 Nov 22, 2023 •

edited

Loading

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev Nov 22, 2023

aborg-dev commented Nov 22, 2023

aborg-dev left a comment

MCJOHN974 commented Nov 22, 2023

CI: spectests in CI #95

CI: spectests in CI #95

Conversation

MCJOHN974 commented Nov 21, 2023 • edited Loading

MCJOHN974 commented Nov 21, 2023

nagisa commented Nov 22, 2023

nagisa commented Nov 22, 2023

MCJOHN974 commented Nov 22, 2023

aborg-dev commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MCJOHN974 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aborg-dev commented Nov 22, 2023

aborg-dev left a comment

Choose a reason for hiding this comment

MCJOHN974 commented Nov 22, 2023

MCJOHN974 commented Nov 21, 2023 •

edited

Loading

MCJOHN974 Nov 22, 2023 •

edited

Loading