-
Notifications
You must be signed in to change notification settings - Fork 745
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
Fuzzer: Use a directory for important fuzz testcases #6297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable, although one thing to note is that that may make reproducibility slightly worse since the fuzz results will now depend on the contents of the fuzz directory.
scripts/fuzz_opt.py
Outdated
@@ -1402,6 +1388,8 @@ def handle(self, wasm): | |||
lit_tests = shared.get_tests(shared.get_test_dir('lit'), test_suffixes, recursive=True) | |||
all_tests = core_tests + passes_tests + spec_tests + wasm2js_tests + lld_tests + unit_tests + lit_tests | |||
|
|||
fuzz_cases = shared.get_tests(shared.get_test_dir('fuzz'), test_suffixes, recursive=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't break if the fuzz
directory doesn't exist, will it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll add a readme.txt in that directory to force it to exist in git.
I'm not strongly opinionated, but wouldn't it be simpler test directories are categorized based on their kinds ( |
I'll clarify in a comment more what I intend here, I think I wasn't clear enough. The new directory is for manually adding new fuzz testcases for the fuzzer - we wouldn't move any existing testcases there, and probably would not even commit anything there. But when fuzzing locally the user can drop a few files in there that get coverage that way. |
That makes sense. Thanks for the explanation! |
After thinking a little I think maybe it is clearer to make the directory not exist and document the feature as being for local changes, that is, now the docs say "If you want to fuzz some files with high importance, create 'fuzz' and put the files there". That's the intended use case so we might as well describe it that way. |
The advantage of checking in the directory with a readme is that it makes this feature and its documentation more discoverable, but either way sgtm. |
Makes sense, maybe the important thing is to keep the fuzz dir parallel to the test dir, so it's clear normal testcases do not go there. I did that now and clarified in the readme there. |
Also documented in https://github.com/WebAssembly/binaryen/wiki/Fuzzing#helper-scripts |
Users can put files in ./fuzz and they will be fuzzed with high priority. Docs in source and https://github.com/WebAssembly/binaryen/wiki/Fuzzing#helper-scripts
This removes the manual list inside the fuzzer with a simple directory that we
scan. The manual list was not well-maintained and nothing there really
mattered enough to keep. Instead, users can place files in
/test/fuzz/
that theyconsider important.