museun/mock_instant

compatibility with SystemTime

Closed this issue · 10 comments

bryanlarsen commented

This crate looks super useful to me. However, I need to mock SystemTime, not Instant. SystemTime has almost-but-not-quite the same API as Instant. I can almost-but-not-quite use mock_instant::Instant as SystemTime.

I can fork your crate, but it'd be nice if there was some way of unifying the two. Here's what I need to use this crate to mock SystemTime, for my use case. It's not complete, of course.

Some of the changes would be non-disruptive if I PR'd them, but I'm not sure how to deal with the change in return signature for functions like duration_since.

@@ -107,14 +107,22 @@
 /// the `MockClock`
 #[derive(Debug, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
 pub struct Instant(Duration);
+#[derive(Debug)]
+pub struct SystemTimeError(Duration);
+
+pub const UNIX_EPOCH: Instant = Instant(Duration::new(0, 0));
 
 impl Instant {
     pub fn now() -> Self {
         Self(MockClock::time())
     }
 
-    pub fn duration_since(&self, earlier: Self) -> Duration {
-        self.0 - earlier.0
+    pub fn duration_since(&self, earlier: Self) -> Result<Duration, SystemTimeError> {
+        if self.0 > earlier.0 {
+            Ok(self.0 - earlier.0)
+        } else {
+            Err(SystemTimeError(earlier.0 - self.0))
+        }
     }
 
     pub fn checked_duration_since(&self, earlier: Self) -> Option<Duration> {
@@ -159,12 +167,12 @@
     }
 }
 
-impl std::ops::Sub for Instant {
-    type Output = Duration;
-    fn sub(self, rhs: Self) -> Self::Output {
-        self.duration_since(rhs)
-    }
-}
+// FIXME: commenting this out because I'm too lazy to figure out the problem
+// impl std::ops::Sub for Instant {
+//     type Output = Duration;
+//     fn sub(self, rhs: Self) -> Self::Output {
+//         self.duration_since(rhs)
+//     }
+// }
 
 impl std::ops::Sub<Duration> for Instant {
     type Output = Instant;
@@ -216,7 +224,10 @@
         MockClock::advance(Duration::from_millis(100));
 
         let next = Instant::now();
-        assert_eq!(next.duration_since(now), Duration::from_millis(400));
+        assert_eq!(
+            next.duration_since(now).unwrap(),
+            Duration::from_millis(400)
+        );
     }
 
     #[test]
museun commented

We can just add a SystemTime api that mostly duplicates MockClock, MockSystemTime::advance et al

If you provide the impl (it'd be mostly copy-paste of the original code, but with the minor changes you need) I'll surely accept the PR. If you need any help with the impls, I can also help with that.

bryanlarsen commented
museun commented

After looking at the SystemTime api, I think I can provide a unified api. but I'm not 100% certain. (Its been a few years since I've actually looked at the code in this crate -- its simple and it works, so I didn't have a need to poke at it)

museun commented

So, one problem I can see immediately is that because there's a "global" time source, mock_instant::SystemTime and mock_instant::Instant will share the time 'start' time. This isn't a problem in practice, but it could be a be surprising if one is expecting the monotonic vs non-monotonic distinction between the two types in std.

For testing, I think the shared determinism will actually be the correct choice. So, its only a problem in "correctness" in a real system

museun commented

And a final problem: https://doc.rust-lang.org/std/time/struct.SystemTimeError.html this is a new type wrapping a duration, but I can't create a value for the type. So, either the error types will be different or I have to change the signature. the wasi impl in std returns a Result<Duration, Duration> but thats because wasi doesn't required std::error::Error (or more specifically the backtrace part of it).

bryanlarsen commented
bryanlarsen commented
museun commented

Considering testers have full control over how time advances, I don't think it'll be too surprising. We can already use your set_time api to make time go backwards...
…
On Wed, Apr 19, 2023, 7:50 p.m. museun @.> wrote: So, one problem I can see immediately is that because there's a "global" time source, mock_instant::SystemTime and mock_instant::Instant will share the time 'start' time. This isn't a problem in practice, but it could be a be surprising if one is expecting the monotonic vs non-monotonic distinction between the two types in std. For testing, I think the shared determinism will actually be the correct choice. So, its only a problem in "correctness" in a real system — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH2SJ6YEV5VVQX76N7IWTXCCB5NANCNFSM6AAAAAAXEAGQZU . You are receiving this because you authored the thread.Message ID: @.>

Well, my concern is that if you do

let now = mock_instant::Instant::now(); 
let st_now = mock_instant::SystemTime::now(); MockClock::advance(Duration::from_secs(2));

Then both st_now and now get advanced. It shouldn't really be a problem, but it can be surprising if one isn't aware that they'd share the same time source. I could add a separate 'Clock' for SystemTime so indv. operations can be used.

museun commented

I simply added the SystemTime/SystemTimeError wrapper and added a 2nd internal state, and the following functions on MockClock:

  • set_system_time
  • advance_system_time
  • system_time

I think this is sufficient, if that looks usable I can merge it and publish the 0.3 version with it.