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

Fix and test that rfq is wrapped in object in CreateExchange endpoint #177

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

diehuxx
Copy link

@diehuxx diehuxx commented Feb 20, 2024

Fixes a bug in implementation of TBD54566975/tbdex#167

Also adds test conditions as per #176

Comment on lines 98 to 99
assertEquals(request1.path, "/exchanges/${rfq.metadata.exchangeId}")
assertEquals(request1.body.readUtf8(), Json.stringify(mapOf("rfq" to rfq)))
Copy link
Author

Choose a reason for hiding this comment

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

What do we think of testing path and url this way? Other endpoints will also have http headers that we need to test for, but this one does not.

If we like this pattern, I will open a follow up PR which adds these assertions to all relevant http client tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

The assertion of the path is fine, but the assertion of the body may lead to weird false negatives because I'm not sure we can safely assume the .readUtf8() and Json.stringify() functions will encode the string in the exact same ways (you could imagine one using spaces, and the other tabs, or any weird whitespace thing, or non-whitespace special characters such as \n)

But, we may be doing this all over the place, in which case I wouldn't worry about it, at least right now, and also I'm not a Kotlin expert so this skepticism may be totally misplaced

Copy link
Author

@diehuxx diehuxx Feb 21, 2024

Choose a reason for hiding this comment

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

Here's an alternative that addresses potential differences in whitespace. We can JSON parse the actual and expected request bodies to get JsonNode objects which we can deep compare. What do you think?

assertEquals(
  Json.jsonMapper.readTree(request1.body.readUtf8()),
  Json.jsonMapper.readTree(Json.stringify(mapOf("rfq" to rfq)))
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm better but still don't love it

Does the assertEquals() do a deep compare on any Kotlin object? If so, you could parse the http body into an RFQ object and just do a assertion with the existing rfq variable. This would also technically test our parsing functionality which is a little against the concept of a unit test.

Yeah I'm cool with any of the above approaches... how it is now is fine, your proposal is fine, or what I just ideated on with parsing. Your call!

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, i believe assertEquals() does do a deep compare on objects, based on the equals() method for all objects which you can override to compare specific attributes.

i imagine that JsonNode returned from readTree() calls have such an equals method that allows deep comparison between two JsonNodes.

i prefer the second one @diehuxx proposed since we can be sure it's a comparison of 2 things that are the same type :)

Copy link
Author

@diehuxx diehuxx Feb 21, 2024

Choose a reason for hiding this comment

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

Thanks for the input, all!

I'm gonna go with the Json.jsonMapper.readTree approach. Parsing the RFQ does look a bit nicer, but comparing the JSON Nodes allows us to not only check that the right fields are present, but also that there are not extra fields in the JSON that we don't expect.

Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Merging #177 (48c22ac) into main (7f7fe24) will decrease coverage by 0.02%.
Report is 7 commits behind head on main.
The diff coverage is 66.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
- Coverage   77.59%   77.58%   -0.02%     
==========================================
  Files          33       33              
  Lines         808      812       +4     
  Branches       80       80              
==========================================
+ Hits          627      630       +3     
- Misses        134      135       +1     
  Partials       47       47              
Components Coverage Δ
protocol 84.13% <ø> (ø)
httpclient 78.46% <66.66%> (-0.07%) ⬇️

Copy link
Contributor

@KendallWeihe KendallWeihe left a comment

Choose a reason for hiding this comment

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

lgtm, @jiyoontbd any additional thoughts?

Copy link
Contributor

@jiyoontbd jiyoontbd left a comment

Choose a reason for hiding this comment

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

thank you!

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.

Test that http client requests have expected path, headers, body
3 participants