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

graphql: migrate test cases from hive #470

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Sep 15, 2023

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
Copy link
Contributor Author

jsvisa commented Sep 15, 2023

@s1na please take a look

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

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

@s1na
Copy link
Contributor

s1na commented Sep 18, 2023

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
Copy link
Contributor Author

jsvisa commented Sep 18, 2023

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

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

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)

graphql.json Outdated Show resolved Hide resolved
@lightclient lightclient changed the title schema: add graphql query schema and tests graphql: migrate test cases from hive Sep 20, 2023
@lightclient lightclient added the A-test Area: testing label Sep 20, 2023
@jsvisa
Copy link
Contributor Author

jsvisa commented Sep 21, 2023

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
Copy link
Member

lightclient commented Sep 21, 2023

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
Copy link
Contributor Author

jsvisa commented Sep 22, 2023

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
Copy link
Contributor

s1na commented Oct 3, 2023

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.

Signed-off-by: jsvisa <[email protected]>
This reverts commit 0deb40b.

Signed-off-by: jsvisa <[email protected]>
This reverts commit c3325fd.

Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
This reverts commit 272fa23.

Signed-off-by: jsvisa <[email protected]>
@shemnon
Copy link
Contributor

shemnon commented Oct 19, 2023

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
Copy link
Contributor Author

jsvisa commented Oct 20, 2023

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
Copy link
Contributor

s1na commented Oct 23, 2023

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
Copy link
Contributor

shemnon commented Oct 23, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-test Area: testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants