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

Using real data for unit test mocking #204

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Using real data for unit test mocking #204

merged 2 commits into from
Jan 29, 2025

Conversation

AndrewTwydell
Copy link
Contributor

@AndrewTwydell AndrewTwydell commented Jan 10, 2025

What It Does
Adapts SDK unit tests to use real CMCI responses for a more accurate behaviour.

Review Checklist
I certify that I have:

Additional Comments

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.56%. Comparing base (b51d5c2) to head (fe88b40).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #204   +/-   ##
=======================================
  Coverage   28.56%   28.56%           
=======================================
  Files         147      147           
  Lines        5031     5031           
  Branches      882      881    -1     
=======================================
  Hits         1437     1437           
  Misses       3594     3594           

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

Copy link
Contributor

@davenice davenice left a comment

Choose a reason for hiding this comment

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

This looks good!

Great to use actual data.

We might find in the future we need to have specific data per test, rather than a generic set of mocks, but I imagine that the generic set will do us for a while!

@AndrewTwydell AndrewTwydell force-pushed the sdk-unit-mocking branch 3 times, most recently from 85af963 to 2dc5377 Compare January 20, 2025 10:22
@AndrewTwydell AndrewTwydell marked this pull request as ready for review January 20, 2025 10:22
@AndrewTwydell
Copy link
Contributor Author

@zFernand0 I'd like to run this by you, now that I've marked it ready for review - the only potential contentious point would be that I've changed the method we're mocking here.

We're unit testing getResource still, but I'm now mocking the XML response rather than the JSON parsed version of the XML, so if anything I'm now testing more?? The idea behind this was so we can test real CMCI error messages more easily, but I'd like to know if you're onboard with this style of testing or not! 😀

@AndrewTwydell AndrewTwydell force-pushed the sdk-unit-mocking branch 2 times, most recently from 9dfa2e8 to 34f9a9f Compare January 27, 2025 16:24
@JTonda JTonda requested review from zFernand0 and traeok January 28, 2025 16:04
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @AndrewTwydell!

@AndrewTwydell AndrewTwydell merged commit 1087db6 into main Jan 29, 2025
19 checks passed
@AndrewTwydell AndrewTwydell deleted the sdk-unit-mocking branch January 29, 2025 21:40
@AndrewTwydell AndrewTwydell added the release-current Indicates that there is no new functionality being delivered label Jan 29, 2025
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM! 😋

Apologies for the late review 😓

Copy link
Member

Choose a reason for hiding this comment

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

I really like this approach 🙏
Thanks for adding the mock responses 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog release-current Indicates that there is no new functionality being delivered
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

6 participants