chore: refactor after code review

.github/workflows/integration-test.yml → .github/workflows/integration-tests.yml

@@ -1,4 +1,4 @@
-name: Integration Test
+name: Integration Tests
 
 on:
   push:
@@ -9,21 +9,15 @@ on:
 jobs:
   test:
     runs-on: ubuntu-latest
-
-    strategy:
-      fail-fast: false
-      matrix:
-        go: ['1.22']
-
-    name: Go ${{ matrix.go }}
+    name: tests
 
     steps:
     - uses: actions/checkout@v4
 
     - name: Set up Go
       uses: actions/setup-go@v5
       with:
-        go-version: ${{ matrix.go }}
+        go-version: 1.22
 
-    - name: Integration test
+    - name: Run tests
       run: go test -count=1 -tags=integration ./tests/

HACKING.md

@@ -202,6 +202,13 @@ ok      github.com/canonical/pebble/cmd/pebble  0.165s
 ...
 ```
 
+Pebble also has a suite of integration tests for testing things like `pebble run`. To run them, use the "integration" build constraint:
+
+```
+$ go test -count=1 -tags=integration ./tests/
+ok  	github.com/canonical/pebble/tests	4.774s
+```
+
 ## Docs
 
 We use [`sphinx`](https://www.sphinx-doc.org/en/master/) to build the docs with styles preconfigured by the [Canonical Documentation Starter Pack](https://github.com/canonical/sphinx-docs-starter-pack).

tests/README.md

@@ -1,5 +1,7 @@
 # Pebble Integration Tests
 
+This directory holds a suite of integration tests for end-to-end tests of things like pebble run. They use the standard go test runner, but are only executed if you set the integration build constraint.
+
 ## Run Tests
 
 ```bash

tests/main_test.go

@@ -56,11 +56,11 @@ func createLayer(t *testing.T, pebbleDir, layerFileName, layerYAML string) {
 	}
 }
 
