compatibility with SystemTime

[bryanlarsen]

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]

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]

Yah, I'm probably trying to over think it.

…

On Wed, Apr 19, 2023, 7:21 p.m. museun ***@***.***> wrote: We can just add a SystemTime api that mostly duplicates MockClock, MockSystemTime::advance et al — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH2SO5C5LUWRY6PPEEQBTXCB6SHANCNFSM6AAAAAAXEAGQZU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[museun]

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]

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]

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]

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: ***@***.***>

[bryanlarsen]

Yah, in my snippet I added a new SystemTimeError to your crate, and then used the test cfg to selectively choose whether to import SystemTimeError from the crate or from std.

…

On Wed, Apr 19, 2023, 8:08 p.m. museun ***@***.***> wrote: 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). — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAH2SONW6CTTANJV4HRLA3XCCEARANCNFSM6AAAAAAXEAGQZU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[museun]

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]

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.

[museun]

I've published this as 0.3.0: https://docs.rs/mock_instant/0.3.0/mock_instant/