PR feedback

Author: stuartpa

cmd/modern/root/open/ads.go

@@ -10,6 +10,8 @@ import (
 	"github.com/microsoft/go-sqlcmd/internal/config"
 	"github.com/microsoft/go-sqlcmd/internal/container"
 	"github.com/microsoft/go-sqlcmd/internal/tools"
+	"runtime"
+	"strings"
 )
 
 // Ads implements the `sqlcmd open ads` command. It opens
@@ -70,19 +72,27 @@ func (c *Ads) ensureContainerIsRunning(endpoint sqlconfig.Endpoint) {
 }
 
 // launchAds launches the Azure Data Studio using the specified server and username.
-func (c *Ads) launchAds(localhost string, port int, username string) {
+func (c *Ads) launchAds(host string, port int, username string) {
 	output := c.Output()
 	args := []string{
 		"-r",
 		fmt.Sprintf(
 			"--server=%s", fmt.Sprintf(
 				"%s,%d",
-				localhost,
+				host,
 				port)),
 	}
 
 	if username != "" {
-		args = append(args, fmt.Sprintf("--user=%s", username))
+
+		// Here's a fun SQL Server behavior  - it allows you to create database
+		// and login names that include the " character. SSMS escapes those
+		// with \" when invoking ADS on the command line, we do the same here
+		args = append(args, fmt.Sprintf("--user=%s", strings.Replace(username, `"`, `\"`, -1)))
+	} else {
+		if runtime.GOOS == "windows" {
+			args = append(args, "--integrated")
+		}
 	}
 
 	tool := tools.NewTool("ads")

cmd/modern/root/open/ads_darwin.go

@@ -5,6 +5,9 @@ import (
 	"github.com/microsoft/go-sqlcmd/internal/cmdparser"
 )
 
+// Type Ads is used to implement the "open ads" which launches Azure
+// Data Studio and establishes a connection to the SQL Server for the current
+// context
 type Ads struct {
 	cmdparser.Cmd
 }

cmd/modern/root/open/ads_linux.go

@@ -5,6 +5,9 @@ import (
 	"github.com/microsoft/go-sqlcmd/internal/cmdparser"
 )
 
+// Type Ads is used to implement the "open ads" which launches Azure
+// Data Studio and establishes a connection to the SQL Server for the current
+// context
 type Ads struct {
 	cmdparser.Cmd
 }

cmd/modern/root/open/ads_windows.go

@@ -8,6 +8,9 @@ import (
 	"github.com/microsoft/go-sqlcmd/internal/secret"
 )
 
+// Type Ads is used to implement the "open ads" which launches Azure
+// Data Studio and establishes a connection to the SQL Server for the current
+// context
 type Ads struct {
 	cmdparser.Cmd
 

cmd/modern/root/open/ads_windows_test.go

@@ -32,14 +32,8 @@ func TestPersistCredentialForAds(t *testing.T) {
 
 	// Test if the correct target name is generated
 	expectedTargetName := "Microsoft.SqlTools|itemtype:Profile|id:providerName:MSSQL|applicationName:azdata|authenticationType:SqlLogin|database:|server:localhost,1433|user:testuser"
-	if ads.credential.TargetName != expectedTargetName {
-		t.Errorf("Expected target name to be %s, got %s", expectedTargetName, ads.credential.TargetName)
-	}
-
-	// Test if the correct username is set
-	if ads.credential.UserName != user.BasicAuth.Username {
-		t.Errorf("Expected username to be %s, got %s", user.BasicAuth.Username, ads.credential.UserName)
-	}
+	assert.Equal(t, ads.credential.TargetName, expectedTargetName, "Expected target name to be %s, got %s", expectedTargetName, ads.credential.TargetName)
+	assert.Equal(t, ads.credential.UserName, user.BasicAuth.Username, "Expected username to be %s, got %s", user.BasicAuth.Username, ads.credential.UserName)
 
 	// Test if the password is decoded correctly
 	decodedPassword := secret.DecodeAsUtf16(user.BasicAuth.Password, user.BasicAuth.PasswordEncrypted)

cmd/modern/root/start.go

@@ -11,8 +11,6 @@ import (
 
 type Start struct {
 	cmdparser.Cmd
-
-	errorLogEntryToWaitFor string
 }
 
 func (c *Start) DefineCommand(...cmdparser.CommandOptions) {
@@ -28,12 +26,6 @@ func (c *Start) DefineCommand(...cmdparser.CommandOptions) {
 	}
 
 	c.Cmd.DefineCommand(options)
-	c.AddFlag(cmdparser.FlagOptions{
-		String:        &c.errorLogEntryToWaitFor,
-		DefaultString: "Recovery is complete",
-		Name:          "errorlog-wait-line",
-		Usage:         "Line in errorlog to wait for before exiting this process",
-	})
 }
 
 func (c *Start) run() {
@@ -56,17 +48,6 @@ func (c *Start) run() {
 		)
 		err := controller.ContainerStart(id)
 		c.CheckErr(err)
-
-		// BUG(stuartpa): Even if we block here for the final entry in the errorlog
-		// "Recovery Complete", a sqlcmd query will still fail for a few seconds
-		// with:
-		//  sqlcmd query "SELECT @@version"
-		//  EOF
-		//  Error: EOF
-		// I'm not sure what else to block here for before returning to ensure
-		// a `sqlcmd query` will work
-		controller.ContainerWaitForLogEntry(
-			id, c.errorLogEntryToWaitFor)
 	} else {
 		output.FatalfWithHintExamples([][]string{
 			{"Create new context with a sql container ", "sqlcmd create mssql"},

internal/config/config_test.go

@@ -176,9 +176,8 @@ func TestFindUniqueUserName(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if gotUniqueUserName := FindUniqueUserName(tt.args.name); gotUniqueUserName != tt.wantUniqueUserName {
-				t.Errorf("FindUniqueUserName() = %v, want %v", gotUniqueUserName, tt.wantUniqueUserName)
-			}
+			gotUniqueUserName := FindUniqueUserName(tt.args.name)
+			assert.Equal(t, gotUniqueUserName, tt.wantUniqueUserName, "FindUniqueUserName() = %v, want %v", gotUniqueUserName, tt.wantUniqueUserName)
 		})
 	}
 }
@@ -194,9 +193,8 @@ func TestGetUser(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if gotUser := GetUser(tt.args.name); !reflect.DeepEqual(gotUser, tt.wantUser) {
-				t.Errorf("GetUser() = %v, want %v", gotUser, tt.wantUser)
-			}
+			gotUser := GetUser(tt.args.name)
+			assert.Truef(t, reflect.DeepEqual(gotUser, tt.wantUser), "GetUser() = %v, want %v", gotUser, tt.wantUser)
 		})
 	}
 }
@@ -228,9 +226,8 @@ func TestUserExists(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if gotExists := UserNameExists(tt.args.name); gotExists != tt.wantExists {
-				t.Errorf("UserNameExists() = %v, want %v", gotExists, tt.wantExists)
-			}
+			gotExists := UserNameExists(tt.args.name)
+			assert.Equal(t, gotExists, tt.wantExists, "UserNameExists() = %v, want %v", gotExists, tt.wantExists)
 		})
 	}
 }
