-
Notifications
You must be signed in to change notification settings - Fork 541
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
Move benchmarks to a separate crate #1243
Conversation
Codecov Report
@@ Coverage Diff @@
## 0.4.x #1243 +/- ##
=======================================
Coverage 85.82% 85.82%
=======================================
Files 37 37
Lines 13517 13517
=======================================
Hits 11601 11601
Misses 1916 1916 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
459ab7e
to
c71f9ba
Compare
I think we should move criterion to be a dev-dependency instead and make it a proper benchmark within the main crate. Then we can restrict our MSRV test to only check |
I gave that another try this morning, but it doesn't seem to work: #1245 |
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.
Okay, let's do it. Can you rename the top-level crate to bench
so it's slightly less confusing to have benches/benches
?
benches/Cargo.toml
Outdated
version = "0.1.0" | ||
edition = "2021" | ||
|
||
# Even as a `dev-dependency` Criterion and its dependencies can affect the MSRV of chrono. |
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 is a really informative comment. 👍
e82177f
to
1d80bf4
Compare
1d80bf4
to
616cd49
Compare
As reported in #1223 a security advisory for
atty
shows up when we resolve the one for time 0.1.By moving the benchmarks to a separate crate criterion and its dependencies no longer affect our MSRV or can cause advisories, and criterion can be updated to 5.1.
I needed aEdit: doesn't work.lib.rs
file to have a valid crate, and we had a tiny one inci/core-test
. It seemed like a nice opportunity to merge them.Fixes #1223, alternative to #1224.