refactor: avoid computing stats and conclusion

apps/backend/package.json

@@ -15,7 +15,7 @@
     "db:drop": "knex-scripts drop",
     "db:dump": "pnpm run db:migrate:latest && knex-scripts dump",
     "db:load": "knex-scripts load",
-    "db:seed": "knex seed:run",
+    "db:seed": "node dist/database/bin/run-seed.js",
     "db:truncate": "knex-scripts truncate",
     "db:migrate:latest": "knex migrate:latest",
     "db:migrate:rollback": "knex migrate:rollback",

apps/backend/src/api/handlers/finalizeBuilds.e2e.test.ts

@@ -2,6 +2,7 @@ import { invariant } from "@argos/util/invariant";
 import request from "supertest";
 import { beforeEach, describe, expect, it } from "vitest";
 
+import { concludeBuild } from "@/build/concludeBuild";
 import type {
   Build,
   Project,
@@ -58,6 +59,7 @@ describe("finalizeBuilds", () => {
   describe("with a bucket already complete", () => {
     beforeEach(async () => {
       await compareScreenshotBucket.$query().patch({ complete: true });
+      await concludeBuild({ buildId: build.id, notify: false });
     });
 
     it("returns 200 with the finalized build", async () => {
@@ -106,6 +108,7 @@ describe("finalizeBuilds", () => {
 
   describe("with a valid build", () => {
     it("returns 200 status code", async () => {
+      await concludeBuild({ buildId: build.id, notify: false });
       await request(app)
         .post(`/builds/finalize`)
         .set("Authorization", "Bearer the-awesome-token")

apps/backend/src/build-notification/comment.ts

@@ -28,9 +28,9 @@ function getBuildName(input: { build: Build; project: Project | null }) {
   return input.build.name;
 }
 
-export const getCommentBody = async (props: {
+export async function getCommentBody(props: {
   commit: string;
-}): Promise<string> => {
+}): Promise<string> {
   const builds = await Build.query()
     .distinctOn("builds.name", "builds.projectId")
     .joinRelated("compareScreenshotBucket")
@@ -44,45 +44,45 @@ export const getCommentBody = async (props: {
 
   const hasMultipleProjects =
     new Set(builds.map((build) => build.projectId)).size > 1;
-  const aggregateStatuses = await Build.getAggregatedBuildStatuses(builds);
-  const buildRows = await Promise.all(
-    builds
-      // Filter out builds that are not associated with a project that has PR comments enabled.
-      // We don't filter at SQL level because we still want to know if it's linked to multiple projects
-      // to determine if we should prepend the project name to the build name.
-      .filter((build) => {
-        invariant(build.project, "Relation `project` should be fetched");
-        return build.project.prCommentEnabled;
-      })
-      .map(async (build, index) => {
-        invariant(build.project, "Relation `project` should be fetched");
+  const [aggregateStatuses, urls] = await Promise.all([
+    Build.getAggregatedBuildStatuses(builds),
+    Promise.all(builds.map((build) => build.getUrl())),
+  ]);
+  const buildRows = builds
+    // Filter out builds that are not associated with a project that has PR comments enabled.
+    // We don't filter at SQL level because we still want to know if it's linked to multiple projects
+    // to determine if we should prepend the project name to the build name.
+    .filter((build) => {
+      invariant(build.project, "Relation `project` should be fetched");
+      return build.project.prCommentEnabled;
+    })
+    .map((build, index) => {
+      invariant(build.project, "Relation `project` should be fetched");
 
-        const [stats, url] = await Promise.all([
-          getStatsMessage(build.id),
-          build.getUrl(),
-        ]);
+      const url = urls[index];
+      invariant(url, "missing build URL");
 
-        const status = aggregateStatuses[index];
-        invariant(status, "unknown build status");
+      const status = aggregateStatuses[index];
+      invariant(status, "missing build status");
 
-        const label = getBuildLabel(build.type, status);
-        const review =
-          status === "changes-detected" ? ` ([Review](${url}))` : "";
-        const name = getBuildName({
-          build,
-          project: hasMultipleProjects ? build.project : null,
-        });
+      const stats = build.stats ? getStatsMessage(build.stats) : null;
 
-        return `| **${name}** ([Inspect](${url})) | ${label}${review} | ${
-          stats || "-"
-        } | ${formatDate(build.updatedAt)} |`;
-      }),
-  );
+      const label = getBuildLabel(build.type, status);
+      const review = status === "changes-detected" ? ` ([Review](${url}))` : "";
+      const name = getBuildName({
+        build,
+        project: hasMultipleProjects ? build.project : null,
+      });
+
+      return `| **${name}** ([Inspect](${url})) | ${label}${review} | ${
+        stats || "-"
+      } | ${formatDate(build.updatedAt)} |`;
+    });
   return [
     getCommentHeader(),
     "",
     `| Build | Status | Details | Updated (UTC) |`,
     `| :---- | :----- | :------ | :------------ |`,
     ...buildRows.sort(),
   ].join("\n");
-};
+}

apps/backend/src/build-notification/notification.e2e.test.ts

@@ -1,6 +1,7 @@
 import { invariant } from "@argos/util/invariant";
 import { beforeEach, describe, expect, it } from "vitest";
 
+import { concludeBuild } from "@/build/concludeBuild.js";
 import { Build } from "@/database/models/Build.js";
 import { BuildNotification } from "@/database/models/BuildNotification.js";
 import { factory } from "@/database/testing/index.js";
@@ -36,6 +37,8 @@ describe("#getNotificationPayload", () => {
       buildNotification = await factory.BuildNotification.create({
         buildId: build.id,
       });
+      await concludeBuild({ buildId: build.id, notify: false });
+      build = await build.$query();
     });
 
     it("returns argos as context", async () => {
@@ -70,6 +73,8 @@ describe("#getNotificationPayload", () => {
       buildNotification = await factory.BuildNotification.create({
         buildId: build.id,
       });
+      await concludeBuild({ buildId: build.id, notify: false });
+      build = await build.$query();
     });
 
     it("returns argos as context", async () => {

apps/backend/src/build-notification/notification.ts

@@ -123,23 +123,28 @@ export function getNotificationStates(input: {
   }
 }
 
+function getBuildStatsMessage(build: Build): string {
+  invariant(build.stats, "Build should be concluded");
+  return getStatsMessage(build.stats);
+}
+
 /**
  * Get the notification description for each platform based on the build
  * notification type and if it's a auto-approved build.
  */
-async function getNotificationDescription(input: {
+function getNotificationDescription(input: {
   buildNotificationType: BuildNotification["type"];
-  buildId: string;
+  build: Build;
   isAutoApproved: boolean;
-}): Promise<string> {
-  const { buildNotificationType, buildId, isAutoApproved } = input;
+}): string {
+  const { buildNotificationType, build, isAutoApproved } = input;
   switch (buildNotificationType) {
     case "queued":
       return "Build is queued";
     case "progress":
       return "Build in progress...";
     case "no-diff-detected": {
-      const statsMessage = await getStatsMessage(buildId);
+      const statsMessage = getBuildStatsMessage(build);
       if (!statsMessage) {
         if (isAutoApproved) {
           return "Auto-approved, no changes found";
@@ -152,18 +157,18 @@ async function getNotificationDescription(input: {
       return `${statsMessage} — no changes found`;
     }
     case "diff-detected": {
-      const statsMessage = await getStatsMessage(buildId);
+      const statsMessage = getBuildStatsMessage(build);
       if (isAutoApproved) {
         return `${statsMessage}, automatically approved`;
       }
       return `${statsMessage} — waiting for your decision`;
     }
     case "diff-accepted": {
-      const statsMessage = await getStatsMessage(buildId);
+      const statsMessage = getBuildStatsMessage(build);
       return `${statsMessage} — approved`;
     }
     case "diff-rejected": {
-      const statsMessage = await getStatsMessage(buildId);
+      const statsMessage = getBuildStatsMessage(build);
       return `${statsMessage} — rejected`;
     }
     default: {
@@ -179,18 +184,16 @@ export async function getNotificationPayload(input: {
   buildNotification: Pick<BuildNotification, "type">;
   build: Build;
 }): Promise<NotificationPayload> {
-  const [description, context] = await Promise.all([
-    getNotificationDescription({
-      buildNotificationType: input.buildNotification.type,
-      buildId: input.build.id,
-      isAutoApproved: input.build.type === "reference",
-    }),
-    getStatusContext(input.build),
-  ]);
+  const description = getNotificationDescription({
+    buildNotificationType: input.buildNotification.type,
+    build: input.build,
+    isAutoApproved: input.build.type === "reference",
+  });
   const states = getNotificationStates({
     buildNotificationType: input.buildNotification.type,
     isAutoApproved: input.build.type === "reference",
   });
+  const context = await getStatusContext(input.build);
 
   return {
     description,

apps/backend/src/build/concludeBuild.ts

@@ -9,33 +9,43 @@ import { Build, BuildConclusion, BuildNotification } from "@/database/models";
  * Concludes the build by updating the conclusion and the stats.
  * Called when all diffs are processed.
  */
-export async function concludeBuild(input: { buildId: string }) {
-  const { buildId } = input;
+export async function concludeBuild(input: {
+  buildId: string;
+  notify?: boolean;
+}) {
+  const { buildId, notify = true } = input;
   const statuses = await Build.getScreenshotDiffsStatuses([buildId]);
   const [[conclusion], [stats]] = await Promise.all([
-    Build.getConclusions([buildId], statuses),
-    Build.getStats([buildId]),
+    Build.computeConclusions([buildId], statuses),
+    Build.computeStats([buildId]),
   ]);
   invariant(stats !== undefined, "No stats found");
   invariant(conclusion !== undefined, "No conclusion found");
   // If the build is not yet concluded, we don't want to update it.
   if (conclusion === null) {
     return;
   }
-  const [, buildNotification] = await transaction(async (trx) => {
-    return Promise.all([
-      Build.query(trx).findById(buildId).patch({
-        conclusion,
-        stats,
-      }),
-      BuildNotification.query(trx).insert({
-        buildId,
-        type: getNotificationType(conclusion),
-        jobStatus: "pending",
-      }),
-    ]);
-  });
-  await buildNotificationJob.push(buildNotification.id);
+  if (notify) {
+    const [, buildNotification] = await transaction(async (trx) => {
+      return Promise.all([
+        Build.query(trx).findById(buildId).patch({
+          conclusion,
+          stats,
+        }),
+        BuildNotification.query(trx).insert({
+          buildId,
+          type: getNotificationType(conclusion),
+          jobStatus: "pending",
+        }),
+      ]);
+    });
+    await buildNotificationJob.push(buildNotification.id);
+  } else {
+    await Build.query().findById(buildId).patch({
+      conclusion,
+      stats,
+    });
+  }
 }
 
 function getNotificationType(conclusion: BuildConclusion) {

apps/backend/src/build/stats.ts

@@ -1,15 +1,11 @@
-import { invariant } from "@argos/util/invariant";
-
 import { Build } from "@/database/models/index.js";
 
 /**
  * Get a message for the build stats.
  * Example: 40 changed, 20 added, 10 removed, 5 failures
  * Around 45 characters max.
  */
-export async function getStatsMessage(buildId: string): Promise<string> {
-  const [stats] = await Build.getStats([buildId]);
-  invariant(stats, "Build stats not found");
+export function getStatsMessage(stats: NonNullable<Build["stats"]>): string {
   const parts = [];
   if (stats.changed > 0) {
     parts.push(`${stats.changed} changed`);

apps/backend/src/config/index.ts

@@ -286,13 +286,6 @@ const config = convict({
         default: join(__dirname, "../../db/migrations"),
       },
     },
-    seeds: {
-      directory: {
-        doc: "Seeds directory",
-        format: String,
-        default: join(__dirname, "../../db/seeds"),
-      },
-    },
     client: {
       doc: "Knex client",
       format: String,

apps/backend/src/database/bin/run-seed.ts

@@ -0,0 +1,5 @@
+import { seed } from "../seeds";
+
+await seed();
+console.log("Seeding complete");
+process.exit(0);