Skip to content

Commit

Permalink
chore(test): Refactor GraphQL tests to use best practices (GRAPHQL-88…
Browse files Browse the repository at this point in the history 
…5) (#7016)

This PR does the following things:
* adds a counter in `/probe/graphql` response which indicate how many time the GraphQL schema has been updated.
* adds some common test methods to update the GQL schema which ensure schema update using the counter.
* Refactors existing tests to use those methods in order to update the GQL schema.
* adds the best practice of using `common.RequireNoGQLErrors` instead of `require.Nil` for verifying that the `GraphQLResponse` has no errors.
* enables some skipped tests in `graphql/e2e/admin_auth`.

With this PR, all the flakiness around GraphQL schema updates should no longer be there.
  • Loading branch information
@abhimanyusinghgaur
abhimanyusinghgaur authored Dec 2, 2020
1 parent 30ee966 commit 7c077e5
Show file tree
Hide file tree
Showing 21 changed files with 870 additions and 1,408 deletions.
4 changes: 3 additions & 1 deletion dgraph/cmd/alpha/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,7 @@ func setupServer(closer *z.Closer) {
// Global Epoch is a lockless synchronization mechanism for graphql service.
// It's is just an atomic counter used by the graphql subscription to update its state.
// It's is used to detect the schema changes and server exit.
// It is also reported by /probe/graphql endpoint as the schemaUpdateCounter.

// Implementation for schema change:
// The global epoch is incremented when there is a schema change.
Expand All @@ -458,7 +459,8 @@ func setupServer(closer *z.Closer) {
}
w.WriteHeader(httpStatusCode)
w.Header().Set("Content-Type", "application/json")
x.Check2(w.Write([]byte(fmt.Sprintf(`{"status":"%s"}`, healthStatus.StatusMsg))))
x.Check2(w.Write([]byte(fmt.Sprintf(`{"status":"%s","schemaUpdateCounter":%d}`,
healthStatus.StatusMsg, atomic.LoadUint64(&globalEpoch)))))
})
http.Handle("/admin", allowedMethodsHandler(allowedMethods{
http.MethodGet: true,
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ require (
github.com/pkg/errors v0.9.1
github.com/pkg/profile v1.2.1
github.com/prometheus/client_golang v0.9.3
github.com/prometheus/common v0.4.1 // indirect
github.com/prometheus/common v0.4.1
github.com/prometheus/procfs v0.0.0-20190517135640-51af30a78b0e // indirect
github.com/soheilhy/cmux v0.1.4
github.com/spf13/cast v1.3.0
Expand Down
4 changes: 4 additions & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ github.com/agnivade/levenshtein v1.0.1/go.mod h1:CURSv5d9Uaml+FovSIICkLbAUZ9S4Rq
github.com/agnivade/levenshtein v1.0.3 h1:M5ZnqLOoZR8ygVq0FfkXsNOKzMCk0xRiow0R5+5VkQ0=
github.com/agnivade/levenshtein v1.0.3/go.mod h1:4SFRZbbXWLF4MU1T9Qg0pGgH3Pjs+t6ie5efyrwRJXs=
github.com/ajg/form v1.5.1/go.mod h1:uL1WgH+h2mgNtvBq0339dVnzXdBETtL2LeUXaIv25UY=
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc h1:cAKDfWh5VpdgMhJosfJnn5/FoN2SRZ4p7fJNX58YPaU=
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf h1:qet1QNfXsQxTZqLG4oE62mJzwPIB8+Tee4RNCL9ulrY=
github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 h1:bvNMNQO63//z+xNgfBlViaCIJKLlCJ6/fmUseuG0wVQ=
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8=
Expand Down Expand Up @@ -476,6 +478,7 @@ github.com/sirupsen/logrus v1.0.5/go.mod h1:pMByvHTf9Beacp5x1UXfOR9xyW/9antXMhjM
github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
github.com/sirupsen/logrus v1.4.1/go.mod h1:ni0Sbl8bgC9z8RoU9G6nDWqqs/fq4eDPysMBDgk/93Q=
github.com/sirupsen/logrus v1.5.0/go.mod h1:+F7Ogzej0PZc/94MaYx/nvG9jOFMD2osvC3s+Squfpo=
github.com/sirupsen/logrus v1.6.0 h1:UBcNElsrwanuuMsnGSlYmtmgbb23qDR5dG+6X6Oo89I=
github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88=
github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d h1:zE9ykElWQ6/NYmHa3jpm/yHnI4xSofP+UP6SpjHcSeM=
github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
Expand Down Expand Up @@ -741,6 +744,7 @@ google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyac
gopkg.in/DataDog/dd-trace-go.v1 v1.13.1 h1:oTzOClfuudNhW9Skkp2jxjqYO92uDKXqKLbiuPA13Rk=
gopkg.in/DataDog/dd-trace-go.v1 v1.13.1/go.mod h1:DVp8HmDh8PuTu2Z0fVVlBsyWaC++fzwVCaGWylTe3tg=
gopkg.in/airbrake/gobrake.v2 v2.0.9/go.mod h1:/h5ZAUhDkGaJfjzjKLSjv6zCL6O0LLBxU4K+aSYdM/U=
gopkg.in/alecthomas/kingpin.v2 v2.2.6 h1:jMFz6MfLP0/4fUyZle81rXUoxOBFi19VUFKVDOQfozc=
gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=
gopkg.in/asn1-ber.v1 v1.0.0-20181015200546-f715ec2f112d/go.mod h1:cuepJuh7vyXfUyUwEgHQXw849cJrilpS5NeIjOWESAw=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
Expand Down
59 changes: 20 additions & 39 deletions graphql/e2e/admin_auth/poorman_auth/admin_auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"net/http"
"testing"

"github.com/dgraph-io/dgraph/x"

"github.com/stretchr/testify/require"

"github.com/dgraph-io/dgraph/graphql/e2e/common"
Expand All @@ -32,49 +34,28 @@ const (
)

func TestAdminOnlyPoorManAuth(t *testing.T) {
t.Skipf("TODO: This test is failing for some reason. FIX IT.")
schema := `type Person {
id: ID!
name: String!
}`
// without X-Dgraph-AuthToken should give error
params := getUpdateGqlSchemaParams()
assertAuthTokenError(t, common.GraphqlAdminURL, params)
headers := http.Header{}
assertAuthTokenError(t, schema, headers)

// setting a wrong value for the token should still give error
params.Headers.Set(authTokenHeader, wrongAuthToken)
assertAuthTokenError(t, common.GraphqlAdminURL, params)
headers.Set(authTokenHeader, wrongAuthToken)
assertAuthTokenError(t, schema, headers)

// setting correct value for the token should not give any GraphQL error
params.Headers.Set(authTokenHeader, authToken)
common.RequireNoGQLErrors(t, params.ExecuteAsPost(t, common.GraphqlAdminURL))
// setting correct value for the token should successfully update the schema
headers.Set(authTokenHeader, authToken)
common.SafelyUpdateGQLSchema(t, common.Alpha1HTTP, schema, headers)
}


func assertAuthTokenError(t *testing.T, url string, params *common.GraphQLParams) {
req, err := params.CreateGQLPost(url)
require.NoError(t, err)

resp, err := common.RunGQLRequest(req)
require.NoError(t, err)
require.JSONEq(t, `{
"errors":[{
"message":"Invalid X-Dgraph-AuthToken",
"extensions":{"code":"ErrorUnauthorized"}
}]
}`, string(resp))
}

func getUpdateGqlSchemaParams() *common.GraphQLParams {
schema := `type Person {
id: ID!
name: String!
}`
return &common.GraphQLParams{
Query: `mutation updateGQLSchema($sch: String!) {
updateGQLSchema(input: { set: { schema: $sch }}) {
gqlSchema {
schema
}
}
}`,
Variables: map[string]interface{}{"sch": schema},
Headers: http.Header{},
}
func assertAuthTokenError(t *testing.T, schema string, headers http.Header) {
resp := common.RetryUpdateGQLSchema(t, common.Alpha1HTTP, schema, headers)
require.Equal(t, x.GqlErrorList{{
Message: "Invalid X-Dgraph-AuthToken",
Extensions: map[string]interface{}{"code": "ErrorUnauthorized"},
}}, resp.Errors)
require.Nil(t, resp.Data)
}
72 changes: 24 additions & 48 deletions graphql/e2e/admin_auth/poorman_auth_with_acl/admin_auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,60 +37,55 @@ const (
)

func TestLoginWithPoorManAuth(t *testing.T) {
t.Skipf("TODO: This test is failing for some reason. FIX IT.")
// without X-Dgraph-AuthToken should give error
params := getGrootLoginParams()
assertAuthTokenError(t, common.GraphqlAdminURL, params)
assertAuthTokenError(t, params.ExecuteAsPost(t, common.GraphqlAdminURL))

// setting a wrong value for the token should still give error
params.Headers.Set(authTokenHeader, wrongAuthToken)
assertAuthTokenError(t, common.GraphqlAdminURL, params)
assertAuthTokenError(t, params.ExecuteAsPost(t, common.GraphqlAdminURL))

// setting correct value for the token should not give any GraphQL error
params.Headers.Set(authTokenHeader, authToken)
common.RequireNoGQLErrors(t, params.ExecuteAsPost(t, common.GraphqlAdminURL))
}

func TestAdminPoorManWithAcl(t *testing.T) {
t.Skipf("TODO: This test is failing for some reason. FIX IT.")
schema := `type Person {
id: ID!
name: String!
}`
// without auth token and access JWT headers, should give auth token related error
params := getUpdateGqlSchemaParams()
assertAuthTokenError(t, common.GraphqlAdminURL, params)
headers := http.Header{}
assertAuthTokenError(t, common.RetryUpdateGQLSchema(t, common.Alpha1HTTP, schema, headers))

// setting a wrong value for the auth token should still give auth token related error
params.Headers.Set(authTokenHeader, wrongAuthToken)
assertAuthTokenError(t, common.GraphqlAdminURL, params)
headers.Set(authTokenHeader, wrongAuthToken)
assertAuthTokenError(t, common.RetryUpdateGQLSchema(t, common.Alpha1HTTP, schema, headers))

// setting correct value for the auth token should now give ACL related GraphQL error
params.Headers.Set(authTokenHeader, authToken)
assertMissingAclError(t, params)
headers.Set(authTokenHeader, authToken)
assertMissingAclError(t, common.RetryUpdateGQLSchema(t, common.Alpha1HTTP, schema, headers))

// setting wrong value for the access JWT should still give ACL related GraphQL error
params.Headers.Set(accessJwtHeader, wrongAuthToken)
assertBadAclError(t, params)
headers.Set(accessJwtHeader, wrongAuthToken)
assertBadAclError(t, common.RetryUpdateGQLSchema(t, common.Alpha1HTTP, schema, headers))

// setting correct value for both tokens should not give errors
accessJwt, _ := grootLogin(t)
params.Headers.Set(accessJwtHeader, accessJwt)
common.RequireNoGQLErrors(t, params.ExecuteAsPost(t, common.GraphqlAdminURL))
headers.Set(accessJwtHeader, accessJwt)
common.AssertUpdateGQLSchemaSuccess(t, common.Alpha1HTTP, schema, headers)
}

func assertAuthTokenError(t *testing.T, url string, params *common.GraphQLParams) {
req, err := params.CreateGQLPost(url)
require.NoError(t, err)

resp, err := common.RunGQLRequest(req)
require.NoError(t, err)
require.JSONEq(t, `{
"errors":[{
"message":"Invalid X-Dgraph-AuthToken",
"extensions":{"code":"ErrorUnauthorized"}
}]
}`, string(resp))
func assertAuthTokenError(t *testing.T, resp *common.GraphQLResponse) {
require.Equal(t, x.GqlErrorList{{
Message: "Invalid X-Dgraph-AuthToken",
Extensions: map[string]interface{}{"code": "ErrorUnauthorized"},
}}, resp.Errors)
require.Nil(t, resp.Data)
}

func assertMissingAclError(t *testing.T, params *common.GraphQLParams) {
resp := params.ExecuteAsPost(t, common.GraphqlAdminURL)
func assertMissingAclError(t *testing.T, resp *common.GraphQLResponse) {
require.Equal(t, x.GqlErrorList{{
Message: "resolving updateGQLSchema failed because rpc error: code = PermissionDenied desc = no accessJwt available",
Locations: []x.Location{{
Expand All @@ -100,8 +95,7 @@ func assertMissingAclError(t *testing.T, params *common.GraphQLParams) {
}}, resp.Errors)
}

func assertBadAclError(t *testing.T, params *common.GraphQLParams) {
resp := params.ExecuteAsPost(t, common.GraphqlAdminURL)
func assertBadAclError(t *testing.T, resp *common.GraphQLResponse) {
require.Equal(t, x.GqlErrorList{{
Message: "resolving updateGQLSchema failed because rpc error: code = Unauthenticated desc = unable to parse jwt token:token contains an invalid number of segments",
Locations: []x.Location{{
Expand Down Expand Up @@ -148,21 +142,3 @@ func getGrootLoginParams() *common.GraphQLParams {
Headers: http.Header{},
}
}

func getUpdateGqlSchemaParams() *common.GraphQLParams {
schema := `type Person {
id: ID!
name: String!
}`
return &common.GraphQLParams{
Query: `mutation updateGQLSchema($sch: String!) {
updateGQLSchema(input: { set: { schema: $sch }}) {
gqlSchema {
schema
}
}
}`,
Variables: map[string]interface{}{"sch": schema},
Headers: http.Header{},
}
}
34 changes: 17 additions & 17 deletions graphql/e2e/auth/add_mutation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (p *Project) delete(t *testing.T, user, role string) {
Variables: map[string]interface{}{"ids": []string{p.ProjID}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (c *Column) delete(t *testing.T, user, role string) {
Expand All @@ -55,7 +55,7 @@ func (c *Column) delete(t *testing.T, user, role string) {
Variables: map[string]interface{}{"colids": []string{c.ColID}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (i *Issue) delete(t *testing.T, user, role string) {
Expand All @@ -71,7 +71,7 @@ func (i *Issue) delete(t *testing.T, user, role string) {
Variables: map[string]interface{}{"ids": []string{i.Id}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (l *Log) delete(t *testing.T, user, role string) {
Expand All @@ -87,7 +87,7 @@ func (l *Log) delete(t *testing.T, user, role string) {
Variables: map[string]interface{}{"ids": []string{l.Id}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (m *Movie) delete(t *testing.T, user, role string) {
Expand All @@ -103,7 +103,7 @@ func (m *Movie) delete(t *testing.T, user, role string) {
Variables: map[string]interface{}{"ids": []string{m.Id}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (a *Author) delete(t *testing.T) {
Expand All @@ -118,7 +118,7 @@ func (a *Author) delete(t *testing.T) {
Variables: map[string]interface{}{"ids": []string{a.Id}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (q *Question) delete(t *testing.T, user string) {
Expand All @@ -134,7 +134,7 @@ func (q *Question) delete(t *testing.T, user string) {
Variables: map[string]interface{}{"ids": []string{q.Id}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func (f *FbPost) delete(t *testing.T, user, role string) {
Expand All @@ -150,7 +150,7 @@ func (f *FbPost) delete(t *testing.T, user, role string) {
Variables: map[string]interface{}{"ids": []string{f.Id}},
}
gqlResponse := getParams.ExecuteAsPost(t, common.GraphqlURL)
require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)
}

func TestAuth_AddOnTypeWithRBACRuleOnInterface(t *testing.T) {
Expand Down Expand Up @@ -237,7 +237,7 @@ func TestAuth_AddOnTypeWithRBACRuleOnInterface(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -335,7 +335,7 @@ func TestAuth_AddOnTypeWithGraphTraversalRuleOnInterface(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -448,7 +448,7 @@ func TestAddDeepFilter(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -551,7 +551,7 @@ func TestAddOrRBACFilter(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -630,7 +630,7 @@ func TestAddAndRBACFilterMultiple(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -705,7 +705,7 @@ func TestAddAndRBACFilter(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -809,7 +809,7 @@ func TestAddComplexFilter(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -877,7 +877,7 @@ func TestAddRBACFilter(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down Expand Up @@ -941,7 +941,7 @@ func TestAddGQLOnly(t *testing.T) {
continue
}

require.Nil(t, gqlResponse.Errors)
common.RequireNoGQLErrors(t, gqlResponse)

err := json.Unmarshal([]byte(tcase.result), &expected)
require.NoError(t, err)
Expand Down
Loading

0 comments on commit 7c077e5

Please sign in to comment.