@@ -246,9 +243,8 @@ func TestUserNameExists(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if gotExists := UserNameExists(tt.args.name); gotExists != tt.wantExists {
-				t.Errorf("UserNameExists() = %v, want %v", gotExists, tt.wantExists)
-			}
+			gotExists := UserNameExists(tt.args.name)
+			assert.Equal(t, gotExists, tt.wantExists, "UserNameExists() = %v, want %v", gotExists, tt.wantExists)
 		})
 	}
 }
@@ -264,9 +260,8 @@ func Test_userOrdinal(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if gotOrdinal := userOrdinal(tt.args.name); gotOrdinal != tt.wantOrdinal {
-				t.Errorf("userOrdinal() = %v, want %v", gotOrdinal, tt.wantOrdinal)
-			}
+			gotOrdinal := userOrdinal(tt.args.name)
+			assert.Equal(t, gotOrdinal, tt.wantOrdinal, "userOrdinal() = %v, want %v", gotOrdinal, tt.wantOrdinal)
 		})
 	}
 }

internal/config/endpoint-container_test.go

@@ -5,6 +5,7 @@ package config
 
 import (
 	. "github.com/microsoft/go-sqlcmd/cmd/modern/sqlconfig"
+	"github.com/stretchr/testify/assert"
 	"strings"
 	"testing"
 )
@@ -14,33 +15,21 @@ import (
 func TestCurrentContextEndpointHasContainer(t *testing.T) {
 	Clean()
 
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf("The code did not panic")
-		}
-	}()
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }()
 	CurrentContextEndpointHasContainer()
 }
 
 func TestGetContainerId(t *testing.T) {
 	Clean()
 
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf("The code did not panic")
-		}
-	}()
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }()
 	ContainerId()
 }
 
 func TestGetContainerId2(t *testing.T) {
 	Clean()
 
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf("The code did not panic")
-		}
-	}()
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }()
 
 	AddEndpoint(Endpoint{
 		AssetDetails: &AssetDetails{},
@@ -66,11 +55,7 @@ func TestGetContainerId2(t *testing.T) {
 
 func TestGetContainerId3(t *testing.T) {
 	Clean()
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf("The code did not panic")
-		}
-	}()
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }()
 
 	AddEndpoint(Endpoint{
 		AssetDetails: &AssetDetails{

internal/config/endpoint_test.go

@@ -3,13 +3,12 @@
 
 package config
 
-import "testing"
+import (
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
 
 func TestEndpointExists(t *testing.T) {
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf("The code did not panic")
-		}
-	}()
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }()
 	EndpointExists("")
 }

internal/config/error_test.go

@@ -5,14 +5,11 @@ package config
 
 import (
 	"errors"
+	"github.com/stretchr/testify/assert"
 	"testing"
 )
 
 func Test_checkErr(t *testing.T) {
-	defer func() {
-		if r := recover(); r == nil {
-			t.Errorf("The code did not panic")
-		}
-	}()
+	defer func() { assert.NotNil(t, recover(), "The code did not panic as expected") }()
 	checkErr(errors.New("verify error handler"))
 }