-
Notifications
You must be signed in to change notification settings - Fork 7
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
test: add unit tests for rolling file #36
Conversation
f0cfc57
to
7ec331f
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.
Hey, I added a few tests for rolling file. Please let me know if we need more tests to cover more cases.
Also, it's my first time to use rust. Feel free to let me know if my change doesn't make sense, thanks a lot.
src/append/rolling_file/rotation.rs
Outdated
|
||
/// Defines a fixed period for rolling of a log file. | ||
#[derive(Clone, Eq, PartialEq, Debug)] | ||
pub enum TimeRotation { |
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.
Let's keep the name Rotation
as we don't introduce other rotation strategy. Also this is a public struct that changing its name would cause downstream software to migrate.
This PR adds tests only, and thus should avoid breaking changes.
.github/workflows/ci.yml
Outdated
cargo test -- --nocapture | ||
cargo test --features rolling_file -- --nocapture |
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.
You can use --all-features
, i.e.,
cargo test --all-features -- --nocapture
src/append/rolling_file/rolling.rs
Outdated
prefix: None, | ||
suffix: None, | ||
max_size: usize::MAX, | ||
max_files: None, | ||
clock: None, |
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.
Just default to DefaultClock
?
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.
Updated, and I simplified the Clock related code as well.
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.
Thanks for your contribution!
Comments above. Rest LGTM.
- You may try to add cases that rotates by both time and size;
- You can run
cargo +nightly fmt --all
to apply all the format rules (some of them are unavailable in the stable channel).
7ec331f
to
e8b34f3
Compare
e8b34f3
to
9b98068
Compare
Thanks for the review, all comments are addressed. And run |
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.
Thank you!
close #34
Changes: