test: Add unit tests for skipSurvey() function (#444)#451
test: Add unit tests for skipSurvey() function (#444)#451Hitanshi7556 wants to merge 2 commits intoOneBusAway:developfrom
Conversation
- Test recurring survey skip: verify _skipped_timestamp set, _skipped NOT present - Test one-time survey skip: verify _skipped set, _skipped_timestamp NOT present - Test stale flag cleanup: verify old _skipped flag removed for recurring surveys - All 24 tests passing
There was a problem hiding this comment.
Pull request overview
Adds Vitest unit coverage for skipSurvey() behavior in src/lib/Surveys/surveyUtils.js, targeting the recurring-vs-one-time skip logic introduced in #420.
Changes:
- Mock
$stores/surveyStoreto isolatesurveyUtilsin unit tests. - Add new
skipSurveytest cases for recurring surveys, one-time surveys, and stale flag cleanup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(localStorage.getItem('survey_123_skipped_timestamp')).toBeDefined(); | ||
| expect(localStorage.getItem('survey_123_skipped')).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
localStorage.getItem() returns either a value or null (never undefined). Using toBeDefined() will pass even when the key is missing (because null is still “defined”), so these assertions won’t actually verify that the timestamp was written. Use an assertion like not.toBeNull() (and ideally assert it’s a numeric timestamp) instead.
| expect(localStorage.getItem('survey_456_skipped')).toBeDefined(); | ||
| expect(localStorage.getItem('survey_456_skipped_timestamp')).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
toBeDefined() is not a reliable check for localStorage.getItem() (it will pass on null). This test should assert the permanent skip key is present with not.toBeNull() (and optionally that it equals 'true'/true depending on conventions) to ensure it really gets set.
| expect(localStorage.getItem('survey_789_skipped')).toBeNull(); | ||
| expect(localStorage.getItem('survey_789_skipped_timestamp')).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Same issue here: localStorage.getItem() returns null for missing keys, so toBeDefined() won’t fail if the timestamp was never set. Prefer not.toBeNull() (and ideally validate it’s a timestamp) so the test actually guards the recurring-survey behavior.
|
Resolved all comments PTAL @aaronbrethorst |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Overview
Adds unit tests for the
skipSurvey()function to improve coverage for the fix introduced in #420.Covered
_skipped_timestampis set whenallows_multiple_responses: true_skippedflag is set correctly_skippedflag when_skipped_timestampis usedTesting
npm run test -- --runCloses #444