Fix ceil with zero duration

[cardigan1008]

Fix 14. Arithmetic overflow bug in #244

In #244 , ceil() is called with a duration of 0, which can lead to an arithmetic overflow. While returning 0 is one way to address this, another approach is to report an error to the user.

I'm not very familiar with operations involving time, so apologies for any misunderstandings.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.75%. Comparing base (0cde737) to head (c58ca0b).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #323   +/-   ##
=======================================
  Coverage   83.75%   83.75%           
=======================================
  Files          22       22           
  Lines        3688     3688           
=======================================
  Hits         3089     3089           
  Misses        599      599           

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

[ChristopherRabotin]

Thanks! Can you add a regression test?

[cardigan1008]

Thanks! Can you add a regression test?

Sure! But I'm wondering if this change is logically correct. In this example, with this change, the final output is 1900-01-01T00:00:00 UTC:

fn main() {
    let _local0 =
        if let Ok(x) = hifitime::prelude::Epoch::maybe_from_gregorian_utc(1792, 1, 1, 0, 0, 0, 0) {
            x
        } else {
            use std::process;
            process::exit(0);
        };
    let _local5 = hifitime::prelude::Epoch::duration_in_year(&(_local0));
    let _ = hifitime::prelude::Epoch::to_utc_seconds(&(_local0));
    let _ = hifitime::prelude::Duration::decompose(&(_local5));
    let a = hifitime::prelude::Epoch::ceil(&(_local0), _local5);
    println!("{:?}", a);
}

[ChristopherRabotin]

Thanks for your contribution. Could you implement a few changes to the test and then I think it's ready to merge.

[cardigan1008]

Thanks for your contribution. Could you implement a few changes to the test and then I think it's ready to merge.

Thank you for your guidance! I have implemented the changes you suggested. As I'm not very familiar with adding regression tests in Rust, your advice was extremely helpful.

[ChristopherRabotin]

My apologies, I didn't take enough time to look at the test. I think that you're making the test too complicated for what should be tested because the Epoch::ceil directly calls Duration::ceil, so only that last function needs testing. Hence, I recommend the following:

#[test]
fn regression_test_gh_244() {
    let zero = Duration::ZERO;
    // Test that the ceil of a zero duration is still zero.
    assert_eq!(zero.ceil(zero), zero);
    let non_zero = Duration::from_parts(1, 23456);
    // Test that the ceil of a non-zero duration by zero is still that original duration.
    assert_eq!(non_zero.ceil(zero), non_zero);
    // Test that the ceil of a zero duration by a non-zero is still zero.
    assert_eq!(zero.ceil(non_zero), zero);
}

[cardigan1008]

My apologies, I didn't take enough time to look at the test. I think that you're making the test too complicated for what should be tested because the Epoch::ceil directly calls Duration::ceil, so only that last function needs testing. Hence, I recommend the following:

#[test]
fn regression_test_gh_244() {
    let zero = Duration::ZERO;
    // Test that the ceil of a zero duration is still zero.
    assert_eq!(zero.ceil(zero), zero);
    let non_zero = Duration::from_parts(1, 23456);
    // Test that the ceil of a non-zero duration by zero is still that original duration.
    assert_eq!(non_zero.ceil(zero), non_zero);
    // Test that the ceil of a zero duration by a non-zero is still zero.
    assert_eq!(zero.ceil(non_zero), zero);
}

I just found the second assertion failed. With this change, the ceil of a non-zero duration by zero is still zero. So here comes the logical issue. What do you think of this?

[ChristopherRabotin]

the ceil of a non-zero duration by zero is still zero
That actually looks like the correct behavior, and that test recommendation is wrong! The whole point of the change is to avoid a modulo operation by zero, so indeed, the ceil of a non-zero duration by zero should be zero.

Could you make that test change and then we're good!

[cardigan1008]

Could you make that test change and then we're good!

Sure, thanks for your prompt response! I have just made the changes to the test.

I learned a lot from our discussion. Initially, I simply copied the code snippet from the issue without considering the coverage and simplicity of the regression test case. Your guidance was very helpful. Thank you!