Skip to content

Commit

Permalink
refactor(FIR-33560): Remove redundant and obsolete tests (#106)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ptiurin
ptiurin authored Jun 12, 2024
1 parent 39fc87d commit ad53156
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 455 deletions.
5 changes: 2 additions & 3 deletions .github/workflows/integration-tests-v2.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,14 @@ jobs:
firebolt-client-id: ${{ secrets.FIREBOLT_CLIENT_ID_STG_NEW_IDN }}
firebolt-client-secret: ${{ secrets.FIREBOLT_CLIENT_SECRET_STG_NEW_IDN}}
api-endpoint: "api.staging.firebolt.io"
account: ${{ vars.FIREBOLT_ACCOUNT_V1 }}
account: ${{ vars.FIREBOLT_ACCOUNT }}

- name: Run integration tests
env:
DATABASE_NAME: ${{ steps.setup.outputs.database_name }}
ENGINE_NAME: ${{ steps.setup.outputs.engine_name }}
FIREBOLT_ENDPOINT: "api.staging.firebolt.io"
ACCOUNT_NAME_V1: ${{ vars.FIREBOLT_ACCOUNT_V1 }}
ACCOUNT_NAME_V2: ${{ vars.FIREBOLT_ACCOUNT_V2 }}
ACCOUNT_NAME: ${{ vars.FIREBOLT_ACCOUNT }}
CLIENT_ID: ${{ secrets.FIREBOLT_CLIENT_ID_STG_NEW_IDN }}
CLIENT_SECRET: ${{ secrets.FIREBOLT_CLIENT_SECRET_STG_NEW_IDN }}
run: |
Expand Down
119 changes: 10 additions & 109 deletions client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"encoding/json"
"errors"
"fmt"
"strings"

"github.com/astaxie/beego/cache"
)
Expand All @@ -21,11 +20,6 @@ type ClientImpl struct {
BaseClient
}

const engineStatusRunning = "Running"
const engineInfoSQL = `
SELECT url, status, attached_to FROM information_schema.engines
WHERE engine_name='%s'
`
const accountError = `account '%s' does not exist in this organization or is not authorized.
Please verify the account name and make sure your service account has the
correct RBAC permissions and is linked to a user`
Expand Down Expand Up @@ -69,44 +63,6 @@ func MakeClient(settings *fireboltSettings, apiEndpoint string) (*ClientImpl, er
}
return client, nil
}
func (c *ClientImpl) getEngineUrlStatusDBByName(ctx context.Context, engineName string, systemEngineUrl string) (string, string, string, error) {
infolog.Printf("Get info for engine '%s'", engineName)
engineSQL := fmt.Sprintf(engineInfoSQL, engineName)
queryRes, err := c.Query(ctx, systemEngineUrl, engineSQL, make(map[string]string), connectionControl{})
if err != nil {
return "", "", "", ConstructNestedError("error executing engine info sql query", err)
}

if len(queryRes.Data) == 0 {
return "", "", "", fmt.Errorf("engine with name %s doesn't exist", engineName)
}

engineUrl, status, dbName, err := parseEngineInfoResponse(queryRes.Data)
if err != nil {
return "", "", "", ConstructNestedError("error parsing server response for engine info SQL query", err)
}
return engineUrl, status, dbName, nil
}

func parseEngineInfoResponse(resp [][]interface{}) (string, string, string, error) {
if len(resp) != 1 || len(resp[0]) != 3 {
return "", "", "", fmt.Errorf("invalid response shape: %v", resp)
}
engineUrl, ok := resp[0][0].(string)
if !ok {
return "", "", "", fmt.Errorf("expected string for engine URL, got %v", resp[0][0])
}
status, ok := resp[0][1].(string)
if !ok {
return "", "", "", fmt.Errorf("expected string for engine status, got %v", resp[0][1])
}
dbName, ok := resp[0][2].(string)
// NULL is also acceptable for database name
if !ok && resp[0][2] != nil {
return "", "", "", fmt.Errorf("expected string for engine status, got %v", resp[0][1])
}
return engineUrl, status, dbName, nil
}

