Skip to content
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

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

1996fanrui
Copy link
Contributor

@1996fanrui 1996fanrui commented Aug 10, 2024

close #34

Changes:

  • refactor: move Rotation into one separate module and add test for it
    • The rolling file has too many code, so move Rotation into one separate module
  • test: add unit tests for rolling file
    • Introduce the Clock related code to test time rotation

@1996fanrui 1996fanrui force-pushed the 34/rolling-file-test branch from f0cfc57 to 7ec331f Compare August 10, 2024 23:39
Copy link
Contributor Author

@1996fanrui 1996fanrui left a 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.


/// Defines a fixed period for rolling of a log file.
#[derive(Clone, Eq, PartialEq, Debug)]
pub enum TimeRotation {
Copy link
Contributor

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.

Comment on lines 63 to 64
cargo test -- --nocapture
cargo test --features rolling_file -- --nocapture
Copy link
Contributor

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

prefix: None,
suffix: None,
max_size: usize::MAX,
max_files: None,
clock: None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just default to DefaultClock?

Copy link
Contributor Author

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.

Copy link
Contributor

@tisonkun tisonkun left a 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.

  1. You may try to add cases that rotates by both time and size;
  2. You can run cargo +nightly fmt --all to apply all the format rules (some of them are unavailable in the stable channel).

@1996fanrui 1996fanrui force-pushed the 34/rolling-file-test branch from 7ec331f to e8b34f3 Compare August 11, 2024 06:55
@1996fanrui 1996fanrui force-pushed the 34/rolling-file-test branch from e8b34f3 to 9b98068 Compare August 11, 2024 06:59
@1996fanrui
Copy link
Contributor Author

Thanks for your contribution!

Comments above. Rest LGTM.

1. You may try to add cases that rotates by both time and size;

2. You can run `cargo +nightly fmt --all` to apply all the format rules (some of them are unavailable in the stable channel).

Thanks for the review, all comments are addressed. And run cargo +nightly fmt --all before commit.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the unit test for rolling file
2 participants