Skip to content

Commit

Permalink
feat: configurable external check failures (noir-lang/noir#6810)
Browse files Browse the repository at this point in the history
chore: move constant creation out of loop (noir-lang/noir#6836)
fix: implement `as_field` and `from_field` in the interpreter (noir-lang/noir#6829)
chore: Use Vec for callstacks (noir-lang/noir#6821)
feat: replace `eval_global_as_array_length` with type/interpreter evaluation (noir-lang/noir#6469)
chore: refactor `DataFlowGraph.insert_instruction_and_results` (noir-lang/noir#6823)
chore(docs): updating noirjs tutorial for 1.0.0 (noir-lang/noir#6792)
feat: Sync from aztec-packages (noir-lang/noir#6824)
chore: Have rust-analyzer use the stable toolchain (noir-lang/noir#6825)
  • Loading branch information
AztecBot committed Dec 18, 2024
2 parents dedd328 + 769aa20 commit db69894
Show file tree
Hide file tree
Showing 30 changed files with 80 additions and 8 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
f037c36f6bfcb8efb1950e444b50bc3eff28ffc4
73ccd45590222fc82642a6a9aa657c2915fc2c58
20 changes: 20 additions & 0 deletions noir/noir-repo/.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.
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.
Empty file.
Empty file.
38 changes: 38 additions & 0 deletions noir/noir-repo/.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 noir/noir-repo/.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
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,15 @@ impl Function {
}
}

let is_unconstrained = matches!(self.runtime(), RuntimeType::Brillig(_)).into();
let is_within_unconstrained = self.dfg.make_constant(is_unconstrained, NumericType::bool());
for instruction_id in is_unconstrained_calls {
let call_returns = self.dfg.instruction_results(instruction_id);
let original_return_id = call_returns[0];

// We replace the result with a fresh id. This will be unused, so the DIE pass will remove the leftover intrinsic call.
self.dfg.replace_result(instruction_id, original_return_id);

let is_unconstrained = matches!(self.runtime(), RuntimeType::Brillig(_)).into();
let is_within_unconstrained =
self.dfg.make_constant(is_unconstrained, NumericType::bool());
// Replace all uses of the original return value with the constant
self.dfg.set_value_from_id(original_return_id, is_within_unconstrained);
}
Expand Down
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

0 comments on commit db69894

Please sign in to comment.