-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(GraphQL): fixes flaky test for subscriptions. #6065
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jatindevdg, @MichaelJCompton, and @pawanrawal)
graphql/e2e/subscription/subscription_test.go, line 584 at r1 (raw file):
} addResult = add.ExecuteAsPost(t, graphQLEndpoint) require.Nil(t, addResult.Errors)
Why weren't we sending the request previously? Was it intentional or did we miss out?
we missed it somehow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @arijitAD, @jatindevdg, @MichaelJCompton, and @pawanrawal)
graphql/e2e/subscription/subscription_test.go, line 754 at r2 (raw file):
err = json.Unmarshal(res, &resp) require.NoError(t, err) require.Nil(t, resp.Errors)
Shouldn't we check resp.Errors
before we do the json.Unmarshal
?
graphql/e2e/subscription/subscription_test.go, line 768 at r2 (raw file):
// add delay for 2nd subscription to timeout // Wait for JWT to expire. time.Sleep(20 * time.Second)
Why does this need to be increased? Is 10 not enough?
graphql/e2e/subscription/subscription_test.go, line 785 at r2 (raw file):
} addResult = add.ExecuteAsPost(t, graphQLEndpoint)
Didn't we fix this before? I thought we did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @arijitAD, @MichaelJCompton, and @pawanrawal)
graphql/e2e/subscription/subscription_test.go, line 754 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Shouldn't we check
resp.Errors
before we do thejson.Unmarshal
?
no, i checked that, resp is filled while unmarshling, so we should check resp.errors after unmarshling.
graphql/e2e/subscription/subscription_test.go, line 768 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Why does this need to be increased? Is 10 not enough?
fixed it.
graphql/e2e/subscription/subscription_test.go, line 785 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Didn't we fix this before? I thought we did.
i missed it at some places.fixed it.
…ubscription-fleakyTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 3 rules errored during the review.
…ubscription-fleakyTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issues found. 9 rules errored during the review.
@@ -71,8 +71,9 @@ const ( | |||
` | |||
) | |||
|
|||
func TestSubscription(t *testing.T) { | |||
var subExp = 3 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid global variables to improve readability and reduce complexity
…ubscription-fleakyTest
(cherry picked from commit b4c6a73)
(cherry picked from commit b4c6a73)
Fixes DGRAPH-2305
This change is
Docs Preview: