Merge pull request #2358 from PR0M3TH3AN/testing-improvement-s3uploadservice-13802788661266692247

🧪 Add tests for S3UploadService and refactor for testability

js/services/s3UploadService.js

@@ -30,31 +30,52 @@ function isValidInfoHash(value) {
   return INFO_HASH_PATTERN.test(value);
 }
 
-async function resolveUploadIdentifier({ infoHash = "", file = null } = {}) {
-  const normalizedInfoHash = normalizeInfoHash(infoHash);
-  if (isValidInfoHash(normalizedInfoHash)) {
-    return normalizedInfoHash;
-  }
-  if (!file) {
-    return "";
-  }
-  try {
-    const computedHash = await calculateTorrentInfoHash(file);
-    const normalizedComputed = normalizeInfoHash(computedHash);
-    if (isValidInfoHash(normalizedComputed)) {
-      return normalizedComputed;
-    }
-  } catch (err) {
-    userLogger.warn("Failed to precompute info hash for storage key:", err);
-  }
-  return "";
-}
+const defaultDeps = {
+  ensureS3SdkLoaded,
+  makeS3Client,
+  multipartUpload,
+  buildR2Key,
+  buildS3ObjectUrl,
+  getCorsOrigins,
+  prepareS3Connection,
+  validateS3Connection,
+  userLogger,
+  buildStoragePointerValue,
+  buildStoragePrefixFromKey,
+  getVideoNoteErrorMessage,
+  normalizeVideoNotePayload,
+  calculateTorrentInfoHash,
+};
 
