sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
797 stars 104 forks source link

Add timer getters for returned object via setTimeout, etc. #362

Closed XadillaX closed 3 years ago

XadillaX commented 3 years ago

Purpose (TL;DR) - mandatory

Background (Problem in detail)

Sometimes we want to get inner metadata (e.g. delay) of a timer. The original way is to use an undocumented map clock.timers[timer.id].

This PR is to make us can use timer.delay directly.

Solution

Add some getters in returned object bind to the original timer metadata.

codecov[bot] commented 3 years ago

Codecov Report

Merging #362 (f5ad14f) into master (43dc8db) will increase coverage by 0.07%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   93.98%   94.06%   +0.07%     
==========================================
  Files           1        1              
  Lines         549      556       +7     
==========================================
+ Hits          516      523       +7     
  Misses         33       33              
Flag Coverage Δ
unit 94.06% <100.00%> (+0.07%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/fake-timers-src.js 94.06% <100.00%> (+0.07%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 43dc8db...f5ad14f. Read the comment docs.

XadillaX commented 3 years ago

/ping @mroderick

XadillaX commented 3 years ago

/ping @fatso83

fatso83 commented 3 years ago

@XadillaX Thanks for your efforts! That being said, I am trying to keep my hands off this project and leave it to someone that has more daily interest in it (I am not even coding JS these days), so hoping @sinonjs/lolex-core can take a look at this and possibly publish a new minor version.

benjamingr commented 3 years ago

Fwiw I saw this but I'm not sure it's a good idea to expose this functionality 😅

Sorry for the lack of feedback and dropping the ball... I'm also with a newborn, so I don't have a ton of free time.

XadillaX commented 3 years ago

Fwiw I saw this but I'm not sure it's a good idea to expose this functionality

Sorry for the lack of feedback and dropping the ball... I'm also with a newborn, so I don't have a ton of free time.

Sometimes I want to check the delay value of a timer that some function to be tested just set.

benjamingr commented 3 years ago

Sometimes I want to check the delay value of a timer that some function to be tested just set.

Isn't checking the effect (that is, .ticking that duration and then seeing the effect happened) sufficient?

fatso83 commented 3 years ago

Closed after lack of response

XadillaX commented 3 years ago

When I create a timer, I can assert its delay without ticking it.

e.g.

let timer;
if (a) {
  timer = setTimeout(() => {}, 1000);
} else {
  timer = setTimeout(() => {}, 2000);
}
fatso83 commented 3 years ago

Hi, @XadillaX . Thanks for getting back to us!

To me, this seems like checking the implementation, not the effects of said implementation. That is usually not regarded good testing practice. You want to know if something happens after X ms through its external interface, but not how (it could be two or ten timeouts in between, but that is internal details and none of your business).

So I kind of second this:

Isn't checking the effect (that is, .ticking that duration and then seeing the effect happened) sufficient?

XadillaX commented 3 years ago

Hi, @XadillaX . Thanks for getting back to us!

To me, this seems like checking the implementation, not the effects of said implementation. That is usually not regarded good testing practice. You want to know if something happens after X ms through its external interface, but not how (it could be two or ten timeouts in between, but that is internal details and none of your business).

So I kind of second this:

Isn't checking the effect (that is, .ticking that duration and then seeing the effect happened) sufficient?

The example may not be very clear.

Let's set a certain timer (with same callback), but in different delays.

class Checker {
  constructor() {
    this.timer = null;
    this.checked = false;
  }

  check() {
    const time = Date.now();
    this.timer = setTimeout(doCheck.bind(this), 1000 - (time % 1000));
  }

  doCheck() {
    this.checked = true;
  }
}

const checker = new Checker();
checker.check();
const now = Date.now();
assert((checker.timer.delay + now) % 1000 === 0);

// do other tests to test checker.checked