feat: update integration tests (#434)

* feat: add more unittests

* fix(tests): use the soft binary to run integration tests

* fix(ci): upload coverage data

* fix: daemon test idle timeout

* fix: daemon flaky test

* chore: add more webhook unit tests

* fix(test): enable webhook integration tests

* fix(tests): readd sync lock

* fix(ci): collect coverage for both unit and integration tests

* fix(ci): coverage test

* fix(ci): remove macos and windows

* fix: return the opened logger file

* fix: daemon idle test

* fix: testscript on windows

* fix: run soft-serve in txtar background

* fix(ci): collecting coverage data

* fix: coverage data

* fix: remove unused

* fix: add browse test

* feat: add stop server endpoint

* fix(tests): run integration tests on windows

* fix(tests): skip daemon idle timeout flaky test

* fix(tests): attempt to fix daemon idle test

.github/workflows/build.yml

@@ -42,5 +42,5 @@ jobs:
       - name: Test
         run: go test ./...
         env:
-          DB_DRIVER: postgres
-          DB_DATA_SOURCE: postgres://postgres:postgres@localhost/postgres?sslmode=disable
+          SOFT_SERVE_DB_DRIVER: postgres
+          SOFT_SERVE_DB_DATA_SOURCE: postgres://postgres:postgres@localhost/postgres?sslmode=disable

.github/workflows/coverage.yml

@@ -8,7 +8,10 @@ on:
 
 jobs:
   coverage:
-    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        os: [ubuntu-latest] # TODO: add macos & windows
+    runs-on: ${{ matrix.os }}
     steps:
       - uses: actions/checkout@v4
 
@@ -18,7 +21,24 @@ jobs:
           go-version: ^1
 
       - name: Test
-        run: go test -failfast -race -coverpkg=./... -covermode=atomic -coverprofile=coverage.txt ./... -timeout 5m
+        run: |
+          # We collect coverage data from two sources,
+          # 1) unit tests 2) integration tests
+          #
+          # https://go.dev/testing/coverage/
+          # https://dustinspecker.com/posts/go-combined-unit-integration-code-coverage/
+          # https://github.com/golang/go/issues/51430#issuecomment-1344711300
+          mkdir -p coverage/unit
+          mkdir -p coverage/int
+
+          # Collect unit tests coverage
+          go test -failfast -race -timeout 5m -skip=^TestScript -cover ./... -args -test.gocoverdir=$PWD/coverage/unit
+
+          # Collect integration tests coverage
+          GOCOVERDIR=$PWD/coverage/int go test -failfast -race -timeout 5m -run=^TestScript ./...
+
+          # Convert coverage data to legacy textfmt format to upload
+          go tool covdata textfmt -i=coverage/unit,coverage/int -o=coverage.txt
       - uses: codecov/codecov-action@v3
         with:
           file: ./coverage.txt

cmd/cmd.go

@@ -33,7 +33,7 @@ func InitBackendContext(cmd *cobra.Command, _ []string) error {
 	ctx = db.WithContext(ctx, dbx)
 	dbstore := database.New(ctx, dbx)
 	ctx = store.WithContext(ctx, dbstore)
-	be := backend.New(ctx, cfg, dbx)
+	be := backend.New(ctx, cfg, dbx, dbstore)
 	ctx = backend.WithContext(ctx, be)
 
 	cmd.SetContext(ctx)

cmd/soft/serve/serve.go

@@ -3,9 +3,12 @@ package serve
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"os"
 	"os/signal"
 	"path/filepath"
+	"strconv"
+	"sync"
 	"syscall"
 	"time"
 
@@ -80,10 +83,26 @@ var (
 			}
 
 			done := make(chan os.Signal, 1)
+			doneOnce := sync.OnceFunc(func() { close(done) })
+
 			lch := make(chan error, 1)
+
+			// This endpoint is added for testing purposes
+			// It allows us to stop the server from the test suite.
+			// This is needed since Windows doesn't support signals.
+			if testRun, _ := strconv.ParseBool(os.Getenv("SOFT_SERVE_TESTRUN")); testRun {
+				h := s.HTTPServer.Server.Handler
+				s.HTTPServer.Server.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					if r.URL.Path == "/__stop" && r.Method == http.MethodHead {
+						doneOnce()
+						return
+					}
+					h.ServeHTTP(w, r)
+				})
+			}
+
 			go func() {
-				defer close(lch)
-				defer close(done)
+				defer doneOnce()
 				lch <- s.Start()
 			}()
 

