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.

Sign up for GitHub

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

feat: optimize sqlserver test #652

Merged
merged 8 commits into from
Sep 21, 2024
Merged

feat: optimize sqlserver test #652

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
db46df2
feat: cancel sqlserver test
hwbrzzl Sep 20, 2024
ce94162
test
hwbrzzl Sep 20, 2024
183f762
test
hwbrzzl Sep 20, 2024
c660a9e
test
hwbrzzl Sep 20, 2024
08c3da6
test
hwbrzzl Sep 20, 2024
fd804e7
test
hwbrzzl Sep 20, 2024
8d7df98
cancel test
hwbrzzl Sep 20, 2024
ff1e323
optimize password
hwbrzzl Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion support/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
type ContainerType string

const (
password = "Goravel123"
password = "Framework!123"
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Update approved, but action required for test files

The password change in support/docker/docker.go has been successfully implemented. However, there are a few important points to address:

  1. The old password "Goravel123" is still present in testing/docker/database_test.go. Please update these test cases to use either the new password or a different test password to avoid confusion and potential test failures.

  2. The password constant is used in various parts of the application, particularly in database-related code. Ensure that all components using this constant are still functioning correctly after the change.

  3. If there's any documentation referencing the old password, make sure to update it accordingly.

Action items:

  • Update the password in testing/docker/database_test.go
  • Verify the functionality of all components using the password constant
  • Update any relevant documentation
Analysis chain

Verify the password change across the codebase.

The password change looks good. However, please ensure that this new password is updated in all relevant places, such as:

  • Configuration files
  • Documentation
  • Any other code references

Also, communicate this change to the team to avoid any disruptions.

Run the following script to verify the usage of the password constant:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the `password` constant across the codebase.

# Test: Search for the `password` constant usage. Expect: Only occurrences with the new value.
rg --type go -A 5 $'password = "Framework!123"'

Length of output: 370

Script:

#!/bin/bash
# Description: Further verify the password change across the codebase.

# Test 1: Search for any remaining instances of the old password.
echo "Searching for old password:"
rg --type go -A 5 "Goravel123"

# Test 2: Look for uses of the 'password' constant.
echo "Searching for uses of 'password' constant:"
ast-grep --lang go --pattern 'password'

Length of output: 5396

username = "goravel"
database = "goravel"

Expand Down
8 changes: 4 additions & 4 deletions support/docker/postgres_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestPostgresTestSuite(t *testing.T) {

func (s *PostgresTestSuite) SetupTest() {
s.mockConfig = &configmocks.Config{}
s.postgres = NewPostgresImpl("goravel", "goravel", "goravel")
s.postgres = NewPostgresImpl(database, username, password)
}

func (s *PostgresTestSuite) TestBuild() {
Expand All @@ -37,9 +37,9 @@ func (s *PostgresTestSuite) TestBuild() {
s.NotNil(instance)

s.Equal("127.0.0.1", s.postgres.Config().Host)
s.Equal("goravel", s.postgres.Config().Database)
s.Equal("goravel", s.postgres.Config().Username)
s.Equal("goravel", s.postgres.Config().Password)
s.Equal(database, s.postgres.Config().Database)
s.Equal(username, s.postgres.Config().Username)
s.Equal(password, s.postgres.Config().Password)
s.True(s.postgres.Config().Port > 0)

res := instance.Exec(`
Expand Down
2 changes: 1 addition & 1 deletion support/docker/sqlite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (s *SqliteTestSuite) TestBuild() {
s.Nil(err)
s.NotNil(instance)

s.Equal("goravel", s.sqlite.Config().Database)
s.Equal(database, s.sqlite.Config().Database)

res := instance.Exec(`
CREATE TABLE users (
Expand Down
47 changes: 39 additions & 8 deletions support/docker/sqlserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,11 @@
username: username,
password: password,
image: &testing.Image{
Repository: "mcmoe/mssqldocker",
Repository: "mcr.microsoft.com/mssql/server",
Tag: "latest",
Env: []string{
"ACCEPT_EULA=Y",
"MSSQL_DB=" + database,
"MSSQL_USER=" + username,
"MSSQL_PASSWORD=" + password,
"SA_PASSWORD=" + password,
"MSSQL_SA_PASSWORD=" + password,
},
ExposedPorts: []string{"1433"},
},
Expand Down Expand Up @@ -122,13 +119,47 @@
)

// docker compose need time to start
for i := 0; i < 60; i++ {
for i := 0; i < 100; i++ {
instance, err = gormio.Open(sqlserver.New(sqlserver.Config{
DSN: fmt.Sprintf("sqlserver://%s:%s@%s:%d?database=%s",
receiver.username, receiver.password, receiver.host, receiver.port, receiver.database),
DSN: fmt.Sprintf("sqlserver://%s:%s@%s:%d?database=master",
"sa", receiver.password, receiver.host, receiver.port),
}))

if err == nil {
// Check if database exists
var exists bool
query := fmt.Sprintf("SELECT CASE WHEN EXISTS (SELECT * FROM sys.databases WHERE name = '%s') THEN CAST(1 AS BIT) ELSE CAST(0 AS BIT) END", receiver.database)
if err := instance.Raw(query).Scan(&exists).Error; err != nil {
return nil, err

Check warning on line 133 in support/docker/sqlserver.go

View check run for this annotation

Codecov / codecov/patch

support/docker/sqlserver.go#L133 

Added line #L133 was not covered by tests
}

if !exists {
// Create User database
if err := instance.Exec(fmt.Sprintf("CREATE DATABASE %s", receiver.database)).Error; err != nil {
return nil, err

Check warning on line 139 in support/docker/sqlserver.go

View check run for this annotation

Codecov / codecov/patch

support/docker/sqlserver.go#L139 

Added line #L139 was not covered by tests
}

// Create User account
if err := instance.Exec(fmt.Sprintf("CREATE LOGIN %s WITH PASSWORD = '%s'", receiver.username, receiver.password)).Error; err != nil {
return nil, err

Check warning on line 144 in support/docker/sqlserver.go

View check run for this annotation

Codecov / codecov/patch

support/docker/sqlserver.go#L144 

Added line #L144 was not covered by tests
}

// Create DB account for User
if err := instance.Exec(fmt.Sprintf("USE %s; CREATE USER %s FOR LOGIN %s", receiver.database, receiver.username, receiver.username)).Error; err != nil {
return nil, err

Check warning on line 149 in support/docker/sqlserver.go

View check run for this annotation

Codecov / codecov/patch

support/docker/sqlserver.go#L149 

Added line #L149 was not covered by tests
}

// Add permission
if err := instance.Exec(fmt.Sprintf("USE %s; ALTER ROLE db_owner ADD MEMBER %s", receiver.database, receiver.username)).Error; err != nil {
return nil, err

Check warning on line 154 in support/docker/sqlserver.go

View check run for this annotation

Codecov / codecov/patch

support/docker/sqlserver.go#L154 

Added line #L154 was not covered by tests
}
}
Comment on lines +132 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

Optimize database and user creation queries.

Currently, the code executes multiple SQL statements separately to set up the database and user. This can be optimized by combining statements and using transactions to ensure atomicity.

You can refactor the SQL commands into a single script:

script := fmt.Sprintf(`
IF NOT EXISTS (SELECT * FROM sys.databases WHERE name = N'%s')
BEGIN
    CREATE DATABASE [%s];
END;

USE [%s];

IF NOT EXISTS (SELECT * FROM sys.sql_logins WHERE name = N'%s')
BEGIN
    CREATE LOGIN [%s] WITH PASSWORD = N'%s';
END;

IF NOT EXISTS (SELECT * FROM sys.database_principals WHERE name = N'%s')
BEGIN
    CREATE USER [%s] FOR LOGIN [%s];
    ALTER ROLE db_owner ADD MEMBER [%s];
END;
`, receiver.database, receiver.database, receiver.database, receiver.username, receiver.username, receiver.password, receiver.username, receiver.username, receiver.username, receiver.username)

if err := instance.Exec(script).Error; err != nil {
    return nil, fmt.Errorf("failed to set up database and user: %v", err)
}

This approach reduces the number of round trips to the database and ensures that the setup is only performed if necessary.


instance, err = gormio.Open(sqlserver.New(sqlserver.Config{
DSN: fmt.Sprintf("sqlserver://%s:%s@%s:%d?database=%s",
receiver.username, receiver.password, receiver.host, receiver.port, receiver.database),
}))

Comment on lines +122 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance error handling in the connection logic.

The connection logic has been extended to include database and user creation. However, if any of these steps fail, the error handling could be improved to provide more detailed information and ensure resources are cleaned up appropriately.

Consider adding more descriptive error messages and handling partial failures. For example:

// After each operation, check for errors and provide context
if err := instance.Exec(fmt.Sprintf("CREATE DATABASE %s", receiver.database)).Error; err != nil {
    return nil, fmt.Errorf("failed to create database '%s': %v", receiver.database, err)
}

// Similarly for other operations

break
}

Expand Down
6 changes: 3 additions & 3 deletions support/docker/sqlserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ func (s *SqlserverTestSuite) TestBuild() {
s.NotNil(instance)

s.Equal("127.0.0.1", s.sqlserver.Config().Host)
s.Equal("goravel", s.sqlserver.Config().Database)
s.Equal("goravel", s.sqlserver.Config().Username)
s.Equal("Goravel123", s.sqlserver.Config().Password)
s.Equal(database, s.sqlserver.Config().Database)
s.Equal(username, s.sqlserver.Config().Username)
s.Equal(password, s.sqlserver.Config().Password)
s.True(s.sqlserver.Config().Port > 0)

res := instance.Exec(`
Expand Down
13 changes: 12 additions & 1 deletion support/env/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package env

import "runtime"
import (
"os"
"runtime"
)

// IsWindows returns whether the current operating system is Windows.
// IsWindows 返回当前操作系统是否为 Windows。
Expand Down Expand Up @@ -37,3 +40,11 @@
func Is64Bit() bool {
return runtime.GOARCH == "amd64" || runtime.GOARCH == "arm64"
}

// IsGithub returns whether the current environment is github action.
// IsGithub 返回当前系统环境是否为 github action。
func IsGithub() bool {
_, exists := os.LookupEnv("GITHUB_ACTION")

Check warning on line 47 in support/env/env.go

View check run for this annotation

Codecov / codecov/patch

support/env/env.go#L46-L47 

Added lines #L46 - L47 were not covered by tests

return exists

Check warning on line 49 in support/env/env.go

View check run for this annotation

Codecov / codecov/patch

support/env/env.go#L49 

Added line #L49 was not covered by tests
}
Loading