From e4bb0b5ce666fe39ce3a4962537eb797188e6b7f Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Tue, 21 May 2024 21:18:58 +0800 Subject: [PATCH] chore: Remove client construction from SignUp/In funcs (#62789) These functions are used in test code, which makes it tricker to pass in loggers for checking requests and responses. Instead, make the API method-based. This introduces slightly more boilerplate since Client construction is fallible, but it allows calling code to pass in loggers more easily for debugging test failures. --- dev/authtest/code_intel_test.go | 7 ++- dev/authtest/main_test.go | 10 ++-- dev/authtest/organization_test.go | 8 +-- dev/authtest/repository_test.go | 8 +-- dev/authtest/site_admin_test.go | 9 ++-- dev/ci/integration/executors/tester/init.go | 11 ++-- dev/codeintel-qa/internal/graphql.go | 5 +- dev/gqltest/main_test.go | 12 +++-- dev/gqltest/site_config_test.go | 14 ++--- dev/gqltest/sub_repo_permissions_test.go | 7 ++- internal/cmd/init-sg/main.go | 20 ++++--- internal/gqltestutil/client.go | 59 +++++++++------------ 12 files changed, 90 insertions(+), 80 deletions(-) diff --git a/dev/authtest/code_intel_test.go b/dev/authtest/code_intel_test.go index 6c5ceb71fb5..47dbae92cf0 100644 --- a/dev/authtest/code_intel_test.go +++ b/dev/authtest/code_intel_test.go @@ -21,10 +21,9 @@ func TestCodeIntelEndpoints(t *testing.T) { // user should receive access denied for LSIF endpoints of repositories the user // does not have access to. const testUsername = "authtest-user-code-intel" - userClient, err := gqltestutil.SignUp(*baseURL, testUsername+"@sourcegraph.com", testUsername, "mysecurepassword") - if err != nil { - t.Fatal(err) - } + userClient, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + require.NoError(t, userClient.SignUp(testUsername+"@sourcegraph.com", testUsername, "mysecurepassword")) defer func() { err := client.DeleteUser(userClient.AuthenticatedUserID(), true) if err != nil { diff --git a/dev/authtest/main_test.go b/dev/authtest/main_test.go index a221184b416..42b49277243 100644 --- a/dev/authtest/main_test.go +++ b/dev/authtest/main_test.go @@ -54,15 +54,17 @@ func TestMain(m *testing.M) { log.Fatal("Failed to check if site needs init: ", err) } + client, err = gqltestutil.NewClient(*baseURL) + if err != nil { + log.Fatal("Failed to create gql client: ", err) + } if needsSiteInit { - client, err = gqltestutil.SiteAdminInit(*baseURL, *email, *username, *password) - if err != nil { + if err := client.SiteAdminInit(*email, *username, *password); err != nil { log.Fatal("Failed to create site admin: ", err) } log.Println("Site admin has been created:", *username) } else { - client, err = gqltestutil.SignIn(*baseURL, *email, *password) - if err != nil { + if err := client.SignIn(*email, *password); err != nil { log.Fatal("Failed to sign in:", err) } log.Println("Site admin authenticated:", *username) diff --git a/dev/authtest/organization_test.go b/dev/authtest/organization_test.go index 6fd8bf19a11..ddb43ca7c87 100644 --- a/dev/authtest/organization_test.go +++ b/dev/authtest/organization_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/gqltestutil" ) @@ -27,10 +28,9 @@ func TestOrganization(t *testing.T) { // "authtest-organization", the user should not have access to any of the // organization's resources. const testUsername = "authtest-user-organization" - userClient, err := gqltestutil.SignUp(*baseURL, testUsername+"@sourcegraph.com", testUsername, "mysecurepassword") - if err != nil { - t.Fatal(err) - } + userClient, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + require.NoError(t, userClient.SignUp(testUsername+"@sourcegraph.com", testUsername, "mysecurepassword")) defer func() { err := client.DeleteUser(userClient.AuthenticatedUserID(), true) if err != nil { diff --git a/dev/authtest/repository_test.go b/dev/authtest/repository_test.go index c3dde8ee52b..22a0a598ad2 100644 --- a/dev/authtest/repository_test.go +++ b/dev/authtest/repository_test.go @@ -5,6 +5,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/gqltestutil" @@ -84,10 +85,9 @@ func TestRepository(t *testing.T) { // Create a test user (authtest-user-repository) which is not a site admin, the // user should only have access to non-private repositories. const testUsername = "authtest-user-repository" - userClient, err := gqltestutil.SignUp(*baseURL, testUsername+"@sourcegraph.com", testUsername, "mysecurepassword") - if err != nil { - t.Fatal(err) - } + userClient, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + require.NoError(t, userClient.SignUp(testUsername+"@sourcegraph.com", testUsername, "mysecurepassword")) defer func() { err := client.DeleteUser(userClient.AuthenticatedUserID(), true) if err != nil { diff --git a/dev/authtest/site_admin_test.go b/dev/authtest/site_admin_test.go index 8d1e3f5933a..f76f883040a 100644 --- a/dev/authtest/site_admin_test.go +++ b/dev/authtest/site_admin_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/gqltestutil" ) @@ -15,10 +17,9 @@ func TestSiteAdminEndpoints(t *testing.T) { // Create a test user (authtest-user-1) which is not a site admin, the user // should receive access denied for site admin endpoints. const testUsername = "authtest-user-1" - userClient, err := gqltestutil.SignUp(*baseURL, testUsername+"@sourcegraph.com", testUsername, "mysecurepassword") - if err != nil { - t.Fatal(err) - } + userClient, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + require.NoError(t, userClient.SignUp(testUsername+"@sourcegraph.com", testUsername, "mysecurepassword")) defer func() { err := client.DeleteUser(userClient.AuthenticatedUserID(), true) if err != nil { diff --git a/dev/ci/integration/executors/tester/init.go b/dev/ci/integration/executors/tester/init.go index d02d8a7e42b..03fb3e915f4 100644 --- a/dev/ci/integration/executors/tester/init.go +++ b/dev/ci/integration/executors/tester/init.go @@ -27,16 +27,17 @@ func initAndAuthenticate() (*gqltestutil.Client, error) { return nil, errors.Wrap(err, "failed to check if site needs init") } - var client *gqltestutil.Client + client, err := gqltestutil.NewClient(SourcegraphEndpoint) + if err != nil { + return nil, errors.Wrap(err, "failed to create gql client") + } if needsSiteInit { - client, err = gqltestutil.SiteAdminInit(SourcegraphEndpoint, adminEmail, adminUsername, adminPassword) - if err != nil { + if err := client.SiteAdminInit(adminEmail, adminUsername, adminPassword); err != nil { return nil, errors.Wrap(err, "failed to create site admin") } log.Println("Site admin has been created:", adminUsername) } else { - client, err = gqltestutil.SignIn(SourcegraphEndpoint, adminEmail, adminPassword) - if err != nil { + if err = client.SignIn(adminEmail, adminPassword); err != nil { return nil, errors.Wrap(err, "failed to sign in") } log.Println("Site admin authenticated:", adminUsername) diff --git a/dev/codeintel-qa/internal/graphql.go b/dev/codeintel-qa/internal/graphql.go index d5cab17acd6..5947b0d4778 100644 --- a/dev/codeintel-qa/internal/graphql.go +++ b/dev/codeintel-qa/internal/graphql.go @@ -11,7 +11,10 @@ var ( ) func InitializeGraphQLClient() (err error) { - client, err = gqltestutil.NewClient(SourcegraphEndpoint, requestWriter.Write, responseWriter.Write) + client, err = gqltestutil.NewClient(SourcegraphEndpoint, gqltestutil.ClientOption{ + GraphQLRequestLogger: requestWriter.Write, + GraphQLResponseLogger: responseWriter.Write, + }) return err } diff --git a/dev/gqltest/main_test.go b/dev/gqltest/main_test.go index 9bf7de35046..53dd63c7e42 100644 --- a/dev/gqltest/main_test.go +++ b/dev/gqltest/main_test.go @@ -66,15 +66,17 @@ func TestMain(m *testing.M) { log.Fatal("Failed to check if site needs init:", err) } + client, err = gqltestutil.NewClient(*baseURL) + if err != nil { + log.Fatal("Failed to create gql client: ", err) + } if needsSiteInit { - client, err = gqltestutil.SiteAdminInit(*baseURL, *email, *username, *password) - if err != nil { - log.Fatal("Failed to create site admin:", err) + if err := client.SiteAdminInit(*email, *username, *password); err != nil { + log.Fatal("Failed to create site admin: ", err) } log.Println("Site admin has been created:", *username) } else { - client, err = gqltestutil.SignIn(*baseURL, *email, *password) - if err != nil { + if err := client.SignIn(*email, *password); err != nil { log.Fatal("Failed to sign in:", err) } log.Println("Site admin authenticated:", *username) diff --git a/dev/gqltest/site_config_test.go b/dev/gqltest/site_config_test.go index 8e2b894d347..7d0d0abf339 100644 --- a/dev/gqltest/site_config_test.go +++ b/dev/gqltest/site_config_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "github.com/sourcegraph/sourcegraph/internal/gqltestutil" "github.com/sourcegraph/sourcegraph/schema" ) @@ -14,10 +16,9 @@ func TestSiteConfig(t *testing.T) { t.Run("builtin auth provider: allowSignup", func(t *testing.T) { // Sign up a new user is allowed by default. const testUsername1 = "gqltest-auth-user-1" - testClient1, err := gqltestutil.SignUp(*baseURL, testUsername1+"@sourcegraph.com", testUsername1, "mysecurepassword") - if err != nil { - t.Fatal(err) - } + testClient1, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + require.NoError(t, testClient1.SignUp(testUsername1+"@sourcegraph.com", testUsername1, "mysecurepassword")) removeTestUserAfterTest(t, testClient1.AuthenticatedUserID()) // Update site configuration to not allow sign up for builtin auth provider. @@ -42,8 +43,9 @@ func TestSiteConfig(t *testing.T) { err = gqltestutil.Retry(5*time.Second, func() error { // Sign up a new user should fail. const testUsername2 = "gqltest-auth-user-2" - testClient2, err := gqltestutil.SignUp(*baseURL, testUsername2+"@sourcegraph.com", testUsername2, "mysecurepassword") - if err != nil { + testClient2, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + if err := testClient2.SignUp(testUsername2+"@sourcegraph.com", testUsername2, "mysecurepassword"); err != nil { if strings.Contains(err.Error(), "Signup is not enabled") { return nil } diff --git a/dev/gqltest/sub_repo_permissions_test.go b/dev/gqltest/sub_repo_permissions_test.go index da37b331ed8..2c05df960eb 100644 --- a/dev/gqltest/sub_repo_permissions_test.go +++ b/dev/gqltest/sub_repo_permissions_test.go @@ -338,10 +338,9 @@ func createTestUserAndWaitForRepo(t *testing.T) (*gqltestutil.Client, string, er // Alice doesn't have access to Security directory. (there is a .sh file) alicePassword := "alicessupersecurepassword" t.Log("Creating Alice") - userClient, err := gqltestutil.SignUpOrSignIn(*baseURL, aliceEmail, aliceUsername, alicePassword) - if err != nil { - t.Fatal(err) - } + userClient, err := gqltestutil.NewClient(*baseURL) + require.NoError(t, err) + require.NoError(t, userClient.SignUp(aliceEmail, aliceUsername, alicePassword)) aliceID := userClient.AuthenticatedUserID() removeTestUserAfterTest(t, aliceID) diff --git a/internal/cmd/init-sg/main.go b/internal/cmd/init-sg/main.go index 52260361622..81c5b238fac 100644 --- a/internal/cmd/init-sg/main.go +++ b/internal/cmd/init-sg/main.go @@ -75,15 +75,17 @@ func initSourcegraph() { log.Fatal("Failed to check if site needs init: ", err) } + client, err = gqltestutil.NewClient(*baseURL) + if err != nil { + log.Fatal("Failed to create gql client: ", err) + } if needsSiteInit { - client, err = gqltestutil.SiteAdminInit(*baseURL, *email, *username, *password) - if err != nil { + if err := client.SiteAdminInit(*email, *username, *password); err != nil { log.Fatal("Failed to create site admin: ", err) } log.Println("Site admin has been created:", *username) } else { - client, err = gqltestutil.SignIn(*baseURL, *email, *password) - if err != nil { + if err := client.SignIn(*email, *password); err != nil { log.Fatal("Failed to sign in:", err) } log.Println("Site admin authenticated:", *username) @@ -140,8 +142,11 @@ func addReposCommand() { log.Fatal("Environment variable GITHUB_TOKEN is not set") } - client, err := gqltestutil.SignIn(*baseURL, *email, *password) + client, err := gqltestutil.NewClient(*baseURL) if err != nil { + log.Fatal("Failed to create gql client: ", err) + } + if err := client.SignIn(*email, *password); err != nil { log.Fatal("Failed to sign in:", err) } log.Println("Site admin authenticated:", *username) @@ -214,8 +219,11 @@ func oobmigrationCommand() { id := *migrationID up := !*migrationDownFlag - client, err := gqltestutil.SignIn(*baseURL, *email, *password) + client, err := gqltestutil.NewClient(*baseURL) if err != nil { + log.Fatal("Failed to create gql client: ", err) + } + if err := client.SignIn(*email, *password); err != nil { log.Fatal("Failed to sign in:", err) } log.Println("Site admin authenticated:", *username) diff --git a/internal/gqltestutil/client.go b/internal/gqltestutil/client.go index cdfc4fb84ae..05586caffa6 100644 --- a/internal/gqltestutil/client.go +++ b/internal/gqltestutil/client.go @@ -30,8 +30,8 @@ func NeedsSiteInit(baseURL string) (bool, string, error) { // SiteAdminInit initializes the instance with given admin account. // It returns an authenticated client as the admin for doing testing. -func SiteAdminInit(baseURL, email, username, password string) (*Client, error) { - return authenticate(baseURL, "/-/site-init", map[string]string{ +func (c *Client) SiteAdminInit(email, username, password string) error { + return c.authenticate("/-/site-init", map[string]string{ "email": email, "username": username, "password": password, @@ -40,46 +40,30 @@ func SiteAdminInit(baseURL, email, username, password string) (*Client, error) { // SignUp signs up a new user with given credentials. // It returns an authenticated client as the user for doing testing. -func SignUp(baseURL, email, username, password string) (*Client, error) { - return authenticate(baseURL, "/-/sign-up", map[string]string{ +func (c *Client) SignUp(email, username, password string) error { + return c.authenticate("/-/sign-up", map[string]string{ "email": email, "username": username, "password": password, }) } -func SignUpOrSignIn(baseURL, email, username, password string) (*Client, error) { - client, err := SignUp(baseURL, email, username, password) - if err != nil { - return SignIn(baseURL, email, password) +func (c *Client) SignUpOrSignIn(email, username, password string) error { + if err := c.SignUp(email, username, password); err != nil { + return c.SignIn(email, password) } - return client, err + return nil } // SignIn performs the sign in with given user credentials. // It returns an authenticated client as the user for doing testing. -func SignIn(baseURL, email, password string) (*Client, error) { - return authenticate(baseURL, "/-/sign-in", map[string]string{ +func (c *Client) SignIn(email, password string) error { + return c.authenticate("/-/sign-in", map[string]string{ "email": email, "password": password, }) } -// authenticate initializes an authenticated client with given request body. -func authenticate(baseURL, path string, body any) (*Client, error) { - client, err := NewClient(baseURL, nil, nil) - if err != nil { - return nil, errors.Wrap(err, "new client") - } - - err = client.authenticate(path, body) - if err != nil { - return nil, errors.Wrap(err, "authenticate") - } - - return client, nil -} - // extractCSRFToken extracts CSRF token from HTML response body. func extractCSRFToken(body string) string { anchor := `X-Csrf-Token":"` @@ -114,16 +98,25 @@ type LogFunc func(payload []byte) func noopLog(payload []byte) {} +type ClientOption struct { + GraphQLRequestLogger LogFunc + GraphQLResponseLogger LogFunc +} + // NewClient instantiates a new client by performing a GET request then obtains the // CSRF token and cookie from its response, if there is one (old versions of Sourcegraph only). -// If request- or responseLogger are provided, the request and response bodies, respectively, +// If loggers are provided via options, the request and response bodies, respectively, // will be written to them for any GraphQL requests only. -func NewClient(baseURL string, requestLogger, responseLogger LogFunc) (*Client, error) { - if requestLogger == nil { - requestLogger = noopLog - } - if responseLogger == nil { - responseLogger = noopLog +func NewClient(baseURL string, options ...ClientOption) (*Client, error) { + requestLogger := noopLog + responseLogger := noopLog + for _, opt := range options { + if opt.GraphQLRequestLogger != nil { + requestLogger = opt.GraphQLRequestLogger + } + if opt.GraphQLResponseLogger != nil { + responseLogger = opt.GraphQLResponseLogger + } } resp, err := http.Get(baseURL)