cmd/soft/serve/server.go

@@ -147,7 +147,7 @@ func (s *Server) Shutdown(ctx context.Context) error {
 		for _, j := range jobs.List() {
 			s.Cron.Remove(j.ID)
 		}
-		s.Cron.Shutdown()
+		s.Cron.Stop()
 		return nil
 	})
 	// defer s.DB.Close() // nolint: errcheck

go.mod

@@ -41,8 +41,7 @@ require (
 	github.com/muesli/roff v0.1.0
 	github.com/prometheus/client_golang v1.17.0
 	github.com/robfig/cron/v3 v3.0.1
-	github.com/rogpeppe/go-internal v1.11.0
-	github.com/rubyist/tracerx v0.0.0-20170927163412-787959303086
+	github.com/rogpeppe/go-internal v1.11.1-0.20231026093722-fa6a31e0812c
 	github.com/spf13/cobra v1.8.0
 	go.uber.org/automaxprocs v1.5.3
 	golang.org/x/crypto v0.16.0

go.sum

@@ -167,10 +167,8 @@ github.com/rivo/uniseg v0.4.3 h1:utMvzDsuh3suAEnhH0RdHmoPbU648o6CvXxTx4SBMOw=
 github.com/rivo/uniseg v0.4.3/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88=
 github.com/robfig/cron/v3 v3.0.1 h1:WdRxkvbJztn8LMz/QEvLN5sBU+xKpSqwwUO1Pjr4qDs=
 github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro=
-github.com/rogpeppe/go-internal v1.11.0 h1:cWPaGQEPrBb5/AsnsZesgZZ9yb1OQ+GOISoDNXVBh4M=
-github.com/rogpeppe/go-internal v1.11.0/go.mod h1:ddIwULY96R17DhadqLgMfk9H9tvdUzkipdSkR5nkCZA=
-github.com/rubyist/tracerx v0.0.0-20170927163412-787959303086 h1:mncRSDOqYCng7jOD+Y6+IivdRI6Kzv2BLWYkWkdQfu0=
-github.com/rubyist/tracerx v0.0.0-20170927163412-787959303086/go.mod h1:YpdgDXpumPB/+EGmGTYHeiW/0QVFRzBYTNFaxWfPDk4=
+github.com/rogpeppe/go-internal v1.11.1-0.20231026093722-fa6a31e0812c h1:fPpdjePK1atuOg28PXfNSqgwf9I/qD1Hlo39JFwKBXk=
+github.com/rogpeppe/go-internal v1.11.1-0.20231026093722-fa6a31e0812c/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/sahilm/fuzzy v0.1.0 h1:FzWGaw2Opqyu+794ZQ9SYifWv2EIXpwP4q8dY1kDAwI=
 github.com/sahilm/fuzzy v0.1.0/go.mod h1:VFvziUEIMCrT6A6tw2RFIXPXXmzXbOsSHF0DOI8ZK9Y=

pkg/access/context_test.go

@@ -0,0 +1,20 @@
+package access
+
+import (
+	"context"
+	"testing"
+)
+
+func TestGoodFromContext(t *testing.T) {
+	ctx := WithContext(context.TODO(), AdminAccess)
+	if ac := FromContext(ctx); ac != AdminAccess {
+		t.Errorf("FromContext(ctx) => %d, want %d", ac, AdminAccess)
+	}
+}
+
+func TestBadFromContext(t *testing.T) {
+	ctx := context.TODO()
+	if ac := FromContext(ctx); ac != -1 {
+		t.Errorf("FromContext(ctx) => %d, want %d", ac, -1)
+	}
+}

pkg/backend/backend.go

@@ -23,14 +23,13 @@ type Backend struct {
 }
 
 // New returns a new Soft Serve backend.
-func New(ctx context.Context, cfg *config.Config, db *db.DB) *Backend {
-	dbstore := store.FromContext(ctx)
+func New(ctx context.Context, cfg *config.Config, db *db.DB, st store.Store) *Backend {
 	logger := log.FromContext(ctx).WithPrefix("backend")
 	b := &Backend{
 		ctx:     ctx,
 		cfg:     cfg,
 		db:      db,
-		store:   dbstore,
+		store:   st,
 		logger:  logger,
 		manager: task.NewManager(ctx),
 	}

pkg/config/context_test.go

@@ -0,0 +1,29 @@
+package config
+
+import (
+	"context"
+	"reflect"
+	"testing"
+)
+
+func TestBadFromContext(t *testing.T) {
+	ctx := context.TODO()
+	if c := FromContext(ctx); c != nil {
+		t.Errorf("FromContext(ctx) => %v, want %v", c, nil)
+	}
+}
+
+func TestGoodFromContext(t *testing.T) {
+	ctx := WithContext(context.TODO(), &Config{})
+	if c := FromContext(ctx); c == nil {
+		t.Errorf("FromContext(ctx) => %v, want %v", c, &Config{})
+	}
+}
+
+func TestGoodFromContextWithDefaultConfig(t *testing.T) {
+	cfg := DefaultConfig()
+	ctx := WithContext(context.TODO(), cfg)
+	if c := FromContext(ctx); c == nil || !reflect.DeepEqual(c, cfg) {
+		t.Errorf("FromContext(ctx) => %v, want %v", c, cfg)
+	}
+}

pkg/config/file_test.go

@@ -0,0 +1,15 @@
+package config
+
+import "testing"
+
+func TestNewConfigFile(t *testing.T) {
+	for _, cfg := range []*Config{
+		nil,
+		DefaultConfig(),
+		&Config{},
+	} {
+		if s := newConfigFile(cfg); s == "" {
+			t.Errorf("newConfigFile(nil) => %q, want non-empty string", s)
+		}
+	}
+}

pkg/config/ssh.go

@@ -1,8 +1,28 @@
 package config
 
-import "github.com/charmbracelet/keygen"
+import (
+	"errors"
+
+	"github.com/charmbracelet/keygen"
+)
+
+var (
+	// ErrNilConfig is returned when a nil config is passed to a function.
+	ErrNilConfig = errors.New("nil config")
+
+	// ErrEmptySSHKeyPath is returned when the SSH key path is empty.
+	ErrEmptySSHKeyPath = errors.New("empty SSH key path")
+)
 
 // KeyPair returns the server's SSH key pair.
-func (c SSHConfig) KeyPair() (*keygen.SSHKeyPair, error) {
-	return keygen.New(c.KeyPath, keygen.WithKeyType(keygen.Ed25519))
+func KeyPair(cfg *Config) (*keygen.SSHKeyPair, error) {
+	if cfg == nil {
+		return nil, ErrNilConfig
+	}
+
+	if cfg.SSH.KeyPath == "" {
+		return nil, ErrEmptySSHKeyPath
+	}
+
+	return keygen.New(cfg.SSH.KeyPath, keygen.WithKeyType(keygen.Ed25519))
 }

pkg/config/ssh_test.go

@@ -0,0 +1,26 @@
+package config
+
+import "testing"
+
+func TestBadSSHKeyPair(t *testing.T) {
+	for _, cfg := range []*Config{
+		nil,
+		{},
+	} {
+		if _, err := KeyPair(cfg); err == nil {
+			t.Errorf("cfg.SSH.KeyPair() => _, nil, want non-nil error")
+		}
+	}
+}
+
+func TestGoodSSHKeyPair(t *testing.T) {
+	cfg := &Config{
+		SSH: SSHConfig{
+			KeyPath: "testdata/ssh_host_ed25519_key",
+		},
+	}
+
+	if _, err := KeyPair(cfg); err != nil {
+		t.Errorf("cfg.SSH.KeyPair() => _, %v, want nil error", err)
+	}
+}

pkg/cron/cron_test.go

@@ -0,0 +1,31 @@
+package cron
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/charmbracelet/log"
+)
+
+func TestCronLogger(t *testing.T) {
+	var buf bytes.Buffer
+	logger := log.New(&buf)
+	logger.SetLevel(log.DebugLevel)
+	clogger := cronLogger{logger}
+	clogger.Info("foo")
+	clogger.Error(fmt.Errorf("bar"), "test")
+	if buf.String() != "DEBU foo\nERRO test err=bar\n" {
+		t.Errorf("unexpected log output: %s", buf.String())
+	}
+}
+
+func TestSchedularAddRemove(t *testing.T) {
+	s := NewScheduler(context.TODO())
+	id, err := s.AddFunc("* * * * *", func() {})
+	if err != nil {
+		t.Fatal(err)
+	}
+	s.Remove(id)
+}

pkg/daemon/daemon.go

@@ -150,30 +150,35 @@ func (d *GitDaemon) handleClient(conn net.Conn) {
 		d.conns.Close(c) // nolint: errcheck
 	}()
 
-	readc := make(chan struct{}, 1)
+	errc := make(chan error, 1)
+
 	s := pktline.NewScanner(c)
 	go func() {
 		if !s.Scan() {
 			if err := s.Err(); err != nil {
-				if nerr, ok := err.(net.Error); ok && nerr.Timeout() {
-					d.fatal(c, git.ErrTimeout)
-				} else {
-					d.logger.Debugf("git: error scanning pktline: %v", err)
-					d.fatal(c, git.ErrSystemMalfunction)
-				}
+				errc <- err
 			}
-			return
 		}
-		readc <- struct{}{}
+		errc <- nil
 	}()
 
 	select {
 	case <-ctx.Done():
 		if err := ctx.Err(); err != nil {
 			d.logger.Debugf("git: connection context error: %v", err)
+			d.fatal(c, git.ErrTimeout)
 		}
 		return
-	case <-readc:
+	case err := <-errc:
+		if nerr, ok := err.(net.Error); ok && nerr.Timeout() {
+			d.fatal(c, git.ErrTimeout)
+			return
+		} else if err != nil {
+			d.logger.Debugf("git: error scanning pktline: %v", err)
+			d.fatal(c, git.ErrSystemMalfunction)
+			return
+		}
+
 		line := s.Bytes()
 		split := bytes.SplitN(line, []byte{' '}, 2)
 		if len(split) != 2 {

pkg/daemon/daemon_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/charmbracelet/soft-serve/pkg/backend"
 	"github.com/charmbracelet/soft-serve/pkg/config"
@@ -50,7 +51,7 @@ func TestMain(m *testing.M) {
 	}
 	datastore := database.New(ctx, dbx)
 	ctx = store.WithContext(ctx, datastore)
-	be := backend.New(ctx, cfg, dbx)
+	be := backend.New(ctx, cfg, dbx, datastore)
 	ctx = backend.WithContext(ctx, be)
 	d, err := NewGitDaemon(ctx)
 	if err != nil {
@@ -78,9 +79,10 @@ func TestIdleTimeout(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	time.Sleep(time.Second)
 	_, err = readPktline(c)
-	if err != nil && err.Error() != git.ErrTimeout.Error() {
-		t.Fatalf("expected %q error, got %q", git.ErrTimeout, err)
+	if err == nil {
+		t.Errorf("expected error, got nil")
 	}
 }
 
@@ -94,7 +96,7 @@ func TestInvalidRepo(t *testing.T) {
 	}
 	_, err = readPktline(c)
 	if err != nil && err.Error() != git.ErrInvalidRepo.Error() {
-		t.Fatalf("expected %q error, got %q", git.ErrInvalidRepo, err)
+		t.Errorf("expected %q error, got %q", git.ErrInvalidRepo, err)
 	}
 }