Add Arbitrary to Glob
#2720
Add Arbitrary to Glob
#2720
Conversation
There was a problem hiding this comment.
Overall LGTM, with a couple changes.
Note that I am working on a rewrite of globset and it's not obvious to me that maintaining this Arbitrary impl will be straight-forward. So this might get lost in a future upgrade that will probably happen this year.
(I also resisted adding this to regex-syntax pretty hard, but it turned out that this was the least bad option after a lot of discussion.)
| @@ -21,6 +21,7 @@ bench = false | |||
|
|
|||
| [dependencies] | |||
| aho-corasick = "1.1.1" | |||
| arbitrary = { version = "1.3.2", optional = true, features = ["derive"] } | |||
There was a problem hiding this comment.
I don't like implicitly depending on crate names for features. Could you explicitly add an arbitrary feature? Example: https://github.com/rust-lang/regex/blob/0c0990399270277832fbb5b91a1fa118e6f63dba/regex-syntax/Cargo.toml#L18
There was a problem hiding this comment.
Please also add docs for this feature in the crate docs, e.g., https://docs.rs/regex-syntax/latest/regex_syntax/#crate-features
There was a problem hiding this comment.
Would you be ok with me adding a fuzz folder with a simple fuzz test?
There was a problem hiding this comment.
Probably fine. But it needs to be tested in CI.
There was a problem hiding this comment.
Thanks @BurntSushi. I've setup a fuzz directory and added a fuzz_testing action to ci.yml. Let me know if you want to adjust the duration for the fuzz testing as this can be modified. I've also addressed the other comments above.
Derive `Arbitrary` for `Glob` struct. This feature is optional so the derive will only take place when the feature is enabled. This feature is mandatory when using Glob in fuzz testing. Signed-off-by: William Johnson <[email protected]>
johnsonw
force-pushed
the
johnsonw/add-arbitrary-to-glob
branch
from
3b62d1a
to
579ef12
Compare
- Add CI Action to run fuzz testing - Add `arbitrary` feature in globset Cargo.toml - Add description for `Crate Features` in Globset Signed-off-by: William Johnson <[email protected]>
Signed-off-by: William Johnson <[email protected]>
Signed-off-by: William Johnson <[email protected]>
Signed-off-by: William Johnson <[email protected]>
|
Happy Friday @BurntSushi, I just wanted to check and see if there is anything else you would like me to address? |
.github/workflows/ci.yml
Outdated
| sudo apt-get update | ||
| sudo apt-get install g++ --yes | ||
|
|
||
| - uses: dtolnay/rust-toolchain@nightly |
There was a problem hiding this comment.
Is nightly needed here? If not, please use @master like the rest of the jobs.
There was a problem hiding this comment.
If we are not concerned with running the fuzz tests in CI then we don't need the nightly build. I'll change this to match the others.
|
|
||
| - name: Install packages (Ubuntu) | ||
| run: | | ||
| sudo apt-get update |
There was a problem hiding this comment.
libFuzzer requires LLVM sanitizer support and a C++ compiler with C++ 11 support. It is required to install the fuzzer and to compile the targets. Please see https://rust-fuzz.github.io/book/cargo-fuzz/setup.html for more information.
| working-directory: fuzz | ||
|
|
||
| - run: ./scripts/run-all.sh | ||
| working-directory: fuzz |
There was a problem hiding this comment.
For CI, I don't think we need to actually run fuzzing. Let's just make sure all of the fuzz targets build.
There was a problem hiding this comment.
Sounds good. I'll update this to simply run a cargo check in the fuzz directory.
instead of actually running the tests. - Fix "Crate Features" documentation - Update lines to have a maximum of 79 columns in README.md - Remove unnecessary files. - Cleanup fuzz_glob test. Signed-off-by: William Johnson <[email protected]>
|
Good afternoon, I just wanted to follow up on this. Let me know if there is anything else I need to address. |
|
I normally take a long time to review things. |
Derive
ArbitraryforGlobstruct. This feature is optional so the derive will only take place when the feature is enabled. This feature is mandatory when using Glob in fuzz testing.