search

sirwart / ripsecrets

A command-line tool to prevent committing secret keys into your source code
MIT License
800 stars 24 forks source link

[rebase] Add a benchmark using Criterion #43

Closed lafrenierejm closed 2 years ago

lafrenierejm commented 2 years ago

Attempt to rebase #25 onto the current HEAD of main, 6feb4f7dccc6834877fe61dcb94cbb547a55a176.

Copying the first part of @sts10's PR description:

Given this project's emphasis on performance, I thought it'd be nice to have some built-in benchmarking. This way, contributors can easily check if their changes improve or hinder performance against a relatively standardized test (of course, absolute values will vary machine-to-machine).

As a first attempt at this, I decided to use a Rust library Criterion. I hit some issues right away, though: Criterion makes it pretty difficult to benchmark functions that are not in src/lib.rs (see this Stack Overflow Q&A).

Because of this, I decided to take the rather aggressive step of renaming src/find_secrets.rs -> src/lib.rs. I think this has benefits beyond easy benchmarking; for example, if someone wanted to use ripsecrets as a Rust library, I think it makes sense to have a lib.rs file.

lafrenierejm commented 2 years ago

I'm a newb with regard to Rust. cargo test is passing, but that's really the only source of confidence I have that I performed this rebase correctly. This should definitely be scrutinized before it's ever considered for acceptance.

sts10 commented 2 years ago

This PR seems good to me (and so clean!): On my machine, cargo test passes as well, and cargo bench runs fine. I also did a cargo run --release -- ../test-directory and it found the same two secrets that the existing ripsecrets version 1.5.0 finds.

But given that we're changing 11 files, I wouldn't mind having more eyes on this.

lafrenierejm commented 2 years ago

But given that we're changing 11 files, I wouldn't mind having more eyes on this.

+1

Tagging @janriemer because you had participated some in #25. No pressure at all, just in case you're interested and available to review.

sirwart commented 2 years ago

Even though it's 11 files it looks like there are no logic changes, so this looks safe to merge to me. Also, running the main commands (cargo {bench, test, build}) all worked for me. Thanks for writing @sts10 and for rebasing it @lafrenierejm!