perf: wait for the dbus-daemon to be ready using its output (#152)

dbus-daemon can print out its listen address when it's ready, by waiting
so instead of just guessing how much to wait.

Doing this through some goroutines although being this test code we
could very likely just hang.

PS: Since these two functions _were already_ pretty much the same, can
we just maybe move them in a common place? If so, which one should I
pick?

internal/brokers/broker_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/ubuntu/authd/internal/brokers"
 	"github.com/ubuntu/authd/internal/brokers/responses"
 	"github.com/ubuntu/authd/internal/testutils"
+	brokertestutils "github.com/ubuntu/authd/internal/testutils/broker"
 )
 
 var supportedLayouts = map[string]map[string]string{
@@ -325,7 +326,7 @@ func newBrokerForTests(t *testing.T, cfgDir, brokerName string) (b brokers.Broke
 		brokerName = strings.ReplaceAll(t.Name(), "/", "_")
 	}
 
-	cfgPath, cleanup, err := testutils.StartBusBrokerMock(cfgDir, brokerName)
+	cfgPath, cleanup, err := brokertestutils.StartBusBrokerMock(cfgDir, brokerName)
 	require.NoError(t, err, "Setup: could not start bus broker mock")
 	t.Cleanup(cleanup)
 
@@ -342,5 +343,5 @@ func newBrokerForTests(t *testing.T, cfgDir, brokerName string) (b brokers.Broke
 // prefixID is a helper function that prefixes the given ID with the test name to avoid conflicts.
 func prefixID(t *testing.T, id string) string {
 	t.Helper()
-	return t.Name() + testutils.IDSeparator + id
+	return t.Name() + brokertestutils.IDSeparator + id
 }

internal/brokers/manager_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/ubuntu/authd/internal/brokers"
 	"github.com/ubuntu/authd/internal/testutils"
+	brokertestutils "github.com/ubuntu/authd/internal/testutils/broker"
 )
 
 var (
@@ -328,10 +329,14 @@ func TestStartAndEndSession(t *testing.T) {
 	require.NoError(t, *firstErr, "First NewSession should not return an error, but did")
 	require.NoError(t, *secondErr, "Second NewSession should not return an error, but did")
 
-	require.Equal(t, b1.ID+"-"+testutils.GenerateSessionID("user1"), *firstID, "First NewSession should return the expected session ID, but did not")
-	require.Equal(t, testutils.GenerateEncryptionKey(b1.Name), *firstKey, "First NewSession should return the expected encryption key, but did not")
-	require.Equal(t, b2.ID+"-"+testutils.GenerateSessionID("user2"), *secondID, "Second NewSession should return the expected session ID, but did not")
-	require.Equal(t, testutils.GenerateEncryptionKey(b2.Name), *secondKey, "Second NewSession should return the expected encryption key, but did not")
+	require.Equal(t, b1.ID+"-"+brokertestutils.GenerateSessionID("user1"),
+		*firstID, "First NewSession should return the expected session ID, but did not")
+	require.Equal(t, brokertestutils.GenerateEncryptionKey(b1.Name),
+		*firstKey, "First NewSession should return the expected encryption key, but did not")
+	require.Equal(t, b2.ID+"-"+brokertestutils.GenerateSessionID("user2"),
+		*secondID, "Second NewSession should return the expected session ID, but did not")
+	require.Equal(t, brokertestutils.GenerateEncryptionKey(b2.Name),
+		*secondKey, "Second NewSession should return the expected encryption key, but did not")
 
 	assignedBroker, err := m.BrokerFromSessionID(*firstID)
 	require.NoError(t, err, "First NewSession should have assigned a broker for the session, but did not")

internal/brokers/withexamples.go

@@ -4,20 +4,17 @@ package brokers
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"os"
-	"os/exec"
-	"path/filepath"
 	"time"
 
 	"github.com/ubuntu/authd/internal/brokers/examplebroker"
 	"github.com/ubuntu/authd/internal/log"
+	"github.com/ubuntu/authd/internal/testutils"
 )
 
 // useExampleBrokers starts a mock system bus and exports the examplebroker in it.
 func useExampleBrokers() (string, func(), error) {
-	busCleanup, err := startSystemBusMock()
+	busCleanup, err := testutils.StartSystemBusMock()
 	if err != nil {
 		return "", nil, err
 	}
@@ -50,65 +47,7 @@ func useExampleBrokers() (string, func(), error) {
 	}, nil
 }
 
-// startSystemBusMock starts a mock dbus daemon and returns a cancel function to stop it.
-func startSystemBusMock() (func(), error) {
-	busDir := filepath.Join(os.TempDir(), "authd-system-bus-mock")
-	if err := os.MkdirAll(busDir, 0750); err != nil {
-		return nil, err
-	}
-
-	cfgPath := filepath.Join(busDir, "bus.conf")
-	listenPath := filepath.Join(busDir, "bus.sock")
-
-	err := os.WriteFile(cfgPath, []byte(fmt.Sprintf(localSystemBusCfg, listenPath)), 0600)
-	if err != nil {
-		err = errors.Join(err, os.RemoveAll(busDir))
-		return nil, err
-	}
-
-	busCtx, busCancel := context.WithCancel(context.Background())
-	//#nosec:G204 // This is only for manual testing purposes and won't be in production code.
-	cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath)
-	if err := cmd.Start(); err != nil {
-		busCancel()
-		err = errors.Join(err, os.RemoveAll(busDir))
-		return nil, err
-	}
-	// Give some time for the daemon to start.
-	time.Sleep(500 * time.Millisecond)
-
-	prev, set := os.LookupEnv("DBUS_SYSTEM_BUS_ADDRESS")
-	os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path="+listenPath)
-
-	return func() {
-		busCancel()
-		_ = cmd.Wait()
-		_ = os.RemoveAll(busDir)
-
-		if !set {
-			os.Unsetenv("DBUS_SYSTEM_BUS_ADDRESS")
-		} else {
-			os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", prev)
-		}
-	}, nil
-}
-
 // Stop calls the function responsible for cleaning up the examplebrokers.
 func (m *Manager) Stop() {
 	m.cleanup()
 }
-
-var localSystemBusCfg = `<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-Bus Bus Configuration 1.0//EN"
- "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
-<busconfig>
-  <type>system</type>
-  <keep_umask/>
-  <listen>unix:path=%s</listen>
-  <policy context="default">
-    <allow user="*"/>
-    <allow send_destination="*" eavesdrop="true"/>
-    <allow eavesdrop="true"/>
-    <allow own="*"/>
-  </policy>
-</busconfig>
-`

internal/services/pam/pam_test.go

@@ -19,6 +19,7 @@ import (
 	cachetests "github.com/ubuntu/authd/internal/cache/tests"
 	"github.com/ubuntu/authd/internal/services/pam"
 	"github.com/ubuntu/authd/internal/testutils"
+	brokertestutils "github.com/ubuntu/authd/internal/testutils/broker"
 	usertests "github.com/ubuntu/authd/internal/users/tests"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
@@ -154,7 +155,7 @@ func TestSelectBroker(t *testing.T) {
 			}
 
 			if tc.username != "" {
-				tc.username = t.Name() + testutils.IDSeparator + tc.username
+				tc.username = t.Name() + brokertestutils.IDSeparator + tc.username
 			}
 
 			sbRequest := &authd.SBRequest{
@@ -613,7 +614,7 @@ func initBrokers() (brokerConfigPath string, cleanup func(), err error) {
 		_ = os.RemoveAll(tmpDir)
 		return "", nil, err
 	}
-	_, brokerCleanup, err := testutils.StartBusBrokerMock(brokersConfPath, "BrokerMock")
+	_, brokerCleanup, err := brokertestutils.StartBusBrokerMock(brokersConfPath, "BrokerMock")
 	if err != nil {
 		_ = os.RemoveAll(tmpDir)
 		return "", nil, err
@@ -682,7 +683,7 @@ func startSession(t *testing.T, client authd.PAMClient, username string) string
 	t.Helper()
 
 	// Prefixes the username to avoid concurrency issues.
-	username = t.Name() + testutils.IDSeparator + username
+	username = t.Name() + brokertestutils.IDSeparator + username
 
 	sbResp, err := client.SelectBroker(context.Background(), &authd.SBRequest{
 		BrokerId: mockBrokerGeneratedID,

internal/testutils/broker.go → internal/testutils/broker/broker.go

@@ -1,4 +1,5 @@
-package testutils
+// Package brokertestutils provides utility functions and behaviors for testing brokers.
+package brokertestutils
 
 import (
 	"bytes"

internal/testutils/dbus.go

@@ -2,12 +2,14 @@
 package testutils
 
 import (
+	"bufio"
 	"context"
 	"errors"
 	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -56,17 +58,47 @@ func StartSystemBusMock() (func(), error) {
 
 	busCtx, busCancel := context.WithCancel(context.Background())
 	//#nosec:G204 // This is a test helper and we are in control of the arguments.
-	cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath)
+	cmd := exec.CommandContext(busCtx, "dbus-daemon", "--config-file="+cfgPath, "--print-address=1")
+	dbusStdout, err := cmd.StdoutPipe()
+	if err != nil {
+		busCancel()
+		return nil, errors.Join(err, os.RemoveAll(tmp))
+	}
 	if err := cmd.Start(); err != nil {
 		busCancel()
 		err = errors.Join(err, os.RemoveAll(tmp))
 		return nil, err
 	}
-	// Give some time for the daemon to start.
-	time.Sleep(500 * time.Millisecond)
 
+	waitDone := make(chan struct{})
+	var busAddress string
+
+	go func() {
+		scanner := bufio.NewScanner(dbusStdout)
+		for scanner.Scan() {
+			busAddress = scanner.Text()
+			close(waitDone)
+			break
+		}
+	}()
+
+	select {
+	case <-time.After(10 * time.Second):
+		busCancel()
+		err = errors.New("dbus-daemon failed to start in 10 seconds")
+		return nil, errors.Join(err, os.RemoveAll(tmp))
+	case <-waitDone:
+	}
+
+	if !strings.HasPrefix(busAddress, "unix:path=") {
+		busCancel()
+		err = fmt.Errorf("invalid bus path: %s", busAddress)
+		return nil, errors.Join(err, os.RemoveAll(tmp))
+	}
+
+	busAddress, _, _ = strings.Cut(busAddress, ",")
 	prev, set := os.LookupEnv("DBUS_SYSTEM_BUS_ADDRESS")
-	os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path="+listenPath)
+	os.Setenv("DBUS_SYSTEM_BUS_ADDRESS", busAddress)
 
 	return func() {
 		busCancel()