Reworking the Noir compiler test suite [WIP]

TomAFrench commented 1 year ago

Problem

Submitting this so I don't lose it but I'll continue fleshing this out later today

The Noir compiler integration test suite has grown to be quite large and we're treating all the different test cases in the same way, i.e. we attempt to compile and execute every single circuit.

A number of circuits we expect to not be able to compile at all. The test suite does not distinguish between this and the circuit compiling but failing to execute.
We don't test the actual ACIR generated by the Noir compiler except that it's a valid to the ACVM. Noir should aim for good ACIR, not just valid ACIR:
1. See https://github.com/noir-lang/noir/pull/1989 for an example of an issue which could be easily prevented by an automated check of the emitted ACIR to reject opcodes of the form witness - constant = 0
We're passing a lot of empty circuits to Barretenberg to test that it can create proofs for them. Checking that Barretenberg can create proofs for circuits with no constraints aren'tthe most rigorous checks we could perform.

Happy Case

We should split up our test suite into the following categories which all live in different directories for ease of manipulation.

compile-failure: circuits which are not valid or unsatisfiable Noir code and so the compiler should reject.
compile-success: circuits which are valid satisfiable Noir code but have no arguments.
execution-success: circuits which are valid Noir satisfiable code and have arguments.
test-success: Noir programs which are written as #[test] cases.

execution-success and compile-success are distinct as compile-success is expected to compile down to an empty circuit. It is then not useful to test its execution or pass this to barretenberg as a test case.

The new testing flow would look something like this:

flowchart TD

    subgraph test_data/compile-failure
        A1[Attempt to compile] --> A2[Assert compilation fails]
    end

    subgraph test_data/compile-success
        B1[Attempt to compile] --> B2[Assert compilation succeeds]
        B2 --> B3[Write circuit to file]
        B3 --> B4[Assert empty circuit]
    end

    subgraph test_data/execution-success
        C1[Attempt to compile] --> C2[Assert compilation succeeds]
        C2 --> C3[Write circuit to file]
        C3 --> C4[Assert execution succeeds]
        C4 --> C5[write witness to file]

        C6[Publish witness + circuit as artifact]
        C3 --> C6
        C5 --> C6

        C3 ----> C7[Circuit optimisation checks]
    end

    subgraph test_data/test-success
        D1[Assert `nargo test` succeeds]
    end

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

Yes

Support Needs

No response

vezenovm commented 1 year ago

Part of this rework could also include the ability to have Brillig foreign calls that are purely for tests. As per issue #1911 we currently have oracles that exist as nargo builtins when they do not need to be. Other callers (aside nargo like aztec's acvm_js) also have many oracles of their own that they need to be able to call for tests. Some sort of ability to mock oracle calls for tests would be very useful.

vezenovm commented 1 year ago

Linking this issue for visibility: https://github.com/noir-lang/noir/issues/2036

TomAFrench commented 1 year ago

This issue being addressed would also make potential optimisations such as https://github.com/noir-lang/noir/issues/2189 painfully obvious.

TomAFrench commented 1 year ago

Workflow to build nargo CLI in https://github.com/noir-lang/noir/issues/2013 can be repurposed to provide the nargo cli to use for all of this compilation, etc.

TomAFrench commented 1 year ago

I think we can consider this closed. I'll break out issues for the extra checks we can perform.

noir-lang / noir