Betterment / journaled

A Rails engine to reliably deliver loosely-ordered schematized events to Amazon Kinesis via Delayed::Job
MIT License
13 stars 9 forks source link

RSpec matcher: cast all global_ids/JSON-compatible objects to String before comparing #30

Open smudge opened 1 year ago

smudge commented 1 year ago

The rspec matcher seems to match against the inputs to the JSON serialization, which results in false negatives where a developer might supply a string, but the matcher internally compares against a GlobalID and and fails the test. We should consider doing a round trip to JSON and back for both the event attributes and the dev-supplied attributes, so that the Ruby hashes used for comparison have JSONified scalar values.


# we want this to pass:
journal_event_including(attributes: { owner: "gid://app/model/ID" })

# and this should also pass:
journal_event_including(attributes: { owner: owner.to_global_id.to_s })

# but right now only this passes:
journal_event_including(attributes: { owner: owner.to_global_id })
pat-whitrock commented 1 year ago

What do you think of https://github.com/pat-whitrock/journaled/commit/d6609ec7185001a73ee808b990e6ff37f39bddb2 as a potential solution?

This worked for both of the cases mentioned above. I tested with the expected event containing both GIDs and strings for the actor attribute and an attribute nested in an arguments hash.

The notable drawback of the JSON round trip seems to be incompatibility with RSpec argument matchers, which might not be obvious to the author.

# all of these will pass
journal_event_including(attributes: { owner: "gid://app/model/ID" })
journal_event_including(attributes: { owner: owner.to_global_id.to_s })
journal_event_including(attributes: { owner: owner.to_global_id })

# but these will not
journal_event_including(attributes: { owner: an_instance_of(String) })
journal_event_including(attributes: { owner: an_instance_of(GlobalID) })

Any concerns with that?

smudge commented 1 year ago

@pat-whitrock Did the RSpec argument matchers previously work? It might be nice to continue supporting those.

The challenge I'm seeing is in supporting both:

journal_event_including(attributes: { owner: owner.to_global_id })

and:

journal_event_including(attributes: { owner: an_instance_of(String) })
pat-whitrock commented 1 year ago

Yes, just confirmed the RSpec argument matchers did previously work.

smudge commented 1 year ago

@pat-whitrock Maybe we can use deep_transform_values on the dev-supplied input to JSON-roundtrip-ify anything that isn't an respec matcher? (The actual payload can still go through a single JSON roundtrip). We should also then deep-symbolize (or deep-stringify, depending on which looks better in matcher failure outputs) the keys on both sides to make the hashes match again.