Skip to content

chore(ci): add tests to explicitly run through regression seeds#9959

Closed
TomAFrench wants to merge 4 commits intomasterfrom
tf/fuzzing-regressions
Closed

chore(ci): add tests to explicitly run through regression seeds#9959
TomAFrench wants to merge 4 commits intomasterfrom
tf/fuzzing-regressions

Conversation

@TomAFrench
Copy link
Member

Description

Problem*

Resolves #9852
Resolves #9851
Resolves #9873

Summary*

I was a bit concerned about how in #9957 it's not immediately clear from github that running the fuzzers with the reproduction seed would succeed.

This PR adds a fuzzing-regressions.txt file which contains a list of seeds to run through all the various fuzzer flavours as an easy way to prove that a particular fuzzing error has been resolved. Adding the seed from #9957 fails as expected and I've added the seeds from the listed issues which ended up passing.

Additional Context

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.

@TomAFrench TomAFrench requested a review from aakoshh September 23, 2025 10:27
@aakoshh
Copy link
Contributor

aakoshh commented Sep 23, 2025

and I've added the seeds from the listed issues which ended up passing.

That surprises me, as I am not aware that we have fixed this error.

The problem with trying the seeds on anything but that exact commit, is that the AST generation itself could have changed. For instance #9957 adjusts the frequencies of some of the generators in the fuzzer, because I got some calibration test failures in #9948. By doing so, the seed probably now results in a different AST being generated, which avoids the original bug. So I would not conclude that this PR resolves the tickets in the description.

I just checked with the actual minimised code that I captured in #9852 and to my astonishment the difference between Brillig and ACIR did disappear, probably due to the recent changes in remove_unreachable_instruction, however #9851 still fails on master.

@aakoshh
Copy link
Contributor

aakoshh commented Sep 23, 2025

Unfortunately I can't say about #9872 because @michaeljklein didn't capture the commit and I also didn't log the code I saw when I made my comment (I didn't try to minimise this, just ran whatever AST it was and looked at which SSA step failed).

@TomAFrench TomAFrench disabled auto-merge September 23, 2025 10:53
@TomAFrench
Copy link
Member Author

That surprises me, as I am not aware that we have fixed this error.

Yeah, this was a surprise to me as well.

The problem with trying the seeds on anything but that exact commit, is that the AST generation itself could have changed.

Ah, yeah that's pretty annoying and I was relying on the seed reproducing the same AST so this PR isn't fit for purpose as a result.

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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 6d9d070 Previous: f355e1d Ratio
rollup-root 0.006 s 0.004 s 1.50

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

CC: @TomAFrench

@aakoshh
Copy link
Contributor

aakoshh commented Sep 23, 2025

We could use it if we for example captured the git hash of the ast_fuzzer subdirectory, put that in the file with the seeds, and then as long as that hasn't changed, we use the seeds, then as the ast_fuzzer moves on, we stop using them and we can prune them eventually.

@TomAFrench
Copy link
Member Author

I just checked with the actual minimised code that I captured in #9852 and to my astonishment the difference between Brillig and ACIR did disappear, probably due to the recent changes in remove_unreachable_instruction, however #9851 still fails on master.

I've added a regression test here: #9960

@aakoshh
Copy link
Contributor

aakoshh commented Sep 23, 2025

Also noting from Michael's way of running stuff is that potentially this can be executed from a script by looping over the lines in the file, and then calling NOIR_AST_FUZZER_SEED=$SEED cargo test -q -p noir_ast_fuzzer_fuzz vs which runs all targets with that seed. (Maybe easier to stitch together with git commands in bash?)

@TomAFrench TomAFrench closed this Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants