fix(testing): do not throw when calling FakeTime.restore() more than once

[iuioiua]

Reviewing #5123, I thought this behavior was unreasonable.

[iuioiua]

Previously, this threw.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.73%. Comparing base (2c550f4) to head (570d815).
Report is 1 commits behind head on main.

Files	Patch %	Lines
testing/time.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5125      +/-   ##
==========================================
+ Coverage   92.72%   92.73%   +0.01%     
==========================================
  Files         479      479              
  Lines       38305    38305              
  Branches     5394     5395       +1     
==========================================
+ Hits        35520    35524       +4     
+ Misses       2732     2728       -4     
  Partials       53       53              

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

[kt3k]

I thought this behavior was unreasonable.

Can you elaborate on the reasoning?

[kt3k]

@KyleJune I'd appreciate if you can take a look

[iuioiua]

I thought this behavior was unreasonable.

Can you elaborate on the reasoning?

Well, what's wrong about calling FakeTime.restore() multiple times? If the answer is "nothing", then why do we throw? There's at least a reason not to throw, as seen in #5123 (comment).

[KyleJune]

@KyleJune I'd appreciate if you can take a look

Spy and Stub have the same behavior, if it is restored multiple times it will throw an error saying that it is already restored. If something was already restored before it was expected to, that may lead to unexpected behavior. I believe the idea I had at the time was that it would help you catch those types of situations.

However, FakeTime is a bit different than spy and stub. Spy and stub replace a method on an object while FakeTime completely replaces the Date object and some other global functions related to time.

Well, what's wrong about calling FakeTime.restore() multiple times? If the answer is "nothing", then why do we throw? There's at least a reason not to throw, as seen in #5123 (comment).

If you look at what the restore method does, it actually just takes the original globals and puts them back in place. I've taken the code sample from your comment, modified it slightly, and added comments to highlight the issue.

{
  using _time1 = new FakeTime(1000);
  assertEquals(Date.now(), 1000);
  {
    using _time2 = new FakeTime(2000);
    assertEquals(Date.now(), 2000);
  }
  assertEquals(Date.now(), 1000); // throws because Date is no longer a fake
}

The reason it throws is that when FakeTime.restore() was called, we just replaced the globals with their original values.

I think it would make more sense to instead prevent FakeTime from being called multiple times, if time is already fake, it should give an error indicating that. I checked the spy/stub code and it looks like it currently prevents spying on a method if it's already being spied on.

https://github.com/denoland/deno_std/blob/61a39ce14695b6890784c0779f95883fa509f7db/testing/mock.ts#L659

If you want to support faking time while it is already faked, I think the static restore function should be removed and that the constructor create references to what the globals are so that it can restore them to the state they were when the function was called instead of always restoring them to the original state. That would make it so that the assertion I point out as failing would pass since the inner fake time would restore time to the outer fake time.

[kt3k]

I think it would make more sense to instead prevent FakeTime from being called multiple times, if time is already fake, it should give an error indicating that.

This makes sense to me.

If you want to support faking time while it is already faked,

This feels overly complex to me. Let's avoid supporting this scenario

[iuioiua]

Thank you, @KyleJune. Yes, I think preventing FakeTime from being called multiple times, as long as there's no reasonable use case for doing so (I can't think of one).

[kt3k]

Can we close this now as we landed #5130 ?