[Tests]: System time tampering effects on `_.throttle` and `_.debounce` methods #2913

ognjenjevremovic · 2021-03-06T19:32:33Z

✅ Closes: #2883

Changes

Tests only:

Monkey patch Date.prototype.getTime and Date.prototype.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.

Checklist

After the changes I ensured that:

no stylistic or unwanted errors are present, by running npm run lint,
no tests are failing, by running npm run test,
build passes with no issues, by running npm run bundle.

Done

Added/updated unit tests for this change
Included links to related issues/PRs

Additional notes

The underlying changes could've been done by monkey patching the native Date and Date.prototype methods in before (or beforeEach) and after (or afterEach) hooks in the Functions QUnit module. However, that would require a minor changes in two other test cases from the same module (as the valueOf method of the Date.prototype would be changed for all test cases and always return the same value which would cause certain test cases to hang to the operations new Date - date < amount as that will always result in the false value in the while loop).

I can provide the change if necessary using before and after hooks instead.
I just wanted to assure that the implementation of _.throttle and _.debounce methods will not break, if the system time would be tempered (monkey patched).
I guess the test cases can be safely omitted as they're redundant @jgonggrijp?
If so, I can provide another commit to the same PR removing the obsolete test cases.

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

coveralls · 2021-03-06T19:34:27Z

Coverage remained the same at 95.197% when pulling 3a5c878 on ognjenjevremovic:test/time-tampering-tests into a4cc7c0 on jashkenas:master.

jgonggrijp

This is a great patch! Not only did you update those time-tampering tests to make them meaningful again, you also added new tests proving that _.throttle and _.debounce provide a stronger guarantee, i.e., they even continue working in the face of... let's call it "time vandalism". And you were very thorough in the PR description, I like that. Thank you so much, @ognjenjevremovic!

I have a few very minor change requests about the assertion comments, otherwise everything looks excellent.

Out of curiosity: why is there a ring emoji in the commit title? Is it some kind of cultural reference?

To answer your notes:

The underlying changes could've been done (...) in before (or beforeEach) and after (or afterEach) hooks in the Functions QUnit module. However, that would require a minor changes in two other test cases from the same module (...).

I can provide the change if necessary using before and after hooks instead.

I'm fine with the current solution, but if you want to refactor things a little bit, I'm fine with that, too. You can nest QUnit modules so that the hooks only affect the tests that should be affected, see docs here: https://api.qunitjs.com/QUnit/module/.

I just wanted to assure that the implementation of _.throttle and _.debounce methods will not break, if the system time would be tempered (monkey patched).
I guess the test cases can be safely omitted as they're redundant @jgonggrijp?
If so, I can provide another commit to the same PR removing the obsolete test cases.

Which test cases are obsolete?

jgonggrijp · 2021-03-07T23:19:06Z

test/functions.js

-    _.now = function() {
-      return new Date(2013, 0, 1, 1, 1, 1);
-    };
+    assert.strictEqual(counter, 1, '_.throttle: incr was called immediately');


Yes, good to add a comment to the assertion. Much clearer if it fails. 👍

jgonggrijp · 2021-03-07T23:19:31Z

test/functions.js

    }, 200);
  });

