-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: Add testDescription helper and update error processing #1
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: asararatnakar <[email protected]>
Signed-off-by: asararatnakar <[email protected]>
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.
Need to keep the separation for error handling, need smarter comparisons for responses with field errors
// Match stepzen AWS 60 second timeout. | ||
const defaultTimeout = 60000 | ||
|
||
function testDescription(testRoot, fullDirName) { |
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.
I'm not yet convinced this is suitable for a generic test harness, seems clearer and more typical just to have constants for describe
/ it
.
Maybe move to a separate PR.
// (3) not implemented - root value with field errors: {data: {customer: {name: "Fred" email:null}}, "errors":[...]} | ||
// (4) not implemented - root value with request errors: {"errors":[...]} | ||
// (3) root value with field errors: {data: {customer: {name: "Fred" email:null}}, "errors":[...]} | ||
// or request errors: {"errors":[...]} |
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.
Responses with request errors are different to responses with field errors, which is why they were separated. Please keep the separation.
if (Object.hasOwn(expected, "errors")) { | ||
chai.expect.fail("field errors in response not yet supported.") | ||
} | ||
if (Object.hasOwn(expected, "data") || Object.hasOwn(expected, "errors")) { |
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.
Just 2,3 means it's data && errors
.
if (Object.hasOwn(expected, "errors")) { | ||
chai.expect.fail("field errors in response not yet supported.") | ||
} | ||
if (Object.hasOwn(expected, "data") || Object.hasOwn(expected, "errors")) { | ||
chai.expect(response.body).to.deep.equal(expected); |
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.
Generically this will not work for expected with errors, which is why it needs to be separated.
This is because the order of errors cannot be guaranteed. Expected field errors need to be explicitly tested that a matching error with the expected path exists. Thus need to test that data matches and then process expected errors.
// defaultTimeout can be used in describe.timeout() | ||
// for requests against a graphql endpoint. | ||
// Match stepzen AWS 60 second timeout. | ||
const defaultTimeout = 60000 |
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.
Probably ok for now, but gqltest
was trying to stay GraphQL neutral (hence stepzen.js
). They may need to be some thought on how to handle default timeouts nicely.
Signed-off-by: asararatnakar [email protected]