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

chore: generate compilation tests from frontend tests #7753

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

michaeljklein
Copy link
Contributor

@michaeljklein michaeljklein commented Mar 19, 2025

Description

Problem*

Frontend tests are run in an environment that could become out of sync with nargo compile.

Summary*

During get_program_with_options and similar, we can:

  • Generate a crate name by hashing the source from the test name
  • Use the source as the contents of main.nr
  • Generate a simple Nargo.toml
  • Only regenerate when the source test changes

Note: when the cached version is out of date in CI, there's an error like the following:

--- STDERR:              noirc_frontend tests::traits::trait_alias_with_where_clause_has_equivalent_errors ---

thread 'tests::traits::trait_alias_with_where_clause_has_equivalent_errors' panicked at compiler/noirc_frontend/src/tests.rs:254:17:
test generated from frontend unit test noirc_frontend::tests::traits::trait_alias_with_where_clause_has_equivalent_errors is out of date: run `cargo test` to update

Additional Context

Questions:

  1. My current approach is to generate the tests during get_program, but is there a better way to get this path than using the CARGO_MANIFEST_DIR? I considered trying to generate using build.rs, but I'm not sure how to do that without a two-step process of a) generating ephemeral output files when running the noirc_frontend tests, b) converting the ephemeral files to test_programs crates with build.rs.
    // "compiler/noirc_frontend"
    let noirc_frontend_path = Path::new(std::env!("CARGO_MANIFEST_DIR"));
  1. The following snippet is from nargo_cli/src/cli/init_cmd: is there a sane way to import this without cyclic dependencies or a bunch of refactoring?
        let package_type = "bin"; // nargo::package::PackageType::Binary;
        let toml_contents = format!(
            r#"
            [package]
            name = "{package_name}"
            type = "{package_type}"
            authors = [""]
            
            [dependencies]"#
        );

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@michaeljklein michaeljklein changed the title Convert frontend tests to compilation tests chore: convert frontend tests to compilation tests Mar 21, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 0f1faeb Previous: 6f0b119 Ratio
zkemail_noir-jwt_ 5 s 3 s 1.67

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@michaeljklein michaeljklein changed the title chore: convert frontend tests to compilation tests chore: generate compilation tests from frontend tests Mar 21, 2025
@michaeljklein michaeljklein marked this pull request as ready for review March 25, 2025 19:18
@michaeljklein michaeljklein requested a review from a team March 25, 2025 19:22
@jfecher
Copy link
Contributor

jfecher commented Mar 27, 2025

How many of the errors caught by this PR would also be caught by adding monomorphizing to the get_program test function? It seems like a lot of machinery to add macros for updating all these generated files. I think if we can't get the existing test structure working with changes like now monomorphizing them, then I'd prefer us to just commit to moving these tests to dedicated test_programs instead of housing them in both locations & having CI check the source matches the generated version - at least I'm not sure what the advantages of the generated+CI approach would be.

@jfecher
Copy link
Contributor

jfecher commented Mar 27, 2025

Discussed in standup: we're going to keep the current approach since it has things we need now like more testing for the formatter and future support for the fuzzer. We may revisit it in the future but for now we'll keep the CI check as well to ensure the generated tests are up to date.

Edit: compared to the test_programs only approach, this lets us keep writing unit tests for now which can be easier to debug due to having no stdlib.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 0f1faeb Previous: 6f0b119 Ratio
rollup-block-root 148 s 120 s 1.23

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants