chrono icon indicating copy to clipboard operation
chrono copied to clipboard

Add a feature and methods to override `now`

Open bryanburgers opened this issue 3 years ago • 6 comments

Support more testing use cases by adding a way to override the current time that is returned by Utc::now() and Local::now().

[package]
resolver = "2"

[dependencies]
chrono = "0.4"

[dev-dependencies]
chrono = { version = "0.4", features = ["test-override"] }
use chrono::{Utc, Datelike};

pub fn is_today_leap_day() -> bool {
    let today = Utc::today();
    today.month() == 2 && today.day() == 29
}

#[cfg(test)]
mod tests {
    use super::is_today_leap_day;
    use chrono::{Utc, DateTime};

    #[test]
    fn returns_true_on_leap_day() {
        Utc::test_override_now(Utc.ymd(2020, 2, 29).and_hms(0, 0, 0));
        assert!(is_today_leap_day());
    }

    #[test]
    fn returns_false_on_other_days() {
        Utc::test_override_now(Utc.ymd(2020, 2, 28).and_hms(0, 0, 0));
        assert!(!is_today_leap_day());
        Utc::test_override_now(Utc.ymd(2020, 3, 1).and_hms(0, 0, 0));
        assert!(!is_today_leap_day());
    }
}

Fixes #105

Thanks for contributing to chrono!

  • [x] Have you added yourself and the change to the changelog? (Don't worry about adding the PR number)

bryanburgers avatar Aug 07 '21 13:08 bryanburgers

I'm definitely open to thoughts on what the name of the feature should be and what the name of the methods should be.

I also considered returning another struct when overriding that will restore on drop. Something like

{
    let o = Utc::test_override();
    o.set(Utc.ymd(2020, 2, 29).and_hms(0, 0, 0));
    assert!(Utc::now() < Utc.ymd(2021, 1, 1).and_hms(0, 0, 0)); 
}

assert!(Utc::now() > Utc.ymd(2021, 1, 1).and_hms(0, 0, 0));

but I thought that might add too much complexity, especially if Utc::test_override was called multiple times.

bryanburgers avatar Aug 07 '21 13:08 bryanburgers

The other thought on naming is to put the override functions in their own module instead of hanging them off of Utc and Local.

  • chrono::test_override::set_utc(_: DateTime<Utc>)
  • chrono::test_override::clear_utc()
  • chrono::test_override::set_local(_: DateTime<Local>)
  • chrono::test_override::clear_local()
use chrono::{Utc, Datelike};

pub fn is_today_leap_day() -> bool {
    let today = Utc::today();
    today.month() == 2 && today.day() == 29
}

#[cfg(test)]
mod tests {
    use super::is_today_leap_day;
    use chrono::{Utc, DateTime};

    #[test]
    fn returns_true_on_leap_day() {
        chrono::test_override::set_utc(Utc.ymd(2020, 2, 29).and_hms(0, 0, 0));
        assert!(is_today_leap_day());
    }

    #[test]
    fn returns_false_on_other_days() {
        chrono::test_override::set_utc(Utc.ymd(2020, 2, 29).and_hms(0, 0, 0));
        assert!(!is_today_leap_day());
        chrono::test_override::set_utc(Utc.ymd(2020, 2, 29).and_hms(0, 0, 0));
        assert!(!is_today_leap_day());
    }
}

bryanburgers avatar Aug 09 '21 16:08 bryanburgers

Any progress on this?

bbigras avatar Nov 08 '21 19:11 bbigras

I am working on some tests where I would need this. Is there anything I can do to help push this forward?

slashrsm avatar Nov 22 '21 12:11 slashrsm

Hi @slashrsm and @bryanburgers - I can see where this is useful for testing, just a few thoughts:

  • Could your use cases instead be solved by passing a DateTime to the relevant functions, and having the actual current time generated somewhere earlier in the code?
  • I'm a bit concerned about this being enabled by a dependency and then causing unexpected effects, potentially one way to resolve this could be changing the cfg directive from #[cfg(feature = "test-override")] to #[cfg(all(test, feature = "test-override"))]. Another (or additional) option could be making the set and clear functions private and having an API that runs the fixed now time on a separate thread to avoid interference with other code

esheppa avatar Jul 04 '22 12:07 esheppa

I think this would have to come with a very strong rationale for why (something like) this is the best possible design. I don't think it fits very well with the scope of chrono as a fairly low-level library.

djc avatar Jul 04 '22 13:07 djc

I think this would have to come with a very strong rationale for why (something like) this is the best possible design. I don't think it fits very well with the scope of chrono as a fairly low-level library.

I personally don't have any strong rationale to offer. I can imagine there are integration tests and unfortunately structured codebases where adjusting the current time is the easiest or even only way to do some tests.

However https://github.com/chronotope/chrono/issues/105#issuecomment-633260201 in the related issue has collected 28 thumb ups in three years. This may be our most popular feature request.

pitdicker avatar Jul 10 '23 09:07 pitdicker

To comment on this PR: I would add just one standalone public function in the offset module, which can serve to set and clear the override:

pub fn override_now(datetime: Option<DateTime<FixedOffset>>);

And by making it take a FixedOffset you can also use it to test different timezone offsets.

I also like the suggestion by @esheppa in https://github.com/chronotope/chrono/pull/580#issuecomment-1173780170 to confine the feature to testing only with #[cfg(all(test, feature = "test-override"))].

@bryanburgers would you still be available to work on this after two years?

pitdicker avatar Jul 10 '23 09:07 pitdicker

I personally don't have any strong rationale to offer. I can imagine there are integration tests and unfortunately strutured codebases where adjusting the current time is the easiest or even only way to do some tests.

Yes, I have that exact scenarios, and it's not that uncommon. You're writing a test looking at data fixtures coming originally from an outside service, and your code is time related to now(). For example checking if more than a time period has passed, or should that session be expired or not.

penso avatar Jul 10 '23 12:07 penso

To chime in "as a Test Engineer", I think the overall idea here is sound.

Skimming the code changes, they seem okay to me. However, I can't comment if this is necessarily the cleanest way to do this; would require a long think.

Additionally, I couldn't determine if the override of now behavior is being used for all the test cases that call now, which is what this PR should also do (if it's not already). At first glance, it appears that is not occurring.

jtmoon79 avatar Jul 15 '23 21:07 jtmoon79

The way I do it so far is having an app context Context and use ctx.now() everywhere I need:

#[derive(Clone)]
pub struct AppContext {
    pub now: Option<DateTime<Utc>>,
}

impl AppContext {
    pub fn now(&self) -> DateTime<Utc> {
        self.now.unwrap_or(chrono::Utc::now())
    }

    pub fn set_now(&mut self, now: &str) {
        self.now = Some(now.parse().expect("Can't parse now"));
    }
}

penso avatar Jul 16 '23 10:07 penso

@jtmoon79 I planned on taking over this PR next week, but maybe it is more your cup of tea?

pitdicker avatar Jul 16 '23 17:07 pitdicker

@jtmoon79 I planned on taking over this PR next week, but maybe it is more your cup of tea?

Before doing any work, I'll wait until @djc approves of the concept.

jtmoon79 avatar Jul 16 '23 18:07 jtmoon79

I guess I'm open to having something like this. I think it should default to off and should only be turned on with a cfg flag (not a Cargo feature, which can be turned on by indirect dependencies). We should offer a way both to set the current time and the timezone/offset.

djc avatar Jul 24 '23 11:07 djc

https://github.com/chronotope/chrono/pull/1244 is the updated PR.

pitdicker avatar Sep 02 '23 20:09 pitdicker