+  QUnit.test('throttle continues to function after system time is not accessible (or in invalid format)', function(assert) {


jgonggrijp · 2021-03-07T23:23:14Z

test/functions.js

+
+    _.delay(function() {
+      throttledIncr();
+      assert.strictEqual(counter, 2, '_.throttle: incr was debounced successfully, with tampered system time');


Throttling and debouncing are subtly different things, so to prevent confusion, I'd rather avoid the word "debounce" here. There is no convention to prefix all assertion comments with a qualified function name (at least not as far as I'm aware) and I don't think such a convention is necessary, either, so I suggest just changing it to this:

Suggested change

assert.strictEqual(counter, 2, '_.throttle: incr was debounced successfully, with tampered system time');

assert.strictEqual(counter, 2, 'incr was throttled successfully, with tampered system time');

jgonggrijp · 2021-03-07T23:23:55Z

test/functions.js

+
+    _.delay(function() {
+      throttledIncr();
+      assert.strictEqual(counter, 3, '_.throttle: incr was debounced successfully, after system time method restoration');


likewise

Suggested change

assert.strictEqual(counter, 3, '_.throttle: incr was debounced successfully, after system time method restoration');

assert.strictEqual(counter, 3, 'incr was throttled successfully, after system time method restoration');

jgonggrijp · 2021-03-07T23:24:48Z

test/functions.js


    _.delay(function() {
      debouncedIncr();
-      assert.strictEqual(counter, 2, 'incr was debounced successfully');
+      assert.strictEqual(counter, 2, '_.debounce: incr was debounced successfully, with tampered system time');


for brevity

Suggested change

assert.strictEqual(counter, 2, '_.debounce: incr was debounced successfully, with tampered system time');

assert.strictEqual(counter, 2, 'incr was debounced successfully, with tampered system time');

jgonggrijp · 2021-03-07T23:26:52Z

test/functions.js

+
+    _.delay(function() {
+      debouncedIncr();
+      assert.strictEqual(counter, 2, '_.debounce: incr was debounced successfully, with tampered system time');


again for brevity

Suggested change

assert.strictEqual(counter, 2, '_.debounce: incr was debounced successfully, with tampered system time');

assert.strictEqual(counter, 2, 'incr was debounced successfully, with tampered system time');

jgonggrijp · 2021-03-07T23:27:20Z

test/functions.js

+
+    _.delay(function() {
+      debouncedIncr();
+      assert.strictEqual(counter, 3, '_.debounce: incr was debounced successfully, after system time method restoration');


and one last time.

Suggested change

assert.strictEqual(counter, 3, '_.debounce: incr was debounced successfully, after system time method restoration');

assert.strictEqual(counter, 3, 'incr was debounced successfully, after system time method restoration');

ognjenjevremovic · 2021-03-08T09:19:33Z

Hey @jgonggrijp, thank you so much for such an in depth review.
I really appreciate it!

I have a few very minor change requests about the assertion comments, otherwise everything looks excellent.

You're right; the messages should definitely be more concise.
To be honest, I wasn't sure how to structure the assertion comments. I wanted to somehow diversify the messages between _.debounce and _.throttle but I'm not that good with the wording; I thought prefixing the assertion comments with the method name could help.
I'll be sure to address these with the suggested changes.

Out of curiosity: why is there a ring emoji in the commit title? Is it some kind of cultural reference?

I apologize for this 😄 .
I usually like to structure the commit messages in a descriptive manner, using the format: intention: + summary of the change (as a commit message head) followed by a long description. I use git-cz to make such formated commit messages.
I think the latest major version of the tool included the emojis and automatically prepends them to every commit message (made using the tool).
I can change the commit message and remove the emoji if you'd like?
* (Just to assure you, the emoji does not represents a symbol of any sort and it was unintentional as it does not really provide any meaningful value to the actual commit message).

Thank you for noticing such a detail 🙂 .

Which test cases are obsolete?

Just wanted to check if adding these 2 new test cases provided value to the overall tests and didn't include redundancy.
Also, I wanted to check if it is ok to keep the altered tests now that they use Date.prototype methods instead of _.now.
I think you already answered my question in your review; we want to keep all the tests.

I'm fine with the current solution, but if you want to refactor things a little bit, I'm fine with that, too.

If code duplication in tests is not too big of a trouble and doesn't introduce the confusion in the test cases by any means, I'm fine with the current implementation as well.
Using beforeEach and beforeAll would require certain changes and might be different from all other scenarios in the tests.

Update the assertion comments in `_.throttle` and `_.debounce` unit tests, in order to prevent confusion and add more brevity to the test case messages.

jgonggrijp

To be honest, I wasn't sure how to structure the assertion comments. I wanted to somehow diversify the messages between _.debounce and _.throttle but I'm not that good with the wording; I thought prefixing the assertion comments with the method name could help.
I'll be sure to address these with the suggested changes.

For clarity, I'm not entirely opposed to doing this. I just suggested removing the prefix in a few places because it was redundant in those particular cases.

I usually like to structure the commit messages in a descriptive manner, using the format: intention: + summary of the change (as a commit message head) followed by a long description. I use git-cz to make such formated commit messages.
I think the latest major version of the tool included the emojis and automatically prepends them to every commit message (made using the tool).
I can change the commit message and remove the emoji if you'd like?

Ah, that explains! I think I have seen similar commit messages elsewhere, now I understand where they're coming from. No need to apologize and you can leave the emoji in there. I think it's amusing.

If code duplication in tests is not too big of a trouble and doesn't introduce the confusion in the test cases by any means, I'm fine with the current implementation as well.
Using beforeEach and beforeAll would require certain changes and might be different from all other scenarios in the tests.

Yeah, if going that route, it's probably more consistent to restructure the tests a bit in general. That would more likely belong in a new issue or PR.

jgonggrijp · 2021-03-08T14:35:12Z

Congratulations with your first contribution! Please feel welcome to make more contributions.

(A suggestion, if you feel up to it: I have a big PR waiting (#2908) which requires multiple reviews by different people. If you could take a look at the diff and leave some comments, or if you know other, seasoned Underscore users who might be willing to do a code review (or contribute a real-world benchmark), that would be great.)

ognjenjevremovic changed the title ~~test: 💍 Time tampering tests for _.throttle and _.deobounce~~ [Tests]: System time tampering effects on _.throttle and _.debounce methods Mar 6, 2021

jgonggrijp requested changes Mar 7, 2021

View reviewed changes

test: Assertion comment updates; _.throttle and _.debounce.

3a5c878

Update the assertion comments in `_.throttle` and `_.debounce` unit tests, in order to prevent confusion and add more brevity to the test case messages.

ognjenjevremovic requested a review from jgonggrijp March 8, 2021 09:40

jgonggrijp approved these changes Mar 8, 2021

View reviewed changes

jgonggrijp merged commit 548fa01 into jashkenas:master Mar 8, 2021

jgonggrijp mentioned this pull request Apr 1, 2021

Changes to debounce() causing test failures with mocked timers #2918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tests]: System time tampering effects on `_.throttle` and `_.debounce` methods #2913

[Tests]: System time tampering effects on `_.throttle` and `_.debounce` methods #2913

ognjenjevremovic commented Mar 6, 2021

coveralls commented Mar 6, 2021 •

edited

Loading

jgonggrijp left a comment

jgonggrijp Mar 7, 2021

jgonggrijp Mar 7, 2021

jgonggrijp Mar 7, 2021

jgonggrijp Mar 7, 2021

jgonggrijp Mar 7, 2021

jgonggrijp Mar 7, 2021

jgonggrijp Mar 7, 2021

ognjenjevremovic commented Mar 8, 2021

jgonggrijp left a comment

jgonggrijp commented Mar 8, 2021

	assert.strictEqual(counter, 2, '_.throttle: incr was debounced successfully, with tampered system time');
	assert.strictEqual(counter, 2, 'incr was throttled successfully, with tampered system time');

	assert.strictEqual(counter, 3, '_.throttle: incr was debounced successfully, after system time method restoration');
	assert.strictEqual(counter, 3, 'incr was throttled successfully, after system time method restoration');

	assert.strictEqual(counter, 2, '_.debounce: incr was debounced successfully, with tampered system time');
	assert.strictEqual(counter, 2, 'incr was debounced successfully, with tampered system time');

	assert.strictEqual(counter, 3, '_.debounce: incr was debounced successfully, after system time method restoration');
	assert.strictEqual(counter, 3, 'incr was debounced successfully, after system time method restoration');

[Tests]: System time tampering effects on _.throttle and _.debounce methods #2913

[Tests]: System time tampering effects on _.throttle and _.debounce methods #2913

Conversation

ognjenjevremovic commented Mar 6, 2021

Changes

Checklist

Done

Additional notes

coveralls commented Mar 6, 2021 • edited Loading

jgonggrijp left a comment

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

jgonggrijp Mar 7, 2021

Choose a reason for hiding this comment

ognjenjevremovic commented Mar 8, 2021

jgonggrijp left a comment

Choose a reason for hiding this comment

jgonggrijp commented Mar 8, 2021

[Tests]: System time tampering effects on `_.throttle` and `_.debounce` methods #2913

[Tests]: System time tampering effects on `_.throttle` and `_.debounce` methods #2913

coveralls commented Mar 6, 2021 •

edited

Loading