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

adding fuzz tests #392

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jveiga
Copy link

@jveiga jveiga commented May 17, 2020

referencing #386
Added Fuzz tests for query.Parse and query.Parse.
How to run with gotip and run

gotip test --fuzz=Fuzz ./internal/query
# or
gotip test --fuzz=Fuzz ./internal/schema
# or
gotip test --fuzz=FuzzSchema .

Note that calling trying to call test fuzz with multiple matching tests will fail with

gotip test --fuzz=Fuzz ./internal/...
cannot use -fuzz flag with multiple packages

@agnivade
Copy link
Contributor

This is a great idea @jveiga ! Now that we have official fuzz support in Go 1.18, would you be willing to update the PR to use that? Of course, we would need to wait for a few weeks for it to be released, but in the meanwhile we can start migrating this PR.

Any thoughts?

@pavelnikolov
Copy link
Member

Yes, indeed the fuzz testing is an amazing idea. Looking forward to Go 1.18.
It would be ideal if we could generate more sophisticated queries based on a given schema.

@jveiga
Copy link
Author

jveiga commented Dec 11, 2021

This is a great idea @jveiga ! Now that we have official fuzz support in Go 1.18, would you be willing to update the PR to use that? Of course, we would need to wait for a few weeks for it to be released, but in the meanwhile we can start migrating this PR.

That's a great approach. I would ask then which functions to fuzz.

Yes, indeed the fuzz testing is an amazing idea. Looking forward to Go 1.18.
It would be ideal if we could generate more sophisticated queries based on a given schema.

This kind of fuzzing is easier to do with go-fuzz which I can also add an integration with. I will see what kind of features go 1.18 will support and update this branch with findings

@agnivade
Copy link
Contributor

That's a great approach. I would ask then which functions to fuzz.

I think ParseSchema is a good start. We can keep the resolver constant, and keep changing the schemaString. Later, we can even add (*Schema).Exec

@pavelnikolov
Copy link
Member

The real value would come from fuzzing the Exec because this is where user input comes during request time.

@jveiga
Copy link
Author

jveiga commented Dec 13, 2021

updated with graphql_test.FuzzSchemaExec

Feel free to add more methods to the implementation of query

Also, I realised that calling fuzz stores a local folder with regressions like

❯ tree testdata
testdata
└── fuzz
    └── FuzzSchemaExec
        ├── 671f05b72e69d643e6570ec3777c27147cb4ec551090f2d71ce1c4db1010c686
        └── c843983602e04670a35adb41fb79674de6dd362f431acf11c4f8f6fe7a78df84

Should I add these folders to .gitignore?

go.mod Outdated Show resolved Hide resolved
func FuzzParse(f *testing.F){
f.Fuzz(func(t *testing.T, schemaString string, useStringDescriptions bool){
s := schema.New()
schema.Parse(s, schemaString, useStringDescriptions)
Copy link
Member

Choose a reason for hiding this comment

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

When using string descriptions the syntax changes. So maybe we'll end up with a bunch of syntax errors which are expected. I wonder how beneficial is such a test. As in what are the type of errors that we could catch and prevent.

@jveiga
Copy link
Author

jveiga commented Dec 15, 2021

@pavelnikolov running random inputs helps finding unexpected bugs, out of bounds errors, logic errors, etc.
This PR started when I found an input that sent the parser into an infinite loop. That's one of the benefits of fuzzing these inputs. It is possible to add an initial corpus with F.Add, we can feed it basic query, mutation and schemas to have better inputs.

@pavelnikolov
Copy link
Member

I guess we'll have to wait for Go 1.18 to be released early in 2022 to merge this PR

@pavelnikolov pavelnikolov added this to the v1.7.0 milestone Mar 9, 2023
@pavelnikolov pavelnikolov self-assigned this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants