From 9293372d94c749e805f1ce50d6d9ebcda88375ef Mon Sep 17 00:00:00 2001
From: garethgeorge <garethgeorge97@gmail.com>
Date: Sun, 12 Nov 2023 03:50:42 -0800
Subject: [PATCH] chore: orchestrator test coverage and some optimizations

---
 internal/orchestrator/orchestrator.go |   7 +-
 internal/orchestrator/repo.go         |  39 +++++---
 internal/orchestrator/repo_test.go    | 132 ++++++++++++++++++++++++++
 pkg/restic/restic.go                  |   2 +-
 webui/src/views/App.tsx               |   2 +-
 5 files changed, 163 insertions(+), 19 deletions(-)
 create mode 100644 internal/orchestrator/repo_test.go

diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go
index 3ff57e2c..403086a2 100644
--- a/internal/orchestrator/orchestrator.go
+++ b/internal/orchestrator/orchestrator.go
@@ -15,8 +15,6 @@ var ErrRepoNotFound = errors.New("repo not found")
 var ErrRepoInitializationFailed = errors.New("repo initialization failed")
 var ErrPlanNotFound = errors.New("plan not found")
 
-
-
 // Orchestrator is responsible for managing repos and backups.
 type Orchestrator struct {
 	configProvider config.ConfigStore
@@ -112,10 +110,7 @@ func (rp *resticRepoPool) GetRepo(repoId string) (repo *RepoOrchestrator, err er
 	}
 
 	// Otherwise create a new repo.
-	repo = &RepoOrchestrator{
-		repoConfig: repoProto,
-		repo: restic.NewRepo(repoProto, opts...),
-	}
+	repo = newRepoOrchestrator(repoProto, restic.NewRepo(repoProto, opts...))
 	rp.repos[repoId] = repo
 	return repo, nil
 }
diff --git a/internal/orchestrator/repo.go b/internal/orchestrator/repo.go
index 595d33cf..ad0cdaac 100644
--- a/internal/orchestrator/repo.go
+++ b/internal/orchestrator/repo.go
@@ -19,11 +19,21 @@ type RepoOrchestrator struct {
 	repoConfig *v1.Repo
 	repo *restic.Repo
 
+	snapshotsMu sync.Mutex // enable very fast snapshot access IF no update is required.
 	snapshotsAge time.Time
 	snapshots []*restic.Snapshot
 }
 
+func newRepoOrchestrator(repoConfig *v1.Repo, repo *restic.Repo) *RepoOrchestrator {
+	return &RepoOrchestrator{
+		repoConfig: repoConfig,
+		repo: repo,
+	}
+}
+
 func (r *RepoOrchestrator) updateSnapshotsIfNeeded(ctx context.Context) error {
+	r.snapshotsMu.Lock()
+	defer r.snapshotsMu.Unlock()
 	if time.Since(r.snapshotsAge) > 10 * time.Minute {
 		r.snapshots = nil
 	}
@@ -32,48 +42,51 @@ func (r *RepoOrchestrator) updateSnapshotsIfNeeded(ctx context.Context) error {
 		return nil
 	}
 
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
 	snapshots, err := r.repo.Snapshots(ctx, restic.WithPropagatedEnvVars(restic.EnvToPropagate...))
 	if err != nil {
 		return fmt.Errorf("failed to update snapshots: %w", err)
 	}
 
 	sort.SliceStable(snapshots, func(i, j int) bool {
-		return snapshots[i].Time > snapshots[j].Time
+		return snapshots[i].Time < snapshots[j].Time
 	})
-
 	r.snapshots = snapshots
 
 	return nil
 }
 
 func (r *RepoOrchestrator) Snapshots(ctx context.Context) ([]*restic.Snapshot, error) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-
 	if err := r.updateSnapshotsIfNeeded(ctx); err != nil {
 		return nil, err
 	}
 
+	r.snapshotsMu.Lock()
+	defer r.snapshotsMu.Unlock()
 	return r.snapshots, nil
 }
 
 func (r *RepoOrchestrator) SnapshotsForPlan(ctx context.Context, plan *v1.Plan) ([]*restic.Snapshot, error) {
-	r.mu.Lock()
-	defer r.mu.Unlock()
-
 	if err := r.updateSnapshotsIfNeeded(ctx); err != nil {
 		return nil, err 
 	}
 
+	r.snapshotsMu.Lock()
+	defer r.snapshotsMu.Unlock()
 	return filterSnapshotsForPlan(r.snapshots, plan), nil
 }
 
