Fixed JS crash between 11pm and 11:59pm when no time values are available#31
Conversation
| expect(culled_time_values).not.toHaveLength(6); | ||
| const now = dayjs().hour(13).minute(59).toString(); | ||
| const culled_time_values = timeValuesToday(now, constructedToday, state.time_values); | ||
| expect(culled_time_values).toHaveLength(3); |
There was a problem hiding this comment.
By adding a new parameter representing the current time to the timeValuesToday function, we decouple our reliance on current moving time when the test is running. We can hence test a more edgy scenario where we need to discard 3 hours and keep 3 hours. This should get rid of the previous failure too happening between midnight and 1 AM, as we have a pure function now with no time reading side effect.
| if (Number(state.focusedDayNum) === Number(state.lastDay)) { | ||
| const newMonth = dayjs(state.date) | ||
| .add(1, "month") | ||
| .add(1, "day") |
There was a problem hiding this comment.
The logic verifies if the last day of the month is the currently selected date. If yes, we either increment the month or not, depending if the last available date was reached.
The logic adds 1 month though to make that check, not merely adding 1 day to get to the next month, i.e. 29 feb will be 29 march, instead of 1st march.
It might worked before though because the last available date setup in the store during initialization was set to 1 month. I set it to 96 hours inadvertently with a copy/paste because this is what is used in Notify. Hence this trigger this bug discovery.
By adding 1 day instead, this should increment to the next month and be within the available time period if it applies.
| expect(label).toEqual("Unavailable, Friday February 28 2020"); | ||
| }); | ||
|
|
||
| test("Handles key nav events", async () => { |
There was a problem hiding this comment.
This is a new test that should trigger the fail scenario we encountered in Notify, i.e. get past 11 pm, the calendar will produce a Javascript error when trying to read inexistent values of available culled time.
|
Thanks for adding specific tests for this. Key to note as mentioned above
|
2 participants
Summary | Résumé
Show case of these two bugs are demonstrated in this other draft PR/branch:
#33
Test instructions | Instructions pour tester la modification
npm run start.Previously, the scheduler would not properly load past 23h00 due to a Javascript error. This should now load fine and display all available hourly time slots for the next day.
Help requested | Aide requise
There is one test that fails with a timeout on a calendar month change. This might be due to different Reactjs state.
Reviewer checklist | Liste de vérification du réviseur
This is a suggested checklist of questions reviewers might ask during their
review | Voici une suggestion de liste de vérification comprenant des questions
que les réviseurs pourraient poser pendant leur examen :
langues officielles?
une baisse de la quantité de code couvert par les tests automatisés?
fonctionnalité existante?
que ça devrait être divisé en de plus petites demandes de tirage (« pull
requests ») afin de réduire le risque lié aux modifications?
modification de la politique de confidentialité?
préoccupations liées à la sécurité?
façon importante la performance?
risque d’utiliser des dépendances ajoutées?
setup, etc.) | Faudra-t-il mettre à jour la documentation à la suite de ce
changement (fichier README, etc.)?