-
Notifications
You must be signed in to change notification settings - Fork 1
Add Settings::from_file/from_str and socket-address tests (#264) #271
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
+ Coverage 10.19% 10.96% +0.77%
==========================================
Files 8 7 -1
Lines 628 529 -99
==========================================
- Hits 64 58 -6
+ Misses 564 471 -93 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @kimhanbeom — friendly reminder about PR #271: "Add Settings::from_file/from_str and socket-address tests (#264)". This PR was approved by pott-cho on 2025-12-30, all CI checks are passing, and the branch is mergeable and not a draft. It adds tests, a dev-dependency (tempfile), and updates Cargo.lock; it also closes #264. It's been over two weeks since approval and the PR is still mergeable with no pending change requests. Could you please merge this when convenient, or let me know if there's a reason to hold (release timing or other policy)? If you'd like, I can rebase or make any tiny follow-ups first — just tell me how you'd prefer to proceed. Thanks! |
Cargo.toml
Outdated
| unwrap_used = "warn" | ||
|
|
||
| [dev-dependencies] | ||
| tempfile = "3.24.0" |
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.
@pott-cho If a crate is not dependent on a specific version, we should specify only the major version.
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.
I've updated it.
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.
From the test code, I noticed that from_file returns a ConfigError, while from_str (the FromStr implementation) returns anyhow::Error. While this isn't a bug, I believe it's a point where we need to achieve consistency. Could you please create an improvement issue to address this?
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.
The issue has been created: #277
a6f8020 to
856506c
Compare
This PR adds comprehensive tests for the Settings configuration parsing and socket-address deserialization to satisfy issue #264.
What I changed
Tests added (high level)
Notes on test design
Files changed
Why this is needed
These tests ensure Settings parsing and socket-address deserialization behave correctly across expected inputs and failure modes, and they guard future changes by asserting stable, user-friendly error messages.
Closes #264
Related issue: #264