Addressing comments

ubuntu · Aug 10, 2023 · 6a13eeb · 6a13eeb
1 parent 0ef1df9
commit 6a13eeb
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 41 deletions.
diff --git a/internal/brokers/broker_test.go b/internal/brokers/broker_test.go
@@ -41,7 +41,7 @@ func TestNewBroker(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			conn, err := testutils.GetSystemBusConnection()
+			conn, err := testutils.GetSystemBusConnection(t)
 			require.NoError(t, err, "Setup: could not connect to system bus")
 
 			if tc.configFile != "" {
@@ -106,7 +106,7 @@ func TestGetAuthenticationModes(t *testing.T) {
 
 		wantErr bool
 	}{
-		"Successfully get authentication modes":                        {},
+		"Successfully get authentication modes":                        {sessionID: "success"},
 		"Does not error out when no authentication modes are returned": {sessionID: "GAM_empty"},
 
 		// broker errors
@@ -120,10 +120,6 @@ func TestGetAuthenticationModes(t *testing.T) {
 
 			b := newBrokerForTests(t)
 
-			if tc.sessionID == "" {
-				tc.sessionID = "success"
-			}
-
 			gotModes, err := b.GetAuthenticationModes(context.Background(), tc.sessionID, nil)
 			if tc.wantErr {
 				require.Error(t, err, "GetAuthenticationModes should return an error, but did not")
@@ -198,15 +194,15 @@ func TestIsAuthorized(t *testing.T) {
 		wantAccessSecondCall string
 		wantErrSecondCall    bool
 	}{
-		"Successfully authorize":                             {},
+		"Successfully authorize":                             {sessionID: "success", wantAccess: responses.AuthAllowed},
 		"Successfully authorize after cancelling first call": {sessionID: "IA_second_call", wantAccess: responses.AuthCancelled, wantAccessSecondCall: responses.AuthAllowed},
 		"Denies authentication when broker times out":        {sessionID: "IA_timeout", wantAccess: responses.AuthDenied},
 
 		// broker errors
 		"Error when authorizing":                                           {sessionID: "IA_error", wantErr: true},
 		"Error when broker returns invalid access":                         {sessionID: "IA_invalid", wantErr: true},
 		"Error when broker returns invalid user info":                      {sessionID: "IA_invalid_info", wantErr: true},
-		"Error when calling IsAuthorized a second time without cancelling": {sessionID: "IA_second_call", sessionIDSecondCall: "IA_second_call", wantErrSecondCall: true},
+		"Error when calling IsAuthorized a second time without cancelling": {sessionID: "IA_second_call", wantAccess: responses.AuthAllowed, sessionIDSecondCall: "IA_second_call", wantErrSecondCall: true},
 	}
 	for name, tc := range tests {
 		tc := tc
@@ -215,13 +211,6 @@ func TestIsAuthorized(t *testing.T) {
 
 			b := newBrokerForTests(t)
 
-			if tc.sessionID == "" {
-				tc.sessionID = "success"
-			}
-			if tc.wantAccess == "" {
-				tc.wantAccess = responses.AuthAllowed
-			}
-
 			var access string
 			var err error
 			ctx, cancel := context.WithCancel(context.Background())
@@ -284,7 +273,7 @@ func TestCancelIsAuthorized(t *testing.T) {
 				access, _, _ = b.IsAuthorized(ctx, tc.sessionID, "password")
 				close(done)
 			}()
-			t.Cleanup(cancel)
+			defer cancel()
 
 			if tc.sessionID == "IA_wait" {
 				// Give some time for the IsAuthorized routine to start.
@@ -305,9 +294,8 @@ func TestEndSession(t *testing.T) {
 
 		wantErr bool
 	}{
-		"Successfully end session": {},
+		"Successfully end session": {sessionID: "success"},
 
-		// broker errors
 		"Error when ending session": {sessionID: "ES_error", wantErr: true},
 	}
 	for name, tc := range tests {
@@ -317,10 +305,6 @@ func TestEndSession(t *testing.T) {
 
 			b := newBrokerForTests(t)
 
-			if tc.sessionID == "" {
-				tc.sessionID = "success"
-			}
-
 			err := b.EndSession(context.Background(), tc.sessionID)
 			if tc.wantErr {
 				require.Error(t, err, "EndSession should return an error, but did not")
@@ -336,7 +320,7 @@ func newBrokerForTests(t *testing.T) brokers.Broker {
 
 	cfgPath := testutils.StartBusBrokerMock(t)
 
-	conn, err := testutils.GetSystemBusConnection()
+	conn, err := testutils.GetSystemBusConnection(t)
 	require.NoError(t, err, "Setup: could not connect to system bus")
 	t.Cleanup(func() { require.NoError(t, conn.Close(), "Teardown: Failed to close the connection") })
 

diff --git a/internal/brokers/manager_test.go b/internal/brokers/manager_test.go
@@ -25,7 +25,8 @@ func TestNewManager(t *testing.T) {
 		rootDir  string
 		noBus    bool
 		readOnly bool
-		wantErr  bool
+
+		wantErr bool
 	}{
 		"Successfully create manager identifying brokers":   {},
 		"Creates only local broker when dir does not exist": {rootDir: "does/not/exist"},
@@ -114,7 +115,8 @@ func TestGetBroker(t *testing.T) {
 		wantErr    bool
 	}{
 		"Successfully returns expected broker": {brokerID: "local", wantBroker: "local"},
-		"Error when broker does not exist":     {brokerID: "does not exist", wantErr: true},
+
+		"Error when broker does not exist": {brokerID: "does not exist", wantErr: true},
 	}
 	for name, tc := range tests {
 		tc := tc
@@ -162,7 +164,7 @@ func TestManagerEndSession(t *testing.T) {
 
 		wantErr bool
 	}{
-		"Successfully end existing session": {},
+		"Successfully end existing session": {sessionID: "success"},
 
 		"Error when broker does not exist":      {sessionID: "does not exist", wantErr: true},
 		"Error when broker can not end session": {sessionID: "ES_error", wantErr: true},
@@ -177,9 +179,6 @@ func TestManagerEndSession(t *testing.T) {
 			m, err := brokers.NewManager(context.Background(), nil, brokers.WithRootDir(rootDir))
 			require.NoError(t, err, "Setup: could not create manager")
 
-			if tc.sessionID == "" {
-				tc.sessionID = "success"
-			}
 			if tc.sessionID != "does not exist" {
 				m.SetBrokerForSessionID(tc.sessionID, &b)
 			}

diff --git a/internal/testutils/broker.go b/internal/testutils/broker.go
@@ -2,9 +2,6 @@ package testutils
 
 import (
 	"context"
-	// embed is used to embed the configuration template for the mock broker.
-	_ "embed"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -107,7 +104,7 @@ func writeConfig(t *testing.T, name string) string {
 // NewSession returns default values to be used in tests or an error if requested.
 func (b *BrokerBusMock) NewSession(username, lang string) (sessionID, encryptionKey string, dbusErr *dbus.Error) {
 	if username == "NS_error" {
-		return "", "", dbus.MakeFailedError(errors.New("NewSession errored out"))
+		return "", "", dbus.MakeFailedError(fmt.Errorf("Broker %q: NewSession errored out", b.name))
 	}
 	if username == "NS_no_id" {
 		return "", username + "_key", nil
@@ -127,7 +124,7 @@ func (b *BrokerBusMock) GetAuthenticationModes(sessionID string, supportedUILayo
 		return nil, nil
 
 	case "GAM_error":
-		return nil, dbus.MakeFailedError(errors.New("GetAuthenticationModes errored out"))
+		return nil, dbus.MakeFailedError(fmt.Errorf("Broker %q: GetAuthenticationModes errored out", b.name))
 	}
 
 	return []map[string]string{
@@ -147,7 +144,7 @@ func (b *BrokerBusMock) SelectAuthenticationMode(sessionID, authenticationModeNa
 		return nil, nil
 
 	case "SAM_error":
-		return nil, dbus.MakeFailedError(errors.New("SelectAuthenticationMode errored out"))
+		return nil, dbus.MakeFailedError(fmt.Errorf("Broker %q: SelectAuthenticationMode errored out", b.name))
 
 	case "SAM_form_no_label":
 		return map[string]string{
@@ -206,12 +203,12 @@ func (b *BrokerBusMock) SelectAuthenticationMode(sessionID, authenticationModeNa
 // IsAuthorized returns default values to be used in tests or an error if requested.
 func (b *BrokerBusMock) IsAuthorized(sessionID, authenticationData string) (access, data string, dbusErr *dbus.Error) {
 	if sessionID == "IA_error" {
-		return "", "", dbus.MakeFailedError(errors.New("IsAuthorized errored out"))
+		return "", "", dbus.MakeFailedError(fmt.Errorf("Broker %q: IsAuthorized errored out", b.name))
 	}
 
 	// Handles the context that will be assigned for the IsAuthorized handler
 	if _, exists := b.isAuthorizedCalls[sessionID]; exists {
-		return "", "", dbus.MakeFailedError(fmt.Errorf("IsAuthorized already running for session %q", sessionID))
+		return "", "", dbus.MakeFailedError(fmt.Errorf("Broker %q: IsAuthorized already running for session %q", b.name, sessionID))
 	}
 	ctx, cancel := context.WithCancel(context.Background())
 	b.isAuthorizedCallsMu.Lock()
@@ -263,7 +260,7 @@ func (b *BrokerBusMock) IsAuthorized(sessionID, authenticationData string) (acce
 // EndSession returns default values to be used in tests or an error if requested.
 func (b *BrokerBusMock) EndSession(sessionID string) (dbusErr *dbus.Error) {
 	if sessionID == "ES_error" {
-		return dbus.MakeFailedError(errors.New("erroring out as requested"))
+		return dbus.MakeFailedError(fmt.Errorf("Broker %q: EndSession errored out", b.name))
 	}
 	return nil
 }

diff --git a/internal/testutils/dbus.go b/internal/testutils/dbus.go
@@ -8,6 +8,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"testing"
 	"time"
 
 	"github.com/godbus/dbus/v5"
@@ -64,14 +65,15 @@ func StartSystemBusMock() (func(), error) {
 	// Give some time for the daemon to start.
 	time.Sleep(5 * time.Millisecond)
 
-	prev := os.Getenv("DBUS_SYSTEM_BUS_ADDRESS")
+	prev, set := os.LookupEnv("DBUS_SYSTEM_BUS_ADDRESS")
 	os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path="+listenPath)
 
 	return func() {
 		busCancel()
 		_ = cmd.Wait()
 		_ = os.RemoveAll(tmp)
-		if prev == "" {
+
+		if !set {
 			os.Unsetenv("DBUS_SYSTEM_BUS_ADDRESS")
 		} else {
 			os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", prev)
@@ -81,7 +83,8 @@ func StartSystemBusMock() (func(), error) {
 
 // GetSystemBusConnection returns a connection to the system bus with a safety check to avoid mistakenly connecting to the
 // actual system bus.
-func GetSystemBusConnection() (*dbus.Conn, error) {
+func GetSystemBusConnection(t *testing.T) (*dbus.Conn, error) {
+	t.Helper()
 	if !isRunning() {
 		return nil, errors.New("system bus mock is not running. If that's intended, manually connect to the system bus instead of using this function")
 	}