-
Notifications
You must be signed in to change notification settings - Fork 345
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
[SO-135] Fix falsely required param bug #325
[SO-135] Fix falsely required param bug #325
Conversation
Is this related to #324 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just minor remarks.
@philsturgeon, no, not related to #324 |
output: BAD_OUTPUT, | ||
}), | ||
).toMatchSnapshot(); | ||
expect(await validator.validateOutput({ resource: httpOperations[1], output: BAD_OUTPUT })).toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(await validator.validateOutput({ resource: httpOperations[1], output: BAD_OUTPUT })).toMatchSnapshot(); | |
expect(validator.validateOutput({ resource: httpOperations[1], output: BAD_OUTPUT })).resolves.toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
You don’t need to return a promise from a test, you _can_ return a promise, but it’s not required.
The rule should be: only add async when the function has an await”, not “always have async for consistency.”
…--
Phil Sturgeon
@philsturgeon
On May 28, 2019, at 10:28, lag-of-death ***@***.***> wrote:
@lag-of-death commented on this pull request.
In packages/http/src/validator/__tests__/functional.spec.ts:
> @@ -18,6 +22,12 @@ describe('HttpValidator', () => {
it('returns validation errors for whole request structure', async () => {
expect(await validator.validateInput({ resource: httpOperations[2], input: BAD_INPUT })).toMatchSnapshot();
});
+
+ describe('when all required params are provided', () => {
+ it('returns no validation errors', async () => {
+ expect(await validator.validateInput({ resource: httpOperations[0], input: GOOD_INPUT })).toEqual([]);
I'm gonna go with async/await unless you think it's crucial to change it. And btw, all of the other tests are async/await. So let's stay consistent here as the alternatives are not in any way better.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Not exactly. This still works because Jest has an internal queue (when using The truth, in general, for other test frameworks and for the JavaScript stuff in general — you should always return or store the promise. |
@philsturgeon, I don't quite understand. I used |
The threading was lost because I replied via email. Don’t worry, it’s already merged. 👍🏼 |
SO-135
Checklist
[ ] Docs have been added / updated (for bug fixes / features)What kind of change does this PR introduce
Bug fix.
What is the current behavior
When all of the required query parameters are provided (for a request), we get a false bug:
{"path":["query"],"code":"type","message":"should be string","severity":0}
Does this PR introduce a breaking change
No.