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

feat: configurable external check failures #6810

Merged
merged 37 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
3c669de
Run external checks with `--format json`
asterite Dec 13, 2024
825a909
Include "suite" in test end event
asterite Dec 16, 2024
79a60b7
Compare test results
asterite Dec 16, 2024
e087aaa
Output inside github.workspace
asterite Dec 16, 2024
d114780
Move files around
asterite Dec 16, 2024
5e06ce3
Merge branch 'master' into ab/external-checks-ignore-test-failures
asterite Dec 16, 2024
d7bde50
More fixes
asterite Dec 16, 2024
eafea81
debug
asterite Dec 16, 2024
c35ab86
Need to checkout current repo?
asterite Dec 16, 2024
8eed8c9
Checkout in noir-repo
asterite Dec 16, 2024
1068209
Show actual run output
asterite Dec 16, 2024
1635062
Try to use correct names
asterite Dec 16, 2024
b31ac6b
More files
asterite Dec 16, 2024
fabdcd0
Fix script
asterite Dec 16, 2024
fe2e626
Reuse function and better output
asterite Dec 16, 2024
bd8bb22
Only compare failed and ignored tests
asterite Dec 16, 2024
29a19ae
Only compare failures
asterite Dec 16, 2024
7bacd52
Run rollup-lib tests with 2 threads
asterite Dec 16, 2024
41baafc
Try with 1 thread
asterite Dec 16, 2024
77a1eb6
Skip rollup-lib for now
asterite Dec 16, 2024
e204139
chore: always run external checks
TomAFrench Dec 16, 2024
99170d9
Check rollup-lib again
asterite Dec 16, 2024
c62e152
Merge branch 'ab/external-checks-ignore-test-failures' of github.com:…
asterite Dec 16, 2024
769b89b
Merge branch 'master' into ab/external-checks-ignore-test-failures
asterite Dec 16, 2024
5e37a5e
Revert "Check rollup-lib again"
asterite Dec 16, 2024
883ed4e
Merge branch 'master' into ab/external-checks-ignore-test-failures
asterite Dec 16, 2024
70e9a34
Reapply "Check rollup-lib again"
asterite Dec 17, 2024
ef0e1b9
Increase timeout minutes for external checks
asterite Dec 17, 2024
3643e83
Document a bit, renames, and some fixes
asterite Dec 17, 2024
eaef064
Don't create file if command fails
asterite Dec 17, 2024
c7ecc5a
Missed renaming file
asterite Dec 17, 2024
00117c7
Use one thread for rollup-lib
asterite Dec 17, 2024
1db6d59
Add file
asterite Dec 17, 2024
9cda5db
--skip-brillig-constraints-check
asterite Dec 17, 2024
fe66452
Explain why we use 1 thread for rollup-lib
asterite Dec 17, 2024
4aa9232
Restore the 30 minutes limit
asterite Dec 17, 2024
a753ddb
Update scripts/check-critical-libraries.sh
TomAFrench Dec 17, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/critical_libraries_status/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Critical Libraries Status

This directory contains one `.failures.jsonl` file per external directory that is checked by CI.
CI will run the external repository tests and compare the test failures against those recorded
in these files. If there's a difference, CI will fail.

This allows us to mark some tests as expected to fail if we introduce breaking changes.
When tests are fixed on the external repository, CI will let us know that we need to remove
the `.failures.jsonl` failures on our side.

The format of the `.failures.jsonl` files is one JSON per line with a failure:

```json
{"suite":"one","name":"foo"}
```

If it's expected that an external repository doesn't compile (because a PR introduces breaking changes
to, say, the type system) you can remove the `.failures.jsonl` file for that repository and CI
will pass again. Once the repository compiles again, CI will let us know and require us to put
back the `.failures.jsonl` file.
4 changes: 4 additions & 0 deletions .github/critical_libraries_status/noir-lang/ec/.actual.jsonl
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{"event":"started","name":"one","test_count":1,"type":"suite"}
{"event":"started","name":"foo","suite":"one","type":"test"}
{"event":"failed","exec_time":0.05356625,"name":"foo","suite":"one","type":"test"}
{"event":"ok","failed":0,"ignored":0,"passed":1,"type":"suite"}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"suite":"one","name":"foo"}
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
38 changes: 38 additions & 0 deletions .github/scripts/check_test_results.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/bin/bash
set -eu

