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

Provide option to save bodies as JSON objects #487

Merged

Conversation

KayleighYum
Copy link
Contributor

A new configuration option has been added, saveStringBodies, which is set to true by default. This can be changed with the new setSaveStringBodies method. A new parameter has been added to toHarPostData and toHarContent where saveStringBodies will be passed in.

When saveStringBodies is set to false and the mimeType of the request/response is application/json the body will be json parsed and saved.

This will only be applied when the mockFormat is 'HAR'.
This will be applied to both request (when saveInputRequestBody is true) and response bodies.

Added in setSaveStringBodies to allow for non-string bodies in request and response
Added unit tests for default and setting values for new saveStringBodies
FromHarContent needed to have the same
check as the others to return text as is if the saveStringBodies was false
Copy link
Member

@divdavem divdavem left a comment

Choose a reason for hiding this comment

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

@KayleighYum Thank you very much for your PR! It is a good improvement for kassette.
I have left some comments about the things I think should or could be changed before integrating this new feature.

@KayleighYum
Copy link
Contributor Author

@KayleighYum Thank you very much for your PR! It is a good improvement for kassette. I have left some comments about the things I think should or could be changed before integrating this new feature.

Thanks @divdavem! I'll take a look at the comments

Made changes based on feedback from PR
Broken link in the api docs generation calling method name used in an earlier iteration of this PR
@KayleighYum
Copy link
Contributor Author

@divdavem I have fixed the issue with the failing api doc generation. Apologies about that

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ce30f1d) 91.52% compared to head (9a8e4a8) 91.64%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
+ Coverage   91.52%   91.64%   +0.11%     
==========================================
  Files          36       36              
  Lines        1192     1209      +17     
  Branches      268      273       +5     
==========================================
+ Hits         1091     1108      +17     
  Misses         50       50              
  Partials       51       51              
Flag Coverage Δ
e2e 79.56% <50.00%> (-0.47%) ⬇️
ut 60.29% <86.36%> (+0.39%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@divdavem divdavem left a comment

Choose a reason for hiding this comment

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

Hi @KayleighYum !
Thank you very much for the changes you have already made following my comments and to fix the build failure.
If it is ok for you, I have added some more comments to improve the code before we can integrate it.
Thank you!

packages/app/configuration/model.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.spec.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
packages/lib/har/harUtils.ts Outdated Show resolved Hide resolved
Made adjustments based on feedback from the pr
@KayleighYum
Copy link
Contributor Author

Hi @KayleighYum ! Thank you very much for the changes you have already made following my comments and to fix the build failure. If it is ok for you, I have added some more comments to improve the code before we can integrate it. Thank you!

Hey @divdavem I've made the adjustments based on your feedback, please can you resolve the conversations if you're happy with the changes or let me know if I've missed anything

Added some more unit tests to increase code coverage
KayleighYum and others added 3 commits December 12, 2023 11:34
Co-authored-by: divdavem <david-emmanuel.divernois@amadeus.com>
…hYum/kassette-stringified-json into feature/non-stringified-json-har
Moved body.toString evaluation
@divdavem divdavem merged commit 863b243 into AmadeusITGroup:master Dec 12, 2023
4 checks passed
@divdavem
Copy link
Member

@KayleighYum Thank you very much for your PR! It is now integrated. 🎉
We will release a new kassette version on npm soon.
If you have any feedback about kassette, and how you use it, we are always happy to hear about it.

@KayleighYum KayleighYum deleted the feature/non-stringified-json-har branch December 12, 2023 11:07
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.

None yet

2 participants