1806 new net p1 adding a reusable mock http client for unit tests #1868
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
I think MockUnitTestClient should return a HttpClient type, not a FetchHttpClient type because the returned object mocks the HttpClient, it isn't a `HttpClient' implementation based on the fetch library.
Consider we can have in the future HttpClient implementation based on different HTTP libraries like axios or undici.
| const mockHttpClient = <T>( | ||
| response: T, | ||
| httpMethod: 'get' | 'post' | ||
| ): FetchHttpClient => { |
There was a problem hiding this comment.
I would return an instance of HttpClient interface, not of FetchHttpClient implementation because this class "mocks", it doesn't represent an HTTP implementation using the methods provided by the fetch library.
There was a problem hiding this comment.
Makes sense since we are expecting a HttpClient anyways when running .askTo()
Test CoverageSummary
|
| httpMethod: 'get' | 'post' | ||
| ): HttpClient => { | ||
| return { | ||
| [httpMethod]: jest.fn().mockImplementation(() => { |
There was a problem hiding this comment.
im bit lost why we need Jest involved here
dont we just implement HttpClient interface where the rest can pas in the beforeRequest and afterResponse functions themselves, and the mock doesnt actually do a http request?
There was a problem hiding this comment.
think of more of a Stub i guess
There was a problem hiding this comment.
Copied from my comment on Slack:
After thinking a bit about it, I'm not sure I see the problem with using HttpClient for mocking things. We could be mock fetch and use FetchHttpClient with callbacks but that wouldn't change much about how it is testing things right now.
For the jest mock implementation fns those were needed for mocking FetchHttpClient but aren't needed now that it's using HttpClient, I have just removed them in the latest commit
Description
Introducing a mock HTTP client for unit tests in the thorest package. It mocks a simple response for either a post or get request (the only two types of HTTP methods we have in the thorest API).
Fixes #1806
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
All unit tests passing
Test Configuration:
Checklist: