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: test error spans and messages #7526

Merged
merged 9 commits into from
Feb 26, 2025
Merged

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Feb 25, 2025

Description

Problem

Resolves #7455

Summary

Introduces a check_errors helper so we can to be used in frontend tests. Here's an example:

let src = "
fn main() -> pub i32 {
                 ^^^ expected i32 because of return type
    true        
    ~~~~ bool returned here
}
";
check_errors(src);

The idea is that "^^^" lines mark primary errors and "~~~" mark secondary errors.

Pros:

  1. (in my opinion) It becomes easier to write these tests as we no longer need a long sequence of code to test thngs
  2. The tests also become easier to understand as they include the error messages and their locations
  3. We get to test error locations and make sure that's where we want them to be
  4. New tests can be written, and be compiled, before the expected error is implemented
  5. Multiple errors can be tested at once
  6. The tests become more resilient to code refactors: if we change the structure of errors it would just affect the check_errors function
  7. Tests are shorter (check the diff: +864, -2010)

Cons:

  1. We no longer check the underlying type of the error, which might have been desired (not sure!)
  2. We can no longer copy the test source code to a file and compile that without first removing the error squiggles (though this wasn't always possible before this PR, for example if a trait Default was introduced in a test it would conflict with the existing Default trait, etc.)

Additional Context

I decided to translate all tests here because after I started working on this I wanted to see it through, but also because I believe it would make things easier from now on, and we can also move faster.

As I translated tests I put some "TODO"s about small things to improve. Once this PR merges I'll create a follow-up issue to tackle those TODOs (here I decided not to change the compiler's code except in a couple of places where testing like this was impossible because of small bugs or unpredictability of the error messages).

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.

@asterite asterite force-pushed the ab/test-error-spans-and-messages branch from 3c1568b to 0857654 Compare February 25, 2025 23:34
@asterite asterite marked this pull request as draft February 25, 2025 23:37
@asterite asterite requested a review from a team February 25, 2025 23:37
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: 0857654 Previous: f0c1c7b Ratio
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types 73 s 56 s 1.30

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

CC: @TomAFrench

@asterite asterite marked this pull request as ready for review February 26, 2025 18:10
@asterite asterite requested review from a team and removed request for a team February 26, 2025 18:20
@asterite
Copy link
Collaborator Author

(re-requested a review because previously it was a draft, now it's ready)

@jfecher
Copy link
Contributor

jfecher commented Feb 26, 2025

Seeing this I can't help but wonder if we should have just started with golden tests all along

@asterite
Copy link
Collaborator Author

Ah, yes, using golden tests would also work. I guess we'd need to put frontend tests in a way similar to the ones in test_programs, right? Then, it wouldn't be as easy to write a test before introducing a new error, but it's not a big deal.

Another thing is that in these tests one can easily see the code and the errors at once. With golden tests I guess the code will be in one file and the error output will be in another one... which isn't a big deal either, though.

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM.
This is moving us closer to a golden-tests style testing which is definitely more ergonomic to write tests for.

@jfecher
Copy link
Contributor

jfecher commented Feb 26, 2025

Ah, yes, using golden tests would also work. I guess we'd need to put frontend tests in a way similar to the ones in test_programs, right? Then, it wouldn't be as easy to write a test before introducing a new error, but it's not a big deal.

Yeah, they'd probably need to be in a different folder to be more discoverable. You should still be able to write the test before the fix though. The program would just error in a different way (presumably) similar to running the frontend test today before the feature is implemented.

Another thing is that in these tests one can easily see the code and the errors at once. With golden tests I guess the code will be in one file and the error output will be in another one... which isn't a big deal either, though.

No, the code and output would be in the same file (see the examples of that repository I linked)

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: 8cb8e0d Previous: 3869fe9 Ratio
private-kernel-tail 1.218 s 0.988 s 1.23

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

CC: @TomAFrench

@TomAFrench
Copy link
Member

We no longer check the underlying type of the error, which might have been desired (not sure!)

We can go some part of the way towards this with #7461 (but granted, we still lose full pattern matching)

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM

@asterite
Copy link
Collaborator Author

We can go some part of the way towards this with #7461 (but granted, we still lose full pattern matching)

I guess once we add error codes we can include them like "^^^ [E1234] some message" and parse that (another big refactor, but a much simpler one)

@asterite asterite added this pull request to the merge queue Feb 26, 2025
@TomAFrench
Copy link
Member

Yep, exactly

Merged via the queue into master with commit da5227e Feb 26, 2025
101 of 102 checks passed
@asterite asterite deleted the ab/test-error-spans-and-messages branch February 26, 2025 20:27
TomAFrench added a commit that referenced this pull request Feb 26, 2025
* master:
  feat: test error spans and messages (#7526)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 26, 2025
chore: revert bump of base64ct to `v1.6.0` after `v1.7.0` yanked (noir-lang/noir#7541)
feat(performance): Avoid extra Brillig array offsetting for constant indices  (noir-lang/noir#7522)
feat: test error spans and messages (noir-lang/noir#7526)
chore: bump wasm-bindgen (noir-lang/noir#7535)
chore: parallelize `test_transform_program_is_idempotent` (noir-lang/noir#7539)
chore(cli): Use `noir_artifact_cli::fs` to read artifacts (noir-lang/noir#7391)
fix(experimental): Replace most remaining match panics with errors (noir-lang/noir#7536)
chore: bump external pinned commits (noir-lang/noir#7537)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request Mar 4, 2025
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore!: bump rust edition to 2024
(noir-lang/noir#7533)
chore: revert bump of base64ct to `v1.6.0` after `v1.7.0` yanked
(noir-lang/noir#7541)
feat(performance): Avoid extra Brillig array offsetting for constant
indices (noir-lang/noir#7522)
feat: test error spans and messages
(noir-lang/noir#7526)
chore: bump wasm-bindgen (noir-lang/noir#7535)
chore: parallelize `test_transform_program_is_idempotent`
(noir-lang/noir#7539)
chore(cli): Use `noir_artifact_cli::fs` to read artifacts
(noir-lang/noir#7391)
fix(experimental): Replace most remaining match panics with errors
(noir-lang/noir#7536)
chore: bump external pinned commits
(noir-lang/noir#7537)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <tom@tomfren.ch>
Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: guipublic <guipublic@gmail.com>
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.

Improve testing error locations
3 participants