-
Notifications
You must be signed in to change notification settings - Fork 164
feats: extend ValuePool by new fields : files, and maxMutations #1033
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
base: main
Are you sure you want to change the base?
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.
Pull request overview
This PR extends the @ValuePool annotation to support two new capabilities: (1) loading files as byte arrays using glob patterns via a new files field, and (2) controlling mutation iterations via a new maxMutations field.
Changes:
- Added
filesfield to@ValuePoolannotation for glob-based file loading - Added
maxMutationsfield to control mutation iterations on pool values - Implemented file pattern matching and path resolution in
ValuePoolRegistry - Updated mutation logic to apply multiple mutations based on
maxMutations - Added caching for ArgumentsMutator instances and file reads
- Comprehensive test coverage for file loading feature
- Extensive documentation updates
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/code_intelligence/jazzer/mutation/annotation/ValuePool.java | Added files and maxMutations fields to annotation with documentation |
| src/main/java/com/code_intelligence/jazzer/mutation/support/ValuePoolRegistry.java | Implemented glob pattern matching, file loading, and maxMutations extraction with path resolution logic |
| src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ValuePoolMutatorFactory.java | Updated mutate/crossOver to apply multiple mutations based on maxMutations |
| src/test/java/com/code_intelligence/jazzer/mutation/support/ValuePoolsTest.java | Added comprehensive tests for file loading patterns and updated test helpers |
| src/main/java/com/code_intelligence/jazzer/mutation/ArgumentsMutator.java | Added caching for ArgumentsMutator instances to improve performance |
| src/main/java/com/code_intelligence/jazzer/junit/FuzzTestExecutor.java | Set system property for baseDir to enable ValuePoolRegistry file resolution |
| docs/mutation-framework.md | Added extensive documentation for ValuePool features including examples and usage patterns |
Comments suppressed due to low confidence (1)
src/test/java/com/code_intelligence/jazzer/mutation/support/ValuePoolsTest.java:415
- The test helper withValuePoolImplementation is missing the maxMutations() method implementation. Since maxMutations() is now part of the @valuepool annotation interface, this anonymous implementation must override it. Without this method, using this test helper will result in runtime errors when the annotation's maxMutations() method is called.
return new ValuePool() {
@Override
public String[] value() {
return value;
}
@Override
public String[] files() {
return files;
}
@Override
public double p() {
return p;
}
@Override
public String constraint() {
return constraint;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/code_intelligence/jazzer/mutation/support/ValuePoolRegistry.java
Outdated
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/annotation/ValuePool.java
Outdated
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/support/ValuePoolRegistry.java
Outdated
Show resolved
Hide resolved
src/test/java/com/code_intelligence/jazzer/mutation/support/ValuePoolsTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ValuePoolMutatorFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/support/ValuePoolRegistry.java
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ValuePoolMutatorFactory.java
Show resolved
Hide resolved
ed09c59 to
e2cc773
Compare
42a3a15 to
6d874ba
Compare
6d874ba to
112ab04
Compare
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/com/code_intelligence/jazzer/mutation/support/ValuePoolsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/code_intelligence/jazzer/mutation/support/ValuePoolsTest.java
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/support/ValuePoolRegistry.java
Outdated
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/support/ValuePoolRegistry.java
Outdated
Show resolved
Hide resolved
src/test/java/com/code_intelligence/jazzer/mutation/support/ValuePoolsTest.java
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ValuePoolMutatorFactory.java
Show resolved
Hide resolved
src/main/java/com/code_intelligence/jazzer/mutation/mutator/lang/ValuePoolMutatorFactory.java
Outdated
Show resolved
Hide resolved
The number of fields in ValuePool is getting large, so that construction using positional args is becoming confusing. Using a builder simplifies the tests.
Don't generate the same mutator for the same fuzz test method multiple times. Before this change, a mutator was generated for each crash file in fuzzing mode.
112ab04 to
49403fd
Compare
Marcono1234
left a comment
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.
Hopefully these comments are useful. Feel free to consider them only as suggestions or ignore them completely.
Please let me know if these comments are not useful or if you consider them disruptive.
| // - Integer mutator for Map values in 'foo' | ||
| } | ||
|
|
||
| static Stream mySupplier() { |
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.
To avoid raw types?
| static Stream mySupplier() { | |
| static Stream<?> mySupplier() { |
| ### Complete Example | ||
| ```java | ||
| class MyFuzzTest { | ||
| static Stream edgeCases() { |
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.
To avoid raw types?
| static Stream edgeCases() { | |
| static Stream<?> edgeCases() { |
| return Stream.of( | ||
| "", "null", "alert('xss')", // Strings | ||
| 0, -1, Integer.MAX_VALUE, // Integers | ||
| new byte[]{0x00, 0xFF}, // A byte array |
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.
Causes a compilation error because 0xFF is an int literal > Byte.MAX_VALUE. Maybe choose something else for simplicity (avoiding a (byte) cast)?
| new byte[]{0x00, 0xFF}, // A byte array | |
| new byte[]{0x00, 0x7F}, // A byte array |
| public @interface ValuePool { | ||
| /** | ||
| * Specifies supplier methods that generate values for fuzzing the annotated method or type. The | ||
| * specified supplier methods must be static and return a {@code Stream <?>} of values. The values |
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.
| * specified supplier methods must be static and return a {@code Stream<?>} of values. The values |
(redundant space between Stream and <?>)
| String[] value() default {}; | ||
|
|
||
| /** | ||
| * Specifies glob patterns matching files that should be provided as {@code byte []} to the |
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.
Redundant space? (or is that intentional?)
| * Specifies glob patterns matching files that should be provided as {@code byte []} to the | |
| * Specifies glob patterns matching files that should be provided as {@code byte[]} to the |
| start = baseDir.toAbsolutePath().normalize(); | ||
| } else { // absolute | ||
| if (Paths.get(prefix).isAbsolute()) { | ||
| start = Paths.get(prefix); |
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.
Shouldn't this consider lastSeparator here as well?
| start = Paths.get(prefix); | |
| start = Paths.get(prefix.substring(0, lastSeparator)); |
Or define a local variable for this to avoid duplicating this code, respectively just reassign prefix as prefix = prefix.substring(0, lastSeparator)?
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.
I think the reason why this is not detected by the new unit tests is that they all create glob patterns where / is immediately followed by *, e.g. .../**.json. Maybe they should also do something like .../f*.json, that would probably trigger this bug.
| Path path = Paths.get(glob); | ||
| Path absolutePath = | ||
| path.isAbsolute() ? path : baseDir.toAbsolutePath().normalize().resolve(path); | ||
| return Files.isRegularFile(absolutePath) ? Stream.of(absolutePath) : Stream.empty(); |
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.
General question for here and below: In case no matching files were found (or an IO error occurs), would it be better to report that as error? That would make troubleshooting for users easier, and they would notice sooner when they renamed / deleted a value pool folder without adjusting the corresponding paths in the fuzz test.
| } else { | ||
| lastSeparator = prefix.lastIndexOf('/'); | ||
| } | ||
|
|
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.
FileSystem#getPathMatcher performs unescaping of \\. For consistency, would it therefore make sense to manually unescape it in prefix.substring(0, lastSeparator)? Otherwise it might be a bit inconsistent that in one part of the glob \ has to be escaped whereas in the other part it does not.
Note: It seems any character can be escaped, e.g. glob:a\b matches ab.
| assertThat( | ||
| assertThrows( | ||
| IllegalArgumentException.class, () -> valuePools.extractFirstMaxMutations(type)) | ||
| .getMessage()) | ||
| .contains("@ValuePool maxMutations must be >= 0"); |
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.
Might be more idiomatic to use assertThat(...).hasMessageThat()..., instead of calling getMessage() manually?
| assertThat(elements).isPresent(); | ||
| assertThat(elements.get()).containsExactly("value1", "value2", "value3"); | ||
| List<?> elements = valuePools.extractUserValues(type).collect(Collectors.toList()); | ||
| assertThat(elements).isNotEmpty(); |
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.
Maybe the isNotEmpty() followed by containsExactly(...) is redundant? Same also for the other test methods below.
(previously for Optional the isPresent() check made sense to avoid an exception when calling get())
@ValuePoolannotation by using the new fieldString[] files---files matching the patterns will be loaded asbyte []. The field uses theglob:syntax ofjava.nio.file.PathMatcher.Example:
maxMutations, the users can now control up to how many times (by default 1) values selected from@ValuePoolwill be mutated by the underlying mutator in one mutation operation.