func constructParameters(databaseName string, queryParams map[string][]string) map[string]string {
parameters := make(map[string]string)
Expand Down Expand Up @@ -217,31 +173,23 @@ func (c *ClientImpl) getQueryParams(setStatements map[string]string) (map[string
for setKey, setValue := range setStatements {
params[setKey] = setValue
}
// Account id is only used when querying system engine for infra v1
if c.ConnectedToSystemEngine && c.AccountVersion == 1 {
if len(c.AccountID) == 0 {
return nil, fmt.Errorf("Trying to run a query against system engine without account id defined")
}
if _, ok := params["account_id"]; !ok {
params["account_id"] = c.AccountID
}
}
return params, nil
}

func (c *ClientImpl) getAccessToken() (string, error) {
return getAccessTokenServiceAccount(c.ClientID, c.ClientSecret, c.ApiEndpoint, c.UserAgent)
}

func (c *ClientImpl) getConnectionParametersV2(
ctx context.Context,
engineName,
databaseName,
systemEngineURL string,
systemEngineParameters map[string]string,
) (string, map[string]string, error) {
engineURL := systemEngineURL
parameters := systemEngineParameters
// GetConnectionParameters returns engine URL and parameters based on engineName and databaseName
func (c *ClientImpl) GetConnectionParameters(ctx context.Context, engineName, databaseName string) (string, map[string]string, error) {
// Assume we are connected to a system engine in the beginning

engineURL, parameters, err := c.getSystemEngineURLAndParameters(context.Background(), c.AccountName, databaseName)
if err != nil {
return "", nil, ConstructNestedError("error during getting system engine url", err)
}
c.ConnectedToSystemEngine = true

control := connectionControl{
updateParameters: func(key, value string) {
parameters[key] = value
Expand All @@ -265,50 +213,3 @@ func (c *ClientImpl) getConnectionParametersV2(
}
return engineURL, parameters, nil
}

func (c *ClientImpl) getConnectionParametersV1(
ctx context.Context,
engineName,
databaseName,
systemEngineURL string,
) (string, map[string]string, error) {
// If engine name is empty, assume system engine
if len(engineName) == 0 {
return systemEngineURL, map[string]string{"database": databaseName}, nil
}

engineUrl, status, dbName, err := c.getEngineUrlStatusDBByName(ctx, engineName, systemEngineURL)
params := map[string]string{"database": dbName}
if err != nil {
return "", params, ConstructNestedError("error during getting engine info", err)
}
// Case-insensitive comparison
if !strings.EqualFold(status, engineStatusRunning) {
return "", params, fmt.Errorf("engine %s is not running", engineName)
}
if len(dbName) == 0 {
return "", params, fmt.Errorf("engine %s not attached to any DB or you don't have permission to access its database", engineName)
}
if len(databaseName) != 0 && databaseName != dbName {
return "", params, fmt.Errorf("engine %s is not attached to database %s", engineName, databaseName)
}
c.ConnectedToSystemEngine = false

return engineUrl, params, nil
}

// GetConnectionParameters returns engine URL and parameters based on engineName and databaseName
func (c *ClientImpl) GetConnectionParameters(ctx context.Context, engineName, databaseName string) (string, map[string]string, error) {
// Assume we are connected to a system engine in the beginning

systemEngineURL, systemEngineParameters, err := c.getSystemEngineURLAndParameters(context.Background(), c.AccountName, databaseName)
if err != nil {
return "", nil, ConstructNestedError("error during getting system engine url", err)
}

c.ConnectedToSystemEngine = true
if c.AccountVersion == 2 {
return c.getConnectionParametersV2(ctx, engineName, databaseName, systemEngineURL, systemEngineParameters)
}
return c.getConnectionParametersV1(ctx, engineName, databaseName, systemEngineURL)
}
16 changes: 2 additions & 14 deletions client_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,18 @@ package fireboltgosdk

import (
"context"
"strings"
"testing"
"time"
)

// TestGetEnginePropsByName test getting system engine url, as well as engine url, status and database by name
func TestGetEnginePropsByName(t *testing.T) {
systemEngineURL, _, err := clientMockWithAccount.getSystemEngineURLAndParameters(context.TODO(), accountNameV1Mock, "")
systemEngineURL, _, err := clientMockWithAccount.getSystemEngineURLAndParameters(context.TODO(), accountName, "")
if err != nil {
t.Errorf("Error returned by getSystemEngineURL: %s", err)
}
if len(systemEngineURL) == 0 {
t.Errorf("Empty system engine url returned by getSystemEngineURL for account: %s", accountNameV1Mock)
}

engineURL, status, _, err := clientMockWithAccount.getEngineUrlStatusDBByName(context.TODO(), engineNameMock, systemEngineURL)
if err != nil {
t.Errorf("Error returned by getEngineUrlStatusDBByName: %s", err)
}
if engineURL == "" {
t.Errorf("Empty engine url returned by getEngineUrlStatusDBByName")
}
if !strings.EqualFold(status, "Running") {
t.Errorf("Invalid status returned by getEngineUrlStatusDBByName. Got: %s, should be Running", status)
t.Errorf("Empty system engine url returned by getSystemEngineURL for account: %s", accountName)
}
}

Expand Down
63 changes: 4 additions & 59 deletions connection_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,42 +10,6 @@ import (
"testing"
)

func setupEngineAndDatabase(t *testing.T) {
conn, err := sql.Open("firebolt", dsnSystemEngineV2Mock)
if err != nil {
t.Errorf("opening a connection failed unexpectedly: %v", err)
t.FailNow()
}
if _, err = conn.Exec(fmt.Sprintf("CREATE DATABASE IF NOT EXISTS \"%s\"", databaseMock)); err != nil {
t.Errorf("creating a database failed unexpectedly: %v", err)
t.FailNow()
}
if _, err = conn.Exec(fmt.Sprintf("CREATE ENGINE IF NOT EXISTS \"%s\"", engineNameMock)); err != nil {
t.Errorf("creating an engine failed unexpectedly: %v", err)
t.FailNow()
}
}

func cleanupEngineAndDatabase(t *testing.T) {
conn, err := sql.Open("firebolt", dsnSystemEngineV2Mock)
if err != nil {
t.Errorf("opening a connection failed unexpectedly: %v", err)
t.FailNow()
}
if _, err = conn.Exec(fmt.Sprintf("STOP ENGINE \"%s\"", engineNameMock)); err != nil {
t.Errorf("stopping an engine failed unexpectedly: %v", err)
t.FailNow()
}
if _, err = conn.Exec(fmt.Sprintf("DROP ENGINE IF EXISTS \"%s\"", engineNameMock)); err != nil {
t.Errorf("dropping an engine failed unexpectedly: %v", err)
t.FailNow()
}
if _, err = conn.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS \"%s\"", databaseMock)); err != nil {
t.Errorf("dropping a database failed unexpectedly: %v", err)
t.FailNow()
}
}

func TestConnectionUseDatabase(t *testing.T) {
tableName := "test_use_database"
createTableSQL := "CREATE TABLE IF NOT EXISTS " + tableName + " (id INT)"
Expand Down Expand Up @@ -107,32 +71,13 @@ func TestConnectionUseDatabase(t *testing.T) {
}
}

func TestConnectionV2(t *testing.T) {
setupEngineAndDatabase(t)
defer cleanupEngineAndDatabase(t)

conn, err := sql.Open("firebolt", dsnV2Mock)
if err != nil {
t.Errorf("opening a connection failed unexpectedly")
t.FailNow()
}

_, err = conn.Exec("SELECT 1")
if err != nil {
t.Errorf("query failed with %v", err)
t.FailNow()
}
}

func TestConnectionV2UseDatabaseEngine(t *testing.T) {
setupEngineAndDatabase(t)
defer cleanupEngineAndDatabase(t)
func TestConnectionUseDatabaseEngine(t *testing.T) {

const createTableSQL = "CREATE TABLE IF NOT EXISTS test_use (id INT)"
const insertSQL = "INSERT INTO test_use VALUES (1)"
const insertSQL2 = "INSERT INTO test_use VALUES (2)"

conn, err := sql.Open("firebolt", dsnSystemEngineV2Mock)
conn, err := sql.Open("firebolt", dsnSystemEngineMock)
if err != nil {
t.Errorf("opening a connection failed unexpectedly")
t.FailNow()
Expand Down Expand Up @@ -188,7 +133,7 @@ func TestConnectionV2UseDatabaseEngine(t *testing.T) {
}

func TestConnectionUppercaseNames(t *testing.T) {
systemConnection, err := sql.Open("firebolt", dsnSystemEngineV2Mock)
systemConnection, err := sql.Open("firebolt", dsnSystemEngineMock)
if err != nil {
t.Errorf("opening a system connection failed unexpectedly %v", err)
t.FailNow()
Expand All @@ -212,7 +157,7 @@ func TestConnectionUppercaseNames(t *testing.T) {

dsnUppercase := fmt.Sprintf(
"firebolt:///%s?account_name=%s&engine=%s&client_id=%s&client_secret=%s",
databaseName, accountNameV2Mock, engineName, clientIdMock, clientSecretMock,
databaseName, accountName, engineName, clientIdMock, clientSecretMock,
)

conn, err := sql.Open("firebolt", dsnUppercase)
Expand Down
Loading

0 comments on commit ad53156

Please sign in to comment.