jashkenas / underscore

JavaScript's utility _ belt
https://underscorejs.org
MIT License
27.33k stars 5.53k forks source link

Review _.throttle, _.debounce tests for time tampering #2883

Closed jgonggrijp closed 3 years ago

jgonggrijp commented 4 years ago

_.throttle and _.debounce both have a test to confirm that the limiting time interval continues unaltered when _.now is overridden. These tests can be found by searching the test/ directory for the phrase after system time is set backwards.

Since the modularization, these tests don't meet the intended purpose anymore; most Underscore functions now use direct references to other Underscore functions instead of indirect references through the _ object. In a way, this has made them more tamper-proof. This also applies to the use of _.now in _.throttle and _.debounce.

How to resolve this issue depends on whether there is any other way to tamper with the time as it is reported by _.now. If there is such a way, then the tests in question should be updated to use it. Otherwise, the tests can be removed.

ognjenjevremovic commented 3 years ago

Hey @jgonggrijp 👋

Giving a glimpse to the implementation of throttle and debounce methods I can see they use the now method directly, rather than indirectly referencing them through the globally available _ object. Does this mean that the related tests (this one and this one) should be removed as obsolete, as the test cases don't provide overall value (since overriding the _ methods have zero impact on the dependent functions)?

I would be willing to submit PR, but would perhaps need a little guidance on it. If I understood well, this issue relates only to test cases? Are there any other test cases that prove to be obsolete after the modularization of the package?

Cheers!

jgonggrijp commented 3 years ago

Hey @ognjenjevremovic 👋

Thanks for taking a peek at this, and for adding the links. I guess I should have done that immediately.

Does this mean that the related tests (this one and this one) should be removed as obsolete, as the test cases don't provide overall value (since overriding the _ methods have zero impact on the dependent functions)?

Well, that's basically the core question of this issue. You can't influence throttle and debounce by overriding _.now, but maybe you can influence the output of now by overriding window.Date/self.Date/global.Date (or maybe just Date.prototype.getTime)? Would be awesome if you could spend some time trying to hack now by overriding Date.

If you're able to do this, then the tests in question should be updated to override Date instead of now. On the other hand, if you can conclusively prove that there is no way to influence the output of now, then the tests can be removed.

If I understood well, this issue relates only to test cases?

Yes, this is only about the tests.

Are there any other test cases that prove to be obsolete after the modularization of the package?

I haven't investigated in detail, but please feel free to search for such tests.

Cheers!

ognjenjevremovic commented 3 years ago

Thanks for getting back to me.

I'll do some research regarding the Date object and see if one could override the methods on the Date prototype and how could that affect throttle and debounce methods. I'll also go through the rest of the methods from the library and see if there are any other use cases where this might apply as well.

I'll post some updates when I have some 🙂 .