Allow falsy values (except undefined) as a valid body #1825
Conversation
🦋 Changeset detectedLatest commit: f79fa25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| fetchOptions: FetchOptions<any>; | ||
| }) => { | ||
| const client = createClient<any>({ baseUrl, bodySerializer: options.bodySerializer }); | ||
| const { getRequest } = useMockRequestHandler({ |
There was a problem hiding this comment.
I assume this is piling one mock over the other. I didn't find any "cleanup" procedure in the existing tests, so piling several more mocks on top 🙂
There was a problem hiding this comment.
Ha, missed that. Thanks 👍
| @@ -1275,6 +1277,186 @@ describe("client", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe("body serialization", () => { | |||
There was a problem hiding this comment.
I think these tests could be better placed next to other body tests
There was a problem hiding this comment.
I originally had them there, but then saw that the whole test group is within "TypeScript checks" and these have nothing to do with TypeScript.
There was a problem hiding this comment.
You are right and it seems that most of the tests in this group are not even about types 🤔
There was a problem hiding this comment.
That’s a good callout. We probably need to clean up our tests by moving TypeScript tests to actual Vitest type tests. This test file has also grown in size (which is a great problem to have) and can probably be broken up. All that to say, not too prescriptive about where this test goes; if it exists somewhere I’m happy 🙂
| return { bodyUsed: request.bodyUsed, bodyText }; | ||
| }; | ||
|
|
||
| it.each(ALL_METHODS)("missing body (with body serializer) - %s", async (method) => { |
There was a problem hiding this comment.
I think we should definitely use more often it.each to have cleaner tests while testing more cases. But in this case isn't it overkill to test every method each time as the logic tested is only about falsy values?
What about a single test it.each([{ body: undefined, except: false }, { body: 1, except: true }, ...])
There was a problem hiding this comment.
But in this case isn't it overkill to test every method each time as the logic tested is only about falsy values?
There are differences in how GET and HEAD are used in some of the underlying implementation (not in this lib, hopefully) - some clients and servers disallow bodies for them. This includes the tools used in these tests. Having to think about these differences inspired me to rather have a comprehensive test of all methods. If you guys don't see it valuable, I'm OK with dropping it.
What about a single test it.each([{ body: undefined, except: false }, { body: 1, except: true }, ...])
I usually prefer the assertions to be as explicit as possible, hence the explicit expect().toBe(), but I don't mind pulling the expectations into the test arguments, if that's what you guys are used to. Matter of taste really.
|
Thanks for the contrib! I just added some comments regarding the tests. But imo, a unit test must test a single feature, in this case |
The fact that I'm running the tests over each method independently doesn't have anything to do with testing method behavior. It's just to ensure that the feature I am testing works consistently across all methods. See my reply to your comment above for more clarity. |
There was a problem hiding this comment.
Thank you for the fix! These are great tests.
Non-blocking suggestion: agree with @kerwanp on some minor suggestions for test cleanup. We could deduplicate a little bit. Agree with your points @goce-cz about being explicit, but a lot of the tests have quite a lot of shared overlap, and very minor differences, that are easy to miss when modifying, and a few could be combined with some more clever iterating as you have
goce-cz
force-pushed
the
1695-allow-falsy-values-as-body
branch
from
449d7e8
to
e9d8506
Compare
|
I rebased and added the change-set. Hopefully correctly. |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "openapi-typescript": patch | |||
There was a problem hiding this comment.
Whoops! Didn’t catch this was for the wrong package. Should be openapi-fetch.
There was a problem hiding this comment.
Ohh... crap. Sorry. I'll issue a fix PR.
There was a problem hiding this comment.
Oh no, no worries; that’s easy to fix. It’s actually more important that you created the file in the correct PR, because then the changelog credits you correctly and links to the PR. But just pointing out for next time
There was a problem hiding this comment.
I see it has been already released. 😢
Changes
Resolves #1695
This PR ensures that all falsy values except
undefinedi.e.null,false,0and""are treated as a valid body. This means:bodySerializer, when specifiedHow to Review
Checklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)