Skip to content

Make analytics event matcher show a diff#7252

Merged
sheldon-b merged 2 commits intomainfrom
sbachstein/show-analytics-event-diffs-in-tests
Oct 31, 2022
Merged

Make analytics event matcher show a diff#7252
sheldon-b merged 2 commits intomainfrom
sbachstein/show-analytics-event-diffs-in-tests

Conversation

@sheldon-b
Copy link
Contributor

@sheldon-b sheldon-b commented Oct 28, 2022

🛠 Summary of changes

Updates our have_logged_event custom matcher to show diffs of unexpected analytics events

I only implemented diffs when we can tell that the expected event occurred exactly once but the attributes are different than expected. This will show diffs for cases when we do a direct comparison of the event's attributes to a hash (example) and when we use an include matcher (example). Other scenarios will fall back to the existing failure message

This is just a change to our tests. No changes to application code

👀 Screenshots

Example of rspec's built-in diff for hashes:

Screen Shot 2022-10-28 at 3 34 14 PM

Current way that have_logged_event displays diffs:

Screen Shot 2022-10-28 at 3 55 45 PM

Updated diff for hash comparison:

Screen Shot 2022-10-28 at 3 45 51 PM

Updated diff for comparing to an include matcher:

Screen Shot 2022-10-28 at 3 47 00 PM

[skip changelog]

@sheldon-b sheldon-b marked this pull request as ready for review October 28, 2022 21:38
@sheldon-b sheldon-b requested review from a team and svalexander October 28, 2022 21:41
Copy link
Contributor

@NavaTim NavaTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll be a really handy feature for debugging!

message += " got: #{actual}\n\n"
message += "Diff:#{differ.diff(actual, expected)}"
message
elsif matching_events&.length == 1 && attributes.instance_of?(RSpec::Matchers::BuiltIn::Include)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully a side-effect of this feature is to encourage the use of these matchers for partial matchers, in support of a ticket created a while back to change the default behavior of the hash-based matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and at least it'll be easier to implement that ticket now since the diffs of any existing tests that end up broken will be easier to parse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and at least it'll be easier to implement that ticket now since the diffs of any existing tests that end up broken will be easier to parse

Yep, though realistically the only way I see that ticket actually getting implemented is with a mass-conversion of existing hash assertions to equivalent include(...).

Copy link
Contributor

@zachmargolis zachmargolis Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we renamed this matcher have_logged_event_including and create a new have_logged_event_exactly and start encouraging that?

@sheldon-b sheldon-b merged commit 7c9884a into main Oct 31, 2022
@sheldon-b sheldon-b deleted the sbachstein/show-analytics-event-diffs-in-tests branch October 31, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants