Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jgonggrijp opened this issue Oct 11, 2020 · 3 comments · Fixed by #2913
Closed

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

jgonggrijp opened this issue Oct 11, 2020 · 3 comments · Fixed by #2913
Labels
starter good choice for new contributors test

Comments

@jgonggrijp
Copy link
Collaborator

_.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.

@jgonggrijp jgonggrijp added test starter good choice for new contributors labels Oct 11, 2020
@ognjenjevremovic
Copy link
Contributor

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
Copy link
Collaborator Author

jgonggrijp commented Mar 5, 2021

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
Copy link
Contributor

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 🙂 .

ognjenjevremovic pushed a commit to ognjenjevremovic/underscore that referenced this issue Mar 6, 2021
Monkey patch `Date.prototype` getTime and valueOf methods and `Date.now`
method in _.debounce and _.throttle test cases. Provide additional test
case asserting the outputs before and after monkey patching. Remove the
reference to _.now method in test cases as the underlying implementation
of library methods don't reference the methods from the `_` object but
rather call dependent methods directly.

✅ Closes: jashkenas#2883
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
starter good choice for new contributors test
Projects
None yet
2 participants