graphql: migrate test cases from hive

[jsvisa]

At present, hive/graphql maintains some test cases independently(https://github.com/ethereum/hive/tree/master/simulators/ethereum/graphql), which is troublesome for different clients when they need to change the graphql RPC, so we can maintain all those files here(all in this repo), and in hive's side, only need to git clone this repo and run the tests like normal rpc test(https://github.com/ethereum/hive/blob/master/simulators/ethereum/rpc-compat/Dockerfile#L6-L22).

[jsvisa]

@s1na please take a look

[s1na]

Is there a specific reason you kept graphql.json in the root directory?

[s1na]

IMO we should also follow the rpc-compat's lead and bring genesis.json and chain.rlp files here too. The tests are defined based on the given chain.

[jsvisa]

Is there a specific reason you kept graphql.json in the root directory?

Not sure if other clients need to rely on this file path, so no change it

[s1na]

Looks good! Nice that now request queries will be validated against the schema

[lightclient]

Overall LGTM, but a couple nits:

• why the numbered prefix of the test folders?

• Can we restructure the folders a little bit so it looks like:

tests/
  jsonrpc/
  graphql/

• why the request/response files? we have this format for RPC tests. Can we reuse it here for consistency?

>> { ---input--- }
<< { ---output --- }

(this probably also implies we should rethink the folders in the tests/graphql folder as they stand currently)

[jsvisa]

why the numbered prefix of the test folders?

Haha, just copied from the hive, may be used for sorting, I can remove them.

Can we restructure the folders a little bit so it looks like:

I totally agree with that structure, however, this is not compatible with hive, so we also need to adjust the hive's code.

why the request/response files?

the request is a type of graphql, which looks weird in the JSON file, so I put them into two files, in order to format :)

AFAIK, these test files are maintained by @s1na and another developer from besu team, they need to modify the response files a lot(maybe by tool, or manually), you can see the response is an array of all possible results: https://github.com/ethereum/hive/blob/322945164a97868825c49619182251198f0ea318/simulators/ethereum/graphql/graphql.go#L184-L189

	// return if a response matches. If not, error out.
	for _, response := range tc.gqlTest.Responses {
		if reflect.DeepEqual(response, got) {
			return nil
		}
	}

we have this format for RPC tests. Can we reuse it here for consistency?

Yeah, I can reconstruct them like input/outputs, but it's hard to read a long line, so maybe we'll put the original formatted request+response file(s) in rpctestgen or other tools and put the merged files in this project.

[lightclient]

I totally agree with that structure, however, this is not compatible with hive, so we also need to adjust the hive's code.

Can you expand on why this isn't possible? We should be able to modify how the simulator pulls the testcases.

--

I want to use the same *.io format as the current RPC tests unless there is a compelling reason to deviate. How are these generated? I feel like a tool should be generating them?

[jsvisa]

Can you expand on why this isn't possible? We should be able to modify how the simulator pulls the testcases.

All we have to do is change the path, eg https://github.com/ethereum/hive/blob/master/simulators/ethereum/rpc-compat/Dockerfile#L22:

-COPY --from=builder /execution-apis/tests ./tests
+COPY --from=builder /execution-apis/tests/jsonrpc ./tests

I think we can split this PR into two, the other one used to change the jsonrpc location.

I want to use the same *.io format as the current RPC tests unless there is a compelling reason to deviate.

AFAIK, the implementation of graphql in different clients may differ, currently, we are testing the same request with any possible results, e.g.:

{
  "responses": [
    {
      "errors": [
        {
          "message": "Exception while fetching data (/account) : Invalid params",
          "locations": [
            {
              "line": 1,
              "column": 2
            }
          ],
          "path": ["account"],
          "extensions": {
            "errorCode": -32602,
            "errorMessage": "Invalid params",
            "classification": "DataFetchingException"
          }
        }
      ],
      "data": null
    },
    {
      "data": {
        "block": null
      }
    }
  ],
  "statusCode": 400
}

I don't like that style either, the testing is also tied to the client, which is not the original purpose of the spec. We can make the graphql spec simple and effective, just like jsonrpc.

@s1na please correct me if I'm wrong or missing something, thanks.

How are these generated? I feel like a tool should be generating them?

Yeah, I'll send a PR in rpctestgen.

[s1na]

I want to use the same *.io format as the current RPC tests unless there is a compelling reason to deviate. How are these generated? I feel like a tool should be generating them?

@lightclient I think that's a good idea. Right now the format is not directly mappable to one request and one response. There is the status code extra and some of the tests have 2 accepted responses (terrible I know). If you don't mind we move forward with how it is now and change the structure after this issue is resolved. I've created this ticket #475 so we can move forward with agreeing on one output.

[shemnon]

There needs to be some room for variability in the spec in two cases: (a) estimates and (b) errors. Simple input/output mapping does not allow for things to be variable where they should be allowed to be variable.

Ideally we would have some way to encode "a gas value was returned" and "an error was returned" without specifying what they are. Multiple returns was the way it was done before.

This format could be extended to support multiple responses with multiple "<< " lines (perhaps with "<<+", on all but the last option) but a regexp I think would be better ("<<*"), however it would require a canonical rendering of the json by the harness, which shouldn't be too bad.

[jsvisa]

There needs to be some room for variability in the spec in two cases: (a) estimates and (b) errors.

@shemnon thanks for the advice, currently, for the error case in graphql test, I am only tested with the errors key and HTTP StatusCode, the body is omitted. Because the error message or format was not defined in our spec.

[s1na]

Ideally we would have some way to encode "a gas value was returned" and "an error was returned" without specifying what they are. Multiple returns was the way it was done before.
This format could be extended to support multiple responses with multiple "<< " lines (perhaps with "<<+", on all but the last option) but a regexp I think would be better ("<<*"), however it would require a canonical rendering of the json by the harness, which shouldn't be too bad.

Staying flexible for error messages should be straightforward because they are at a predictable path in the response. However we should think about standardizing error codes if we forgo the message check.
As for queries like eth_gasPrice, as @lightclient pointed out in another channel, the graphql schema is enough to do a type-check (i.e. that the estimate is a hex encoded number). However this will be a relatively big task which is not available off-the-shelf from graphql libraries. It needs to specifically match the response to the query and do a type-check of every field, including the custom scalar types we have defined.

IMO it's ok to keep the eth_gasPrice testcase in and let it fail and fix this in a future PR.

[shemnon]

IMO it's ok to keep the eth_gasPrice testcase in and let it fail and fix this in a future PR.

But then which clients answer is kept in the automated tests as "canonical"? The other clients will look non-compliant. I'd prefer the test be removed until the harness can respect the specified tolerance of variability.