-class S3UploadService {
-  constructor() {
+export class S3UploadService {
+  constructor(deps = {}) {
+    this.deps = { ...defaultDeps, ...deps };
     this.listeners = new Map();
   }
 
+  async _resolveUploadIdentifier({ infoHash = "", file = null } = {}) {
+    const normalizedInfoHash = normalizeInfoHash(infoHash);
+    if (isValidInfoHash(normalizedInfoHash)) {
+      return normalizedInfoHash;
+    }
+    if (!file) {
+      return "";
+    }
+    try {
+      const computedHash = await this.deps.calculateTorrentInfoHash(file);
+      const normalizedComputed = normalizeInfoHash(computedHash);
+      if (isValidInfoHash(normalizedComputed)) {
+        return normalizedComputed;
+      }
+    } catch (err) {
+      this.deps.userLogger.warn(
+        "Failed to precompute info hash for storage key:",
+        err
+      );
+    }
+    return "";
+  }
+
   on(event, handler) {
     if (typeof handler !== "function") {
       return () => {};
@@ -81,7 +102,11 @@ class S3UploadService {
       try {
         handler(detail);
       } catch (err) {
-        userLogger.error("[s3UploadService] Listener error for", event, err);
+        this.deps.userLogger.error(
+          "[s3UploadService] Listener error for",
+          event,
+          err
+        );
       }
     }
   }
@@ -113,9 +138,11 @@ class S3UploadService {
     createBucketIfMissing = true,
     origins = null,
   } = {}) {
-    const normalized = validateS3Connection(settings);
-    const allowedOrigins = Array.isArray(origins) ? origins : getCorsOrigins();
-    const prepared = await prepareS3Connection({
+    const normalized = this.deps.validateS3Connection(settings);
+    const allowedOrigins = Array.isArray(origins)
+      ? origins
+      : this.deps.getCorsOrigins();
+    const prepared = await this.deps.prepareS3Connection({
       ...normalized,
       origins: allowedOrigins,
       createBucketIfMissing,
@@ -124,7 +151,10 @@ class S3UploadService {
   }
 
   async prepareUpload(settings = {}, { createBucketIfMissing = true } = {}) {
-    const prepared = await this.verifyConnection({ settings, createBucketIfMissing });
+    const prepared = await this.verifyConnection({
+      settings,
+      createBucketIfMissing,
+    });
     return {
       settings: prepared,
       bucketEntry: {
@@ -157,9 +187,9 @@ class S3UploadService {
       throw new Error("Missing required parameters for uploadFile");
     }
 
-    await ensureS3SdkLoaded();
+    await this.deps.ensureS3SdkLoaded();
 
-    const s3 = makeS3Client({
+    const s3 = this.deps.makeS3Client({
       endpoint,
       region,
       accessKeyId,
@@ -167,7 +197,7 @@ class S3UploadService {
       forcePathStyle: Boolean(forcePathStyle),
     });
 
-    await multipartUpload({
+    await this.deps.multipartUpload({
       s3,
       bucket,
       key,
@@ -215,7 +245,7 @@ class S3UploadService {
 
     if (!title) {
       this.setUploadStatus(
-        getVideoNoteErrorMessage(VIDEO_NOTE_ERROR_CODES.MISSING_TITLE),
+        this.deps.getVideoNoteErrorMessage(VIDEO_NOTE_ERROR_CODES.MISSING_TITLE),
         "error"
       );
       return false;
@@ -228,7 +258,7 @@ class S3UploadService {
 
     let normalized = null;
     try {
-      normalized = validateS3Connection(settings);
+      normalized = this.deps.validateS3Connection(settings);
     } catch (err) {
       this.setUploadStatus(err?.message || "Invalid S3 settings.", "error");
       return false;
@@ -238,13 +268,16 @@ class S3UploadService {
     this.updateProgress(0);
 
     try {
-      await ensureS3SdkLoaded();
+      await this.deps.ensureS3SdkLoaded();
 
-      const keyIdentifier = await resolveUploadIdentifier({ infoHash, file });
+      const keyIdentifier = await this._resolveUploadIdentifier({
+        infoHash,
+        file,
+      });
       const normalizedInfoHash = normalizeInfoHash(infoHash || keyIdentifier);
       const hasValidInfoHash = isValidInfoHash(normalizedInfoHash);
 
-      const s3 = makeS3Client({
+      const s3 = this.deps.makeS3Client({
         endpoint: normalized.endpoint,
         region: normalized.region,
         accessKeyId: normalized.accessKeyId,
@@ -252,10 +285,12 @@ class S3UploadService {
         forcePathStyle: Boolean(normalized.forcePathStyle),
       });
 
-      const key = forcedVideoKey || buildR2Key(npub, file, keyIdentifier);
+      const key =
+        forcedVideoKey ||
+        this.deps.buildR2Key(npub, file, keyIdentifier);
       const publicUrl =
         forcedVideoUrl ||
-        buildS3ObjectUrl({
+        this.deps.buildS3ObjectUrl({
           publicBaseUrl: normalized.publicBaseUrl,
           endpoint: normalized.endpoint,
           bucket: normalized.bucket,
@@ -271,7 +306,7 @@ class S3UploadService {
         const thumbExt = thumbnailFile.name.split(".").pop() || "jpg";
         const thumbKey = key.replace(/\.[^/.]+$/, "") + `.thumb.${thumbExt}`;
         try {
-          await multipartUpload({
+          await this.deps.multipartUpload({
             s3,
             bucket: normalized.bucket,
             key: thumbKey,
@@ -280,7 +315,7 @@ class S3UploadService {
             createBucketIfMissing,
             region: normalized.region,
           });
-          const thumbUrl = buildS3ObjectUrl({
+          const thumbUrl = this.deps.buildS3ObjectUrl({
             publicBaseUrl: normalized.publicBaseUrl,
             endpoint: normalized.endpoint,
             bucket: normalized.bucket,
@@ -291,13 +326,16 @@ class S3UploadService {
             metadata.thumbnail = thumbUrl;
           }
         } catch (err) {
-          userLogger.warn("Thumbnail upload failed, continuing with video...", err);
+          this.deps.userLogger.warn(
+            "Thumbnail upload failed, continuing with video...",
+            err
+          );
         }
       }
 
       this.setUploadStatus(statusMessage, "info");
 
-      await multipartUpload({
+      await this.deps.multipartUpload({
         s3,
         bucket: normalized.bucket,
         key,
@@ -323,7 +361,7 @@ class S3UploadService {
             return `${key}.torrent`;
           })();
         try {
-          await multipartUpload({
+          await this.deps.multipartUpload({
             s3,
             bucket: normalized.bucket,
             key: torrentKey,
@@ -333,7 +371,7 @@ class S3UploadService {
             region: normalized.region,
           });
           if (!torrentUrl) {
-            torrentUrl = buildS3ObjectUrl({
+            torrentUrl = this.deps.buildS3ObjectUrl({
               publicBaseUrl: normalized.publicBaseUrl,
               endpoint: normalized.endpoint,
               bucket: normalized.bucket,
@@ -342,12 +380,17 @@ class S3UploadService {
             });
           }
         } catch (err) {
-          userLogger.warn("Torrent metadata upload failed, continuing...", err);
+          this.deps.userLogger.warn(
+            "Torrent metadata upload failed, continuing...",
+            err
+          );
         }
       }
 
       if (typeof publishVideoNote !== "function") {
-        userLogger.warn("publishVideoNote handler missing; skipping publish step.");
+        this.deps.userLogger.warn(
+          "publishVideoNote handler missing; skipping publish step."
+        );
         return false;
       }
 
@@ -366,7 +409,10 @@ class S3UploadService {
         generatedWs = publicUrl;
       } else {
         if (infoHash) {
-          userLogger.warn("Invalid info hash provided. Skipping magnet generation.", infoHash);
+          this.deps.userLogger.warn(
+            "Invalid info hash provided. Skipping magnet generation.",
+            infoHash
+          );
         }
         this.setUploadStatus(
           "Info hash missing or invalid. Publishing URL-first without WebTorrent fallback.",
@@ -374,11 +420,11 @@ class S3UploadService {
         );
       }
 
-      const storagePrefix = buildStoragePrefixFromKey({
+      const storagePrefix = this.deps.buildStoragePrefixFromKey({
         publicBaseUrl: normalized.publicBaseUrl,
         key,
       });
-      const storagePointer = buildStoragePointerValue({
+      const storagePointer = this.deps.buildStoragePointerValue({
         provider: "s3",
         prefix: storagePrefix,
       });
@@ -398,9 +444,10 @@ class S3UploadService {
         isForKids: metadata?.isForKids,
       };
 
-      const { payload, errors } = normalizeVideoNotePayload(rawVideoPayload);
+      const { payload, errors } =
+        this.deps.normalizeVideoNotePayload(rawVideoPayload);
       if (errors.length) {
-        const message = getVideoNoteErrorMessage(errors[0]);
+        const message = this.deps.getVideoNoteErrorMessage(errors[0]);
         this.setUploadStatus(message, "error");
         return false;
       }
@@ -418,7 +465,7 @@ class S3UploadService {
       }
       return Boolean(published);
     } catch (err) {
-      userLogger.error("S3 upload failed:", err);
+      this.deps.userLogger.error("S3 upload failed:", err);
       this.setUploadStatus(
         err?.message ? `Upload failed: ${err.message}` : "Upload failed.",
         "error"

tests/services/s3UploadService.test.mjs

@@ -0,0 +1,228 @@
+import "../test-helpers/setup-localstorage.mjs";
+import { test, mock } from "node:test";
+import assert from "node:assert/strict";
+import { S3UploadService } from "../../js/services/s3UploadService.js";
+
+// Mock VIDEO_NOTE_ERROR_CODES constants if needed, but they are imported in the service.
+// We can just rely on the strings we return from the mocked getVideoNoteErrorMessage.
+
+function createMockDeps() {
+  return {
+    ensureS3SdkLoaded: mock.fn(async () => {}),
+    makeS3Client: mock.fn(() => ({})),
+    multipartUpload: mock.fn(async () => {}),
+    buildR2Key: mock.fn(() => "mock-key"),
+    buildS3ObjectUrl: mock.fn(() => "https://mock-url.com/file"),
+    getCorsOrigins: mock.fn(() => ["*"]),
+    prepareS3Connection: mock.fn(async (opts) => ({
+      ...opts,
+      bucket: "mock-bucket",
+      publicBaseUrl: "https://mock-url.com",
+    })),
+    validateS3Connection: mock.fn((settings) => ({
+      ...settings,
+      bucket: "mock-bucket",
+      endpoint: "https://s3.mock",
+    })),
+    userLogger: {
+      warn: mock.fn(),
+      error: mock.fn(),
+    },
+    buildStoragePointerValue: mock.fn(() => "mock-pointer"),
+    buildStoragePrefixFromKey: mock.fn(() => "mock-prefix"),
+    getVideoNoteErrorMessage: mock.fn((code) => `Error: ${code}`),
+    normalizeVideoNotePayload: mock.fn((payload) => ({
+      payload,
+      errors: [],
+    })),
+    calculateTorrentInfoHash: mock.fn(async () => "mock-info-hash"),
+  };
+}
+
+test("S3UploadService", async (t) => {
+  await t.test("constructor initializes listeners", () => {
+    const service = new S3UploadService(createMockDeps());
+    assert.ok(service.listeners instanceof Map);
+  });
+
+  await t.test("Event Emitter Logic", async (t) => {
+    const service = new S3UploadService(createMockDeps());
+    const handler = mock.fn();
+
+    // on
+    const unsubscribe = service.on("test-event", handler);
+    assert.ok(service.listeners.has("test-event"));
+    assert.strictEqual(service.listeners.get("test-event").size, 1);
+
+    // emit
+    service.emit("test-event", { foo: "bar" });
+    assert.strictEqual(handler.mock.callCount(), 1);
+    assert.deepStrictEqual(handler.mock.calls[0].arguments[0], { foo: "bar" });
+
+    // unsubscribe
+    unsubscribe();
+    assert.ok(!service.listeners.has("test-event"));
+  });
+
+  await t.test("emit handles listener errors safely", () => {
+    const deps = createMockDeps();
+    const service = new S3UploadService(deps);
+    const handler = mock.fn(() => {
+      throw new Error("oops");
+    });
+
+    service.on("error-event", handler);
+    service.emit("error-event", {});
+
+    assert.strictEqual(handler.mock.callCount(), 1);
+    assert.strictEqual(deps.userLogger.error.mock.callCount(), 1);
+    assert.match(
+      deps.userLogger.error.mock.calls[0].arguments[0],
+      /Listener error for/
+    );
+  });
+
+  await t.test("verifyConnection", async () => {
+    const deps = createMockDeps();
+    const service = new S3UploadService(deps);
+    const settings = { foo: "bar" };
+
+    const result = await service.verifyConnection({ settings });
+
+    assert.strictEqual(deps.validateS3Connection.mock.callCount(), 1);
+    assert.strictEqual(deps.prepareS3Connection.mock.callCount(), 1);
+    assert.strictEqual(result.bucket, "mock-bucket");
+  });
+
+  await t.test("prepareUpload", async () => {
+    const deps = createMockDeps();
+    const service = new S3UploadService(deps);
+    const settings = { foo: "bar" };
+
+    const result = await service.prepareUpload(settings);
+
+    assert.ok(result.settings);
+    assert.ok(result.bucketEntry);
+    assert.strictEqual(result.bucketEntry.bucket, "mock-bucket");
+  });
+
+  await t.test("uploadFile", async (t) => {
+    await t.test("validates parameters", async () => {
+      const service = new S3UploadService(createMockDeps());
+      await assert.rejects(
+        () => service.uploadFile({}),
+        /Missing required parameters/
+      );
+    });
+
+    await t.test("uploads successfully", async () => {
+      const deps = createMockDeps();
+      const service = new S3UploadService(deps);
+      const file = new File(["content"], "test.txt", { type: "text/plain" });
+      const options = {
+        file,
+        endpoint: "https://s3.mock",
+        region: "us-east-1",
+        accessKeyId: "key",
+        secretAccessKey: "secret",
+        bucket: "bucket",
+        key: "key",
+      };
+
+      const result = await service.uploadFile(options);
+
+      assert.strictEqual(deps.ensureS3SdkLoaded.mock.callCount(), 1);
+      assert.strictEqual(deps.makeS3Client.mock.callCount(), 1);
+      assert.strictEqual(deps.multipartUpload.mock.callCount(), 1);
+      assert.strictEqual(result.bucket, "bucket");
+      assert.strictEqual(result.key, "key");
+    });
+  });
+
+  await t.test("uploadVideo", async (t) => {
+    const validParams = {
+      npub: "npub123",
+      file: new File(["video"], "video.mp4", { type: "video/mp4" }),
+      metadata: { title: "My Video" },
+      publishVideoNote: mock.fn(async () => true),
+    };
+
+    await t.test("fails if npub missing", async () => {
+      const service = new S3UploadService(createMockDeps());
+      const result = await service.uploadVideo({ ...validParams, npub: "" });
+      assert.strictEqual(result, false);
+    });
+
+    await t.test("fails if title missing", async () => {
+      const service = new S3UploadService(createMockDeps());
+      const result = await service.uploadVideo({
+        ...validParams,
+        metadata: { title: "" },
+      });
+      assert.strictEqual(result, false);
+    });
+
+    await t.test("fails if file missing", async () => {
+      const service = new S3UploadService(createMockDeps());
+      const result = await service.uploadVideo({ ...validParams, file: null });
+      assert.strictEqual(result, false);
+    });
+
+    await t.test("successful upload flow", async () => {
+      const deps = createMockDeps();
+      const service = new S3UploadService(deps);
+
+      // Spy on status updates
+      const statusHandler = mock.fn();
+      service.on("uploadStatus", statusHandler);
+
+      const result = await service.uploadVideo(validParams);
+
+      assert.strictEqual(result, true);
+      assert.strictEqual(deps.ensureS3SdkLoaded.mock.callCount(), 1);
+      assert.strictEqual(deps.makeS3Client.mock.callCount(), 1);
+      // multipartUpload called for video
+      assert.ok(deps.multipartUpload.mock.callCount() >= 1);
+      assert.strictEqual(deps.calculateTorrentInfoHash.mock.callCount(), 1);
+      assert.strictEqual(validParams.publishVideoNote.mock.callCount(), 1);
+    });
+
+    await t.test("uploads thumbnail if provided", async () => {
+      const deps = createMockDeps();
+      const service = new S3UploadService(deps);
+      const thumbnailFile = new File(["img"], "thumb.jpg", {
+        type: "image/jpeg",
+      });
+
+      await service.uploadVideo({ ...validParams, thumbnailFile });
+
+      // multipartUpload called for video and thumbnail
+      assert.ok(deps.multipartUpload.mock.callCount() >= 2);
+    });
+
+    await t.test("handles validation errors from normalizeVideoNotePayload", async () => {
+      const deps = createMockDeps();
+      deps.normalizeVideoNotePayload = mock.fn(() => ({
+        payload: null,
+        errors: ["INVALID_DATA"],
+      }));
+      const service = new S3UploadService(deps);
+
+      const result = await service.uploadVideo(validParams);
+
+      assert.strictEqual(result, false);
+      assert.strictEqual(deps.getVideoNoteErrorMessage.mock.callCount(), 1);
+    });
+
+    await t.test("handles upload exception", async () => {
+        const deps = createMockDeps();
+        deps.multipartUpload = mock.fn(async () => { throw new Error("Upload failed"); });
+        const service = new S3UploadService(deps);
+
+        const result = await service.uploadVideo(validParams);
+
+        assert.strictEqual(result, false);
+        assert.strictEqual(deps.userLogger.error.mock.callCount(), 1);
+    });
+  });
+});