-
Notifications
You must be signed in to change notification settings - Fork 533
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
Add more DateTime.into tests (branch 0.4.x) #1088
Conversation
c8614e4
to
17bbef7
Compare
7b4da00
to
0127fd7
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.
👍
Unrelated note:
I am somewhat surprised to two DateTime
s with different time zones compare equal, while for example their debug format is different. An Ord
implementation makes sense, but what do you think of this?
Not that it can be changed in 0.4.x.
They are the same instant just in different time zones. It makes intuitive sense to me. 🤷 |
I think we should first start to track coverage so we can judge whether this actually improves coverage. |
@jtmoon79 Do you want to rebase and see what the coverage is? |
af4d8a4
to
e282568
Compare
Codecov Report
@@ Coverage Diff @@
## 0.4.x #1088 +/- ##
==========================================
+ Coverage 91.24% 91.31% +0.06%
==========================================
Files 38 38
Lines 17062 17094 +32
==========================================
+ Hits 15568 15609 +41
+ Misses 1494 1485 -9
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
6df882c
to
48f88fc
Compare
From the Codecov report: Total increase
per file increase
IMO it's worthwhile to add. Up to you guys. |
codecov is now reporting
🤨 Is there a way to kick it? |
src/datetime/mod.rs
Outdated
} | ||
|
||
#[test] | ||
fn test_auto_conversion_fixedoffset_into_utc() { |
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.
Is this a simpler way to cover the same as this test and the following?
#[cfg(feature = "clock")]
#[test]
fn test_auto_conversion() {
let dt = NaiveDate::from_ymd_opt(2018, 9, 5).unwrap().and_hms_opt(23, 58, 0).unwrap();
let dt_utc = Utc.from_utc_datetime(&dt);
let dt_fixedoffset = FixedOffset::west_opt(5 * 60 * 60).unwrap().from_utc_datetime(&dt);
let dt_local = Local.from_utc_datetime(&dt);
assert_eq!(dt_utc, DateTime::<Utc>::from(dt_fixedoffset));
assert_eq!(dt_utc, DateTime::<Utc>::from(dt_local));
assert_eq!(dt_fixedoffset, DateTime::<FixedOffset>::from(dt_utc));
assert_eq!(dt_fixedoffset, DateTime::<FixedOffset>::from(dt_local));
assert_eq!(dt_local, DateTime::<Local>::from(dt_utc));
assert_eq!(dt_local, DateTime::<Local>::from(dt_fixedoffset));
}
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.
It is considered best practice to prefer many small tests each exercising fewer things rather than fewer larger tests each exercising many things.
Sometimes I would like to 🤣. In this case I think it started counting some of the lines in |
48f88fc
to
e813233
Compare
More tests for combinations of `DateTime::into`. Follow-up to Pull Request chronotope#271.
e813233
to
546dd9d
Compare
workflow codecov failed. These are often fixed by running again. I don't have access to force a re-run of the codecov workflow.
|
Triggered a re-run. |
@jtmoon79 Thank you for you efforts! Unfortunately I don't think these tests add enough value to merge 😞. |
More tests for combinations of
DateTime::into
.Follow-up to Pull Request #271.