-func (r *RepoOrchestrator) Backup(ctx context.Context, plan *v1.Plan, progressCallback func(event *restic.BackupProgressEntry)) error {
+func (r *RepoOrchestrator) Backup(ctx context.Context, plan *v1.Plan, progressCallback func(event *restic.BackupProgressEntry)) (*restic.BackupProgressEntry, error) {
 	snapshots, err := r.SnapshotsForPlan(ctx, plan)
 	if err != nil {
-		return err
+		return nil, fmt.Errorf("failed to get snapshots for plan: %w", err)
 	}
 
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
 	var opts []restic.BackupOption
 	opts = append(opts, restic.WithBackupPaths(plan.Paths...))
 	opts = append(opts, restic.WithBackupExcludes(plan.Excludes...))
@@ -84,7 +97,11 @@ func (r *RepoOrchestrator) Backup(ctx context.Context, plan *v1.Plan, progressCa
 		opts = append(opts, restic.WithBackupParent(snapshots[len(snapshots) - 1].Id))
 	}
 
-	panic("not yet implemented")
+	summary, err := r.repo.Backup(ctx, progressCallback, opts...)
+	if err != nil {
+		return nil, fmt.Errorf("failed to backup: %w", err)
+	}
+	return summary, nil
 }
 
 func filterSnapshotsForPlan(snapshots []*restic.Snapshot, plan *v1.Plan) []*restic.Snapshot {
diff --git a/internal/orchestrator/repo_test.go b/internal/orchestrator/repo_test.go
new file mode 100644
index 00000000..f67f8f54
--- /dev/null
+++ b/internal/orchestrator/repo_test.go
@@ -0,0 +1,132 @@
+package orchestrator
+
+import (
+	"context"
+	"slices"
+	"testing"
+
+	v1 "github.com/garethgeorge/resticui/gen/go/v1"
+	test "github.com/garethgeorge/resticui/internal/test/helpers"
+	"github.com/garethgeorge/resticui/pkg/restic"
+)
+
+func TestBackup(t *testing.T) {
+	t.Parallel()
+
+	repo := t.TempDir()
+	testData := test.CreateTestData(t)
+
+	// create a new repo with cache disabled for testing
+	r := &v1.Repo{
+		Id: "test",
+		Uri: repo,
+		Password: "test",
+		Flags: []string{"--no-cache"}, 
+	}
+	
+	plan := &v1.Plan{
+		Id: "test",
+		Repo: "test",
+		Paths: []string{testData},
+	}
+
+	orchestrator := newRepoOrchestrator(r, restic.NewRepo(r, restic.WithFlags("--no-cache")))
+
+	summary, err := orchestrator.Backup(context.Background(), plan, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if summary.SnapshotId == "" {
+		t.Fatal("expected snapshot id")
+	}
+
+	if summary.FilesNew != 100 {
+		t.Fatalf("expected 100 new files, got %d", summary.FilesNew)
+	}
+}
+
+func TestSnapshotParenting(t *testing.T) {
+	t.Parallel()
+	
+	repo := t.TempDir()
+	testData := test.CreateTestData(t)
+
+	// create a new repo with cache disabled for testing
+	r := &v1.Repo{
+		Id: "test",
+		Uri: repo,
+		Password: "test",
+		Flags: []string{"--no-cache"}, 
+	}
+
+	plans := []*v1.Plan{
+		&v1.Plan{
+			Id: "test",
+			Repo: "test",
+			Paths: []string{testData},
+		},
+		&v1.Plan{
+			Id: "test2",
+			Repo: "test",
+			Paths: []string{testData},
+		},
+	}
+
+	orchestrator := newRepoOrchestrator(r, restic.NewRepo(r, restic.WithFlags("--no-cache")))
+
+	for i := 0; i < 4; i ++{
+		for _, plan := range plans {
+			summary, err := orchestrator.Backup(context.Background(), plan, nil)
+			if err != nil {
+				t.Fatalf("failed to backup plan %s: %v", plan.Id, err)
+			}
+
+			if summary.SnapshotId == "" {
+				t.Errorf("expected snapshot id")
+			}
+
+			if summary.TotalFilesProcessed != 100 {
+				t.Logf("summary is: %+v", summary)
+				t.Errorf("expected 100 done files, got %d", summary.TotalFilesProcessed)
+			}
+		}
+	}
+	
+	for _, plan := range plans {
+		snapshots, err := orchestrator.SnapshotsForPlan(context.Background(), plan)
+		if err != nil {
+			t.Errorf("failed to get snapshots for plan %s: %v", plan.Id, err)
+			continue
+		}
+
+		if len(snapshots) != 4 {
+			t.Errorf("expected 4 snapshots, got %d", len(snapshots))
+		}
+
+		for i := 1; i < len(snapshots); i++ {
+			prev := snapshots[i - 1]
+			curr := snapshots[i]
+
+			if prev.ToProto().UnixTimeMs >= curr.ToProto().UnixTimeMs {
+				t.Errorf("snapshots are out of order")
+			}
+
+			if prev.Id != curr.Parent {
+				t.Errorf("expected snapshot %s to have parent %s, got %s", curr.Id, prev.Id, curr.Parent)
+			}
+
+			if !slices.Contains(curr.Tags, tagForPlan(plan)) {
+				t.Errorf("expected snapshot %s to have tag %s", curr.Id, tagForPlan(plan))
+			}
+		}
+	}
+
+	snapshots, err := orchestrator.Snapshots(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(snapshots) != 8 {
+		t.Errorf("expected 8 snapshots, got %d", len(snapshots))
+	}
+}
\ No newline at end of file
diff --git a/pkg/restic/restic.go b/pkg/restic/restic.go
index efb3bf00..3e297c44 100644
--- a/pkg/restic/restic.go
+++ b/pkg/restic/restic.go
@@ -26,7 +26,7 @@ type Repo struct {
 	extraEnv []string
 }
 
-// NewRepo instantiates a new repository. TODO: should not accept a v1.Repo 
+// NewRepo instantiates a new repository. TODO: should not accept a v1.Repo, should instead be configured by parameters.
 func NewRepo(repo *v1.Repo, opts ...GenericOption) *Repo {
 	opt := &GenericOpts{}
 	for _, o := range opts {
diff --git a/webui/src/views/App.tsx b/webui/src/views/App.tsx
index 5f25d0b8..7cdb77ef 100644
--- a/webui/src/views/App.tsx
+++ b/webui/src/views/App.tsx
@@ -45,7 +45,7 @@ export const App: React.FC = () => {
   const items = getSidenavItems(config);
 
   return (
-    <Layout>
+    <Layout style={{ height: "100vh" }}>
       <Header style={{ display: "flex", alignItems: "center" }}>
         <h1 style={{ color: colorTextLightSolid }}>ResticUI</h1>
       </Header>