# Usage: ./check_test_results.sh <expected.json> <actual.jsonl>
# Compares the output of two test results of the same repository.
# If any of the files doesn't exist or is empty, the script will consider that the test suite
# couldn't be compiled.

function process_json_lines() {
cat $1 | jq -c 'select(.type == "test" and .event == "failed") | {suite: .suite, name: .name}' | jq -s -c 'sort_by(.suite, .name) | .[]' > $1.jq
}

if [ -f $1 ] && [ -f $2 ]; then
# Both files exist, let's compare them
$(process_json_lines $2)
if ! diff $1 $2.jq; then
echo "Error: test failures don't match expected failures"
echo "Lines prefixed with '>' are new test failures (you could add them to '$1')"
echo "Lines prefixed with '<' are tests that were expected to fail but passed (you could remove them from '$1')"
fi
elif [ -f $1 ]; then
# Only the expected file exists, which means the actual test couldn't be compiled.
echo "Error: external library tests couldn't be compiled."
echo "You could rename '$1' to '$1.does_not_compile' if it's expected that the external library can't be compiled."
exit -1
elif [ -f $2 ]; then
# Only the actual file exists, which means we are expecting the external library
# not to compile but it did.
echo "Error: expected external library not to compile, but it did."
echo "You could create '$1' with these contents:"
$(process_json_lines $2)
cat $2.jq
exit -1
else
# Both files don't exists, which means we are expecting the external library not
# to compile, and it didn't, so all is good.
exit 0
fi
17 changes: 13 additions & 4 deletions .github/workflows/test-js-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -543,8 +543,6 @@ jobs:
external-repo-checks:
needs: [build-nargo, critical-library-list]
runs-on: ubuntu-22.04
# Only run when 'run-external-checks' label is present
if: contains(github.event.pull_request.labels.*.name, 'run-external-checks')
timeout-minutes: 30
strategy:
fail-fast: false
Expand All @@ -557,11 +555,17 @@ jobs:
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/parity-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/reset-kernel-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-lib }
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/types }
# Use 1 test threads for rollup-lib because each test requires a lot of memory, and multiple ones in parallel exceed the maximum memory limit.
- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-lib, nargo_args: "--test-threads 1" }

name: Check external repo - ${{ matrix.project.repo }}/${{ matrix.project.path }}
steps:
- name: Checkout
uses: actions/checkout@v4
with:
path: noir-repo

- name: Checkout
uses: actions/checkout@v4
with:
Expand Down Expand Up @@ -592,9 +596,14 @@ jobs:

- name: Run nargo test
working-directory: ./test-repo/${{ matrix.project.path }}
run: nargo test -q --silence-warnings
run: |
out=$(nargo test --silence-warnings --skip-brillig-constraints-check --format json ${{ matrix.project.nargo_args }}) && echo "$out" > ${{ github.workspace }}/noir-repo/.github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl
env:
NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true

- name: Compare test results
working-directory: ./noir-repo
run: .github/scripts/check_test_results.sh .github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.failures.jsonl .github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl

# This is a job which depends on all test jobs and reports the overall status.
# This allows us to add/remove test jobs without having to update the required workflows.
Expand Down
1 change: 1 addition & 0 deletions tooling/nargo_cli/src/cli/test_cmd/formatters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ impl Formatter for JsonFormatter {
let mut json = Map::new();
json.insert("type".to_string(), json!("test"));
json.insert("name".to_string(), json!(&test_result.name));
json.insert("suite".to_string(), json!(&test_result.package_name));
json.insert("exec_time".to_string(), json!(test_result.time_to_run.as_secs_f64()));

let mut stdout = String::new();
Expand Down
Loading