Update ImageUploadsController spec to use have_logged_event#10258
Update ImageUploadsController spec to use have_logged_event#10258
Conversation
have_logged_event doesn't support this, so we shouldn't use it here. Also, it seems like all the event detail isn't required here--this event is only logged if the form submission succeeds
| remaining_submit_attempts: IdentityConfig.store.doc_auth_max_attempts - 1, | ||
| pii_like_keypaths: pii_like_keypaths, | ||
| flow_path: 'standard', | ||
| ).exactly(0).times |
There was a problem hiding this comment.
This struck me as slightly odd--"make sure we receive this extremely specific method call exactly 0 times". Also there was a comma in the event name. I think what this is checking is that if the form validation fails, we are not logging IdV: doc auth image upload vendor submitted, so I updated it to that.
matthinz
requested review from
a team and
dawei-nava
and removed request for
a team
| expect(@analytics).not_to receive(:track_event).with( | ||
| 'IdV: doc auth image upload vendor submitted', | ||
| any_args, | ||
| ) | ||
|
|
||
| expect(@irs_attempts_api_tracker).to receive(:track_event).with( | ||
| :idv_document_upload_submitted, | ||
| any_args, | ||
| ) | ||
|
|
||
| action | ||
|
|
||
| expect(@analytics).not_to have_logged_event( |
There was a problem hiding this comment.
Do you mind clarifying why have_logged_event is better or more useful than to receive(:track_event).with(...)? Mostly curious and would be good to know for future tests.
There was a problem hiding this comment.
Not sure it's Matt's reason, but one reason to prefer it is that receive(:track_event) may override our custom PII alerting and documentation enforcing helpers.
There was a problem hiding this comment.
Yes, it is mostly those things. Also consistent use of have_logged_event gives us some freedom to make changes to how we test events in a single place rather than updating every single callsite
night-jellyfish
left a comment
night-jellyfish
left a comment
There was a problem hiding this comment.
This looks good to me and I'm approving, but it might be good to have one more Timnit engineer take a look on Monday.
(Also looks like CI is failing due to missing changelog).
Thanks for tagging us!
| stub_attempts_tracker | ||
|
|
||
| expect(@analytics).to receive(:track_event).with( | ||
| expect(@irs_attempts_api_tracker).to receive(:track_event).with( |
There was a problem hiding this comment.
Just curious, is the irs attempts thing still needed?
4 participants
🎫 Ticket
Related to work underway for LG-12657
🛠 Summary of changes
Over on #10230 I am doing some work that involves touching a lot of analytics-related specs. I noticed that the spec for
Idv::ImageUploadsControllerwas using syntax like:This PR updates the spec to use the
have_logged_eventmatcher, which will make some of my changes a little easier.The biggest thing here is that
have_logged_eventneeds to be called after the action, rather than before, so this meant moving some code around.