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

Tests to build #75

Merged
merged 3 commits into from May 22, 2020
Merged

Tests to build #75

merged 3 commits into from May 22, 2020

Conversation

sjparsons
Copy link
Collaborator

@sjparsons sjparsons commented Mar 24, 2020

The document included in this PR proposes a beginning set of tests based on the current draft spec that can be implemented. All comments are welcome as we start to evolve this list. Especially things that I have not yet included.

#69

This was referenced Mar 24, 2020

Server may response with non-200 status codes

Syntax error in the query should result in either a 200 with an `error` attribute in the body or a 400 response.
Copy link
Contributor

Choose a reason for hiding this comment

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

For syntax error spec now allows only 4xx and 5xx error codes
https://github.com/APIs-guru/graphql-over-http#status-codes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point. I feel conflicted over this since I think the spec should probably be more permissive on this point. But my concern is better to raise on the main spec, than here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point is that 200 may be returned only when GraphQL execution actually started. Otherwise there is no "GraphQL context". Syntax error or other errors before execution should return 4xx or 5xx.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

A little editorial; I've tried to be strict with the MUST/MUST NOT/MAY/MAY NOT/SHOULD/SHOULD NOT.

working-group/test-suite.md Outdated Show resolved Hide resolved
working-group/test-suite.md Outdated Show resolved Hide resolved
working-group/test-suite.md Outdated Show resolved Hide resolved
working-group/test-suite.md Outdated Show resolved Hide resolved
working-group/test-suite.md Outdated Show resolved Hide resolved
working-group/test-suite.md Outdated Show resolved Hide resolved

## Responses

A 200 response should include a body. The body should include at least a `data` attribute or an `errors` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A 200 response should include a body. The body should include at least a `data` attribute or an `errors` attribute.
A 200 response MUST include a body. If the body has `Content-Type` of `application/json`, `application/graphql` or `application/graphql+json`, the body MUST be a JSON object that conforms to the ["Response Format" as specified in the GraphQL Specification](https://spec.graphql.org/June2018/#sec-Response-Format).

Copy link
Member

Choose a reason for hiding this comment

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

I think application/graphql is not meant to be used for responses, only for requests. It makes sense when sending a raw query string like so:

curl -v -H "Content-Type: application/graphql" -d "{ hello }"  "localhost:3000/graphql"

It might be a valid response header, but not to represent the result of a GraphQL operation. I guess it could be used for something like a schema/query generator.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe the content-type application/graphql+json can refer to a GraphQL JSON response document and the GraphQL request body. In other words, I think it should be a JSON document containing at least one of data or errors as top level attributes in a response.

When sending a POST request, a client should send application/graphql+json in the general case because they are also sending a JSON document, this time including a query, operationName and variables as the top-level attributes. The GraphQL query itself, which is found in this object (in the case of a POST request) at the query parameter should conform to the GraphQL specification.

I'm not clear on whether application/graphql is a synonym for application/graphql+json. Hypothetically, application/graphql could be distinct and refer to a valid GraphQL document string. However, we then would not be able to use it in GraphQL over HTTP since at no time do we send only a GraphQL query string to a server or receive only a GraphQL query string from a server.

working-group/test-suite.md Outdated Show resolved Hide resolved
working-group/test-suite.md Outdated Show resolved Hide resolved
[HTTP 1.1 Accept](https://tools.ietf.org/html/rfc7231#section-5.3.2)


Server may response with non-200 status codes
Copy link
Member

@benjie benjie Mar 27, 2020

Choose a reason for hiding this comment

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

Suggested change
Server may response with non-200 status codes
Server MUST NOT respond with any status code between 205 and 299 inclusive, or status codes 201, 202 or 203.
Server MUST respond with a 200 status code if the GraphQL request has been executed [as defined in "Executing Operations" in the GraphQL Specification](https://spec.graphql.org/June2018/#sec-Executing-Operations). Server MAY respond with a 204 status code if appropriate according to RFC7231.
Server MAY respond with non-200 status codes if the GraphQL request is invalid, or if the server is unable to process it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to put this language in the spec first and then let the test cases fall out from that. Could this be made as a proposal on #79?

@benjie benjie mentioned this pull request Apr 23, 2020
@sjparsons
Copy link
Collaborator Author

I've made some substantial changes to take into account most of the feedback. Some of this has resulted in another PR since I think some of the great suggestions here are spec-worthy, not just relevant for our tests.

I've also increased the clarity of the difference between ERROR and WARNING assertion failures. Hopefully those can be worked into our tests.

@sjparsons sjparsons merged commit d078a94 into master May 22, 2020
@sjparsons sjparsons deleted the test-suite branch May 22, 2020 22:08
@sjparsons sjparsons mentioned this pull request May 22, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants