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

ognjenjevremovic commented 3 years ago

✅ 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

[x] Added/updated unit tests for this change
[x] 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.

coveralls commented 3 years ago

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

ognjenjevremovic commented 3 years ago

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.

jgonggrijp commented 3 years ago

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

jashkenas / underscore