Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
smudge opened this issue May 15, 2023 · 4 comments

Comments

@smudge
Copy link
Member

smudge commented May 15, 2023

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
Copy link

What do you think of pat-whitrock@d6609ec 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
Copy link
Member Author

smudge commented May 15, 2023

@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
Copy link

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

@smudge
Copy link
Member Author

smudge commented May 15, 2023

@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.

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

No branches or pull requests

2 participants