❌🙁 Fix codecov-test failure - ‼️📅 Rewrite - notifScheduler.ts #1123
Conversation
…haracter (unicode U+202F)
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## service_rewrite_2023 #1123 +/- ##
========================================================
+ Coverage 58.83% 68.14% +9.31%
========================================================
Files 26 27 +1
Lines 1421 1557 +136
Branches 320 332 +12
========================================================
+ Hits 836 1061 +225
+ Misses 585 496 -89
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Why was it failing?The test was working correctly, but the problem was that the logDebug statement was not being ran. What did I do to fix it?I corrected this by making the Do I need to do anything to squash merge my PR with this PR? |
|
@shankari added to Ready for Review |
There was a problem hiding this comment.
I am going to merge this to unblock @the-bay-kay but I have several questions about this.
The test was working correctly, but the problem was that the logDebug statement was not being ran.
- Why was the
logDebugstatement not being run?- Note also my cleanup comment for the original PR was that we shouldn't rely on
logDebugto verify values but should add in introspection methods and check the values correctly. Please make sure to implement all those "future" changes.
- Note also my cleanup comment for the original PR was that we shouldn't rely on
- U+202F is a non-breaking space https://www.compart.com/en/unicode/U+202F It has nothing to do with the quotes. I'm guessing you copy-pasted something from a website, and introduced the NNBSP into the text by default. You will need to delete and type this out to remove it, or view the text in non-unicode to see the character that you remove.
Finally, I am very skeptical that you were not able to reproduce the error(s) while running locally after pulling from the service_rewrite_2023 branch, since multiple other PRs that were based on the branch had failing tests.
I would encourage you to re-pull after this and try to clean up the tests, both in this PR and in the original one.
| console.log('test log: isScheduling during getScheduledNotifs', isScheduling); | ||
| console.log('test log: scheduledPromise during getScheduledNotifs', scheduledPromise); |
There was a problem hiding this comment.
Do you need these console.log statements?
| expect(setIsScheduling).toHaveBeenCalledWith(true); | ||
| expect(logDebug).toHaveBeenCalledWith('After cancelling, there are no scheduled notifications'); | ||
| expect(logDebug).toHaveBeenCalledWith( | ||
| 'After scheduling, there are 4 scheduled notifications at 21:00 first is November 19, 2023 at 9:00 PM', | ||
| ); | ||
| expect(setIsScheduling).toHaveBeenCalledWith(false); | ||
| expect(scheduledNotifs).toHaveLength(4); | ||
| expect(scheduledNotifs[0].key).toEqual(expectedResultcheduleNotifs[0].key); | ||
| expect(scheduledNotifs[1].key).toEqual(expectedResultcheduleNotifs[1].key); | ||
| expect(scheduledNotifs[2].key).toEqual(expectedResultcheduleNotifs[2].key); | ||
| expect(scheduledNotifs[3].key).toEqual(expectedResultcheduleNotifs[3].key); |
There was a problem hiding this comment.
This change removes the setIsScheduling checks. Why are they no longer required?
|
Squash merging to avoid commit churn |
No description provided.