-func pebbleRun(t *testing.T, pebbleDir string, args ...string) (<-chan servicelog.Entry, <-chan servicelog.Entry) {
+func pebbleRun(t *testing.T, pebbleDir string, args ...string) (stdoutCh chan servicelog.Entry, stderrCh chan servicelog.Entry) {
 	t.Helper()
 
-	stdoutCh := make(chan servicelog.Entry)
-	stderrCh := make(chan servicelog.Entry)
+	stdoutCh = make(chan servicelog.Entry)
+	stderrCh = make(chan servicelog.Entry)
 
 	cmd := exec.Command("../pebble", append([]string{"run"}, args...)...)
 	cmd.Env = append(os.Environ(), "PEBBLE="+pebbleDir)
@@ -79,49 +79,44 @@ func pebbleRun(t *testing.T, pebbleDir string, args ...string) (<-chan servicelo
 		t.Fatalf("Error starting 'pebble run': %v", err)
 	}
 
+	stopStdout := make(chan struct{}, 1)
+	stopStderr := make(chan struct{}, 1)
+
 	t.Cleanup(func() {
 		err := cmd.Process.Signal(os.Interrupt)
 		if err != nil {
 			t.Errorf("Error sending SIGINT/Ctrl+C to pebble: %v", err)
 		}
 		cmd.Wait()
+		stopStdout <- struct{}{}
+		stopStderr <- struct{}{}
 	})
 
-	done := make(chan struct{})
-	go func() {
-		defer close(stdoutCh)
-		defer close(stderrCh)
-
-		readLogs := func(parser *servicelog.Parser, ch chan servicelog.Entry) {
-			for parser.Next() {
-				if err := parser.Err(); err != nil {
-					t.Errorf("Cannot parse Pebble logs: %v", err)
-				}
-				select {
-				case ch <- parser.Entry():
-				case <-done:
-					return
-				}
+	readLogs := func(parser *servicelog.Parser, ch chan servicelog.Entry, stop <-chan struct{}) {
+		for parser.Next() {
+			if err := parser.Err(); err != nil {
+				t.Errorf("Cannot parse Pebble logs: %v", err)
+			}
+			select {
+			case ch <- parser.Entry():
+			case <-stop:
+				return
 			}
 		}
+	}
 
-		// Both stderr and stdout are needed, because pebble logs to stderr
-		// while with "--verbose", services otuput to stdout.
-		stderrParser := servicelog.NewParser(stderrPipe, 4*1024)
-		stdoutParser := servicelog.NewParser(stdoutPipe, 4*1024)
-
-		go readLogs(stdoutParser, stdoutCh)
-		go readLogs(stderrParser, stderrCh)
+	// Both stderr and stdout are needed, because pebble logs to stderr
+	// while with "--verbose", services output to stdout.
+	stderrParser := servicelog.NewParser(stderrPipe, 4*1024)
+	stdoutParser := servicelog.NewParser(stdoutPipe, 4*1024)
 
-		// Wait for both parsers to finish
-		<-done
-		<-done
-	}()
+	go readLogs(stdoutParser, stdoutCh, stopStdout)
+	go readLogs(stderrParser, stderrCh, stopStderr)
 
 	return stdoutCh, stderrCh
 }
 
-func waitForLog(t *testing.T, logsCh <-chan servicelog.Entry, expectedLog string, timeout time.Duration) {
+func waitForLog(t *testing.T, logsCh <-chan servicelog.Entry, expectedService, expectedLog string, timeout time.Duration) {
 	t.Helper()
 
 	timeoutCh := time.After(timeout)
@@ -132,7 +127,7 @@ func waitForLog(t *testing.T, logsCh <-chan servicelog.Entry, expectedLog string
 				t.Error("channel closed before all expected logs were received")
 			}
 
-			if strings.Contains(log.Message, expectedLog) {
+			if log.Service == expectedService && strings.Contains(log.Message, expectedLog) {
 				return
 			}
 
@@ -155,7 +150,6 @@ func waitForFile(t *testing.T, file string, timeout time.Duration) {
 		case <-ticker.C:
 			stat, err := os.Stat(file)
 			if err == nil && stat.Mode().IsRegular() {
-				os.Remove(file)
 				return
 			}
 		}

tests/run_test.go

@@ -20,7 +20,6 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
-	"net"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -59,7 +58,7 @@ func TestCreateDirs(t *testing.T) {
 	pebbleDir := filepath.Join(tmpDir, "pebble")
 
 	_, stderrCh := pebbleRun(t, pebbleDir, "--create-dirs")
-	waitForLog(t, stderrCh, "Started daemon", 3*time.Second)
+	waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
 
 	st, err := os.Stat(pebbleDir)
 	if err != nil {
@@ -104,16 +103,7 @@ func TestHTTPPort(t *testing.T) {
 
 	port := "61382"
 	_, stderrCh := pebbleRun(t, pebbleDir, "--http=:"+port)
-	waitForLog(t, stderrCh, "Started daemon", 3*time.Second)
-
-	conn, err := net.Listen("tcp", ":"+port)
-	if err == nil {
-		conn.Close()
-		t.Fatalf("port %s is not being listened: %v", port, err)
-	}
-	if conn != nil {
-		conn.Close()
-	}
+	waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
 
 	resp, err := http.Get(fmt.Sprintf("http://localhost:%s/v1/health", port))
 	if err != nil {
@@ -139,8 +129,8 @@ services:
 	createLayer(t, pebbleDir, layersFileName, layerYAML)
 
 	stdoutCh, stderrCh := pebbleRun(t, pebbleDir, "--verbose")
-	waitForLog(t, stderrCh, "Started daemon", 3*time.Second)
-	waitForLog(t, stdoutCh, "hello world", 3*time.Second)
+	waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
+	waitForLog(t, stdoutCh, "svc1", "hello world", 3*time.Second)
 }
 
 func TestArgs(t *testing.T) {
@@ -162,8 +152,8 @@ services:
 		"-c",
 		"echo 'hello world'; sleep 1000",
 	)
-	waitForLog(t, stderrCh, "Started daemon", 3*time.Second)
-	waitForLog(t, stdoutCh, "hello world", 3*time.Second)
+	waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
+	waitForLog(t, stdoutCh, "svc1", "hello world", 3*time.Second)
 }
 
 func TestIdentities(t *testing.T) {
@@ -175,34 +165,20 @@ identities:
         access: admin
         local:
             user-id: 42
-    alice:
-        access: read
-        local:
-            user-id: 2000
-`
+`[1:]
 	identitiesFileName := "idents-add.yaml"
 	if err := os.WriteFile(filepath.Join(pebbleDir, identitiesFileName), []byte(identitiesYAML), 0o755); err != nil {
 		t.Fatalf("Cannot write identities file: %v", err)
 	}
 
-	_, _ = pebbleRun(t, pebbleDir, "--identities="+filepath.Join(pebbleDir, identitiesFileName))
-	time.Sleep(100 * time.Millisecond)
+	_, stderrCh := pebbleRun(t, pebbleDir, "--identities="+filepath.Join(pebbleDir, identitiesFileName))
+	waitForLog(t, stderrCh, "pebble", "Started daemon", 3*time.Second)
 
 	output := runPebbleCommand(t, pebbleDir, "identity", "bob")
 	expected := `
 access: admin
 local:
     user-id: 42
-`[1:]
-	if output != expected {
-		t.Fatalf("error checking identities. expected: %s; got: %s", expected, output)
-	}
-
-	output = runPebbleCommand(t, pebbleDir, "identity", "alice")
-	expected = `
-access: read
-local:
-    user-id: 2000
 `[1:]
 	if output != expected {
 		t.Fatalf("error checking identities. expected: %s; got: %s", expected, output)