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: raise library test coverage #349

Closed
wants to merge 25 commits into from
Closed

Conversation

bconn98
Copy link
Collaborator

@bconn98 bconn98 commented Feb 17, 2024

Fixes #352 + Adds a ton of tests

@bconn98 bconn98 requested review from estk and gadunga as code owners February 17, 2024 18:11
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.61%. Comparing base (c981ca4) to head (4a56b4e).

Files Patch % Lines
src/encode/json.rs 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #349       +/-   ##
===========================================
+ Coverage   63.01%   84.61%   +21.59%     
===========================================
  Files          24       24               
  Lines        1560     1566        +6     
===========================================
+ Hits          983     1325      +342     
+ Misses        577      241      -336     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bconn98 bconn98 changed the title draft: test: add tests to check coverage draft: test: raise library test coverage Feb 17, 2024
@bconn98 bconn98 force-pushed the add_all_tests branch 3 times, most recently from 1a1ccd8 to 3d157d5 Compare February 24, 2024 21:10
@bconn98 bconn98 force-pushed the add_all_tests branch 14 times, most recently from aadede8 to 0b8215d Compare February 29, 2024 03:24
@bconn98 bconn98 changed the title draft: test: raise library test coverage test: raise library test coverage Feb 29, 2024
@bconn98 bconn98 force-pushed the add_all_tests branch 6 times, most recently from b449689 to 3bf0d57 Compare March 1, 2024 03:10
Copy link
Collaborator Author

@bconn98 bconn98 left a comment

Choose a reason for hiding this comment

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

Ready for review

1. Ensure the archives are small enough that the compression time is acceptable.

For more information see the PR that added [`background_rotation`](https://github.com/estk/log4rs/pull/117).

Copy link
Owner

Choose a reason for hiding this comment

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

why rm?

Copy link
Owner

Choose a reason for hiding this comment

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

so, i just pulled the thread and although gzip is no-longer part of the default features, I still think a warning regarding the use of gzip is important to be featured in the readme... perhaps not as prominently. Also, this change should be pulled out into a separate PR.

@estk
Copy link
Owner

estk commented Mar 2, 2024

I think we should break this up into a few separate PR's, there are too many disparate changes here to review it without sitting down for at least a few hours... which is difficult to find these days.

Can you please open separate PR's with cohesive motif's? Some possible MR's

  • Config tests
  • Encoder tests
  • Filter tests
  • Integration tests
  • Appender tests
  • Compound appender tests

In each PR please:

  1. Call out any changes to code outside of mod test & why its needed
  2. for each test, if it is not obvious why it needs to exists, please add a brief description

@bconn98
Copy link
Collaborator Author

bconn98 commented Mar 3, 2024

Broke into the following PRs:

@bconn98 bconn98 closed this Mar 3, 2024
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.

README out of date with the 1.0 version
3 participants