From 600ff0ecd1c8f688e026b4b18f3c426744f380b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Sun, 7 Jul 2024 14:58:16 +0200 Subject: [PATCH] feat: add build metadata Also add notion of valid bucket. A bucket that can be used as baseline. --- .../20240707121753_build-metadata.js | 55 +++++++++++ apps/backend/db/seeds/seeds.js | 1 + apps/backend/db/structure.sql | 8 +- .../src/build/strategy/strategies/ci/query.ts | 1 + apps/backend/src/database/models/Build.ts | 6 ++ .../backend/src/database/models/BuildShard.ts | 6 ++ .../backend/src/database/models/Screenshot.ts | 26 +++--- .../database/models/ScreenshotBucket.test.ts | 2 + .../src/database/models/ScreenshotBucket.ts | 29 +++++- .../src/database/services/buildMetadata.ts | 91 +++++++++++++++++++ apps/backend/src/database/testing/factory.ts | 1 + apps/backend/src/web/api/v2/builds/update.ts | 38 ++++++-- apps/backend/src/web/api/v2/util.ts | 29 +++--- 13 files changed, 261 insertions(+), 32 deletions(-) create mode 100644 apps/backend/db/migrations/20240707121753_build-metadata.js create mode 100644 apps/backend/src/database/services/buildMetadata.ts diff --git a/apps/backend/db/migrations/20240707121753_build-metadata.js b/apps/backend/db/migrations/20240707121753_build-metadata.js new file mode 100644 index 000000000..00711e387 --- /dev/null +++ b/apps/backend/db/migrations/20240707121753_build-metadata.js @@ -0,0 +1,55 @@ +/** + * @param {import('knex').Knex} knex + */ +export const up = async (knex) => { + await knex.schema.alterTable("builds", async (table) => { + table.jsonb("metadata"); + }); + + await knex.schema.alterTable("build_shards", async (table) => { + table.jsonb("metadata"); + }); + + await knex.schema.alterTable("screenshot_buckets", (table) => { + table.boolean("valid"); + }); + + await knex.raw( + `UPDATE screenshot_buckets SET valid = true WHERE valid IS NULL`, + ); + + await knex.raw(` + ALTER TABLE screenshot_buckets ADD CONSTRAINT screenshot_buckets_valid_not_null_constraint CHECK (valid IS NOT NULL) NOT VALID; + `); + + await knex.raw(` + ALTER TABLE screenshot_buckets VALIDATE CONSTRAINT screenshot_buckets_valid_not_null_constraint; + `); + + await knex.raw(` + ALTER TABLE screenshot_buckets ALTER column valid SET NOT NULL; + `); + + await knex.raw(` + ALTER TABLE screenshot_buckets DROP CONSTRAINT screenshot_buckets_valid_not_null_constraint; + `); +}; + +/** + * @param {import('knex').Knex} knex + */ +export const down = async (knex) => { + await knex.schema.alterTable("builds", async (table) => { + table.dropColumn("metadata"); + }); + + await knex.schema.alterTable("build_shards", async (table) => { + table.dropColumn("metadata"); + }); + + await knex.schema.alterTable("screenshot_buckets", async (table) => { + table.dropColumn("valid"); + }); +}; + +export const config = { transaction: false }; diff --git a/apps/backend/db/seeds/seeds.js b/apps/backend/db/seeds/seeds.js index e06d7fe22..d5b13604a 100644 --- a/apps/backend/db/seeds/seeds.js +++ b/apps/backend/db/seeds/seeds.js @@ -149,6 +149,7 @@ export const seed = async (knex) => { createdAt: "2016-12-08T22:59:55Z", updatedAt: "2016-12-08T22:59:55Z", complete: true, + valid: true, screenshotCount: 0, }; diff --git a/apps/backend/db/structure.sql b/apps/backend/db/structure.sql index c7d7db633..31d25d64d 100644 --- a/apps/backend/db/structure.sql +++ b/apps/backend/db/structure.sql @@ -165,7 +165,8 @@ CREATE TABLE public.build_shards ( "createdAt" timestamp with time zone NOT NULL, "updatedAt" timestamp with time zone NOT NULL, "buildId" bigint NOT NULL, - index integer + index integer, + metadata jsonb ); @@ -221,6 +222,7 @@ CREATE TABLE public.builds ( "runId" character varying(255), "runAttempt" integer, partial boolean DEFAULT false NOT NULL, + metadata jsonb, CONSTRAINT builds_mode_check CHECK ((mode = ANY (ARRAY['ci'::text, 'monitoring'::text]))), CONSTRAINT builds_type_check CHECK ((type = ANY (ARRAY['reference'::text, 'check'::text, 'orphan'::text]))) ); @@ -930,6 +932,7 @@ CREATE TABLE public.screenshot_buckets ( "projectId" bigint NOT NULL, "screenshotCount" integer, mode text DEFAULT 'ci'::text NOT NULL, + valid boolean NOT NULL, CONSTRAINT chk_complete_true_screenshotcount_not_null CHECK (((complete = false) OR ("screenshotCount" IS NOT NULL))), CONSTRAINT screenshot_buckets_mode_check CHECK ((mode = ANY (ARRAY['ci'::text, 'monitoring'::text]))) ); @@ -2809,4 +2812,5 @@ INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('2024060 INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240614204320_build_shards.js', 1, NOW()); INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240616142430_build_shards_indices.js', 1, NOW()); INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240630151704_screenshot-threshold.js', 1, NOW()); -INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240706121810_screenshot-base-name.js', 1, NOW()); \ No newline at end of file +INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240706121810_screenshot-base-name.js', 1, NOW()); +INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20240707121753_build-metadata.js', 1, NOW()); \ No newline at end of file diff --git a/apps/backend/src/build/strategy/strategies/ci/query.ts b/apps/backend/src/build/strategy/strategies/ci/query.ts index dcfd7d39b..5a3ace6ad 100644 --- a/apps/backend/src/build/strategy/strategies/ci/query.ts +++ b/apps/backend/src/build/strategy/strategies/ci/query.ts @@ -34,6 +34,7 @@ export function queryBaseBucket(build: Build) { projectId: build.projectId, name: build.name, complete: true, + valid: true, mode: build.mode, }); } diff --git a/apps/backend/src/database/models/Build.ts b/apps/backend/src/database/models/Build.ts index 199077e04..330403bd1 100644 --- a/apps/backend/src/database/models/Build.ts +++ b/apps/backend/src/database/models/Build.ts @@ -11,6 +11,10 @@ import { z } from "zod"; import config from "@/config/index.js"; +import { + BuildMetadata, + BuildMetadataJsonSchema, +} from "../services/buildMetadata.js"; import { Model } from "../util/model.js"; import { jobModelSchema, @@ -92,6 +96,7 @@ export class Build extends Model { runId: { type: ["string", "null"] }, runAttempt: { type: ["integer", "null"] }, partial: { type: ["boolean", "null"] }, + metadata: BuildMetadataJsonSchema, }, }); @@ -116,6 +121,7 @@ export class Build extends Model { runId!: string | null; runAttempt!: number | null; partial!: boolean | null; + metadata!: BuildMetadata | null; static override get relationMappings(): RelationMappings { return { diff --git a/apps/backend/src/database/models/BuildShard.ts b/apps/backend/src/database/models/BuildShard.ts index f9c086b6d..80eec5fb2 100644 --- a/apps/backend/src/database/models/BuildShard.ts +++ b/apps/backend/src/database/models/BuildShard.ts @@ -1,5 +1,9 @@ import type { RelationMappings } from "objection"; +import { + BuildMetadata, + BuildMetadataJsonSchema, +} from "../services/buildMetadata.js"; import { Model } from "../util/model.js"; import { mergeSchemas, timestampsSchema } from "../util/schemas.js"; import { Build } from "./Build.js"; @@ -13,11 +17,13 @@ export class BuildShard extends Model { properties: { buildId: { type: "string" }, index: { type: ["integer", "null"] }, + metadata: BuildMetadataJsonSchema, }, }); buildId!: string; index!: number | null; + metadata!: BuildMetadata | null; static override get relationMappings(): RelationMappings { return { diff --git a/apps/backend/src/database/models/Screenshot.ts b/apps/backend/src/database/models/Screenshot.ts index 9dc92af48..3ca3dae18 100644 --- a/apps/backend/src/database/models/Screenshot.ts +++ b/apps/backend/src/database/models/Screenshot.ts @@ -44,12 +44,16 @@ export type ScreenshotMetadata = { export const ScreenshotMetadataJsonSchema = { type: ["object", "null"], + required: ["sdk", "automationLibrary"], + additionalProperties: false, properties: { url: { type: "string", }, viewport: { type: "object", + required: ["width", "height"], + additionalProperties: false, properties: { width: { type: "integer", @@ -60,7 +64,6 @@ export const ScreenshotMetadataJsonSchema = { minimum: 0, }, }, - required: ["width", "height"], }, colorScheme: { type: "string", @@ -74,6 +77,8 @@ export const ScreenshotMetadataJsonSchema = { oneOf: [ { type: "object", + required: ["title", "titlePath"], + additionalProperties: false, properties: { id: { type: "string", @@ -101,6 +106,8 @@ export const ScreenshotMetadataJsonSchema = { }, location: { type: "object", + required: ["file", "line", "column"], + additionalProperties: false, properties: { file: { type: "string", @@ -114,18 +121,16 @@ export const ScreenshotMetadataJsonSchema = { minimum: 0, }, }, - required: ["file", "line", "column"], }, }, - required: ["title", "titlePath"], - }, - { - type: "null", }, + { type: "null" }, ], }, browser: { type: "object", + required: ["name", "version"], + additionalProperties: false, properties: { name: { type: "string", @@ -134,10 +139,11 @@ export const ScreenshotMetadataJsonSchema = { type: "string", }, }, - required: ["name", "version"], }, automationLibrary: { type: "object", + required: ["name", "version"], + additionalProperties: false, properties: { name: { type: "string", @@ -146,10 +152,11 @@ export const ScreenshotMetadataJsonSchema = { type: "string", }, }, - required: ["name", "version"], }, sdk: { type: "object", + required: ["name", "version"], + additionalProperties: false, properties: { name: { type: "string", @@ -158,11 +165,8 @@ export const ScreenshotMetadataJsonSchema = { type: "string", }, }, - required: ["name", "version"], }, }, - required: ["sdk", "automationLibrary"], - additionalProperties: false, }; export class Screenshot extends Model { diff --git a/apps/backend/src/database/models/ScreenshotBucket.test.ts b/apps/backend/src/database/models/ScreenshotBucket.test.ts index 9273784f7..b5efbfd62 100644 --- a/apps/backend/src/database/models/ScreenshotBucket.test.ts +++ b/apps/backend/src/database/models/ScreenshotBucket.test.ts @@ -6,6 +6,8 @@ const baseData = { name: "878", branch: "BUGS-130", commit: "ff4474843ccab36e72814e321cbf6ab6a6303385", + valid: true, + complete: true, }; describe("ScreenshotBucket", () => { diff --git a/apps/backend/src/database/models/ScreenshotBucket.ts b/apps/backend/src/database/models/ScreenshotBucket.ts index 389e53850..d4de2255d 100644 --- a/apps/backend/src/database/models/ScreenshotBucket.ts +++ b/apps/backend/src/database/models/ScreenshotBucket.ts @@ -12,7 +12,7 @@ export class ScreenshotBucket extends Model { static override tableName = "screenshot_buckets"; static override jsonSchema = mergeSchemas(timestampsSchema, { - required: ["name", "commit", "branch"], + required: ["name", "commit", "branch", "valid"], properties: { name: { type: "string" }, complete: { type: "boolean" }, @@ -24,16 +24,43 @@ export class ScreenshotBucket extends Model { projectId: { type: "string" }, screenshotCount: { type: "integer" }, mode: { type: "string", enum: ["ci", "monitoring"] }, + valid: { type: "boolean" }, }, }); + /** + * The name of the screenshot bucket, identic to the build name. + */ name!: string; + /** + * True if screenshots from all shards have been uploaded. + */ complete!: boolean; + /** + * The commit hash of the build. + */ commit!: string; + /** + * The branch of the build. + */ branch!: string; + /** + * The project ID of the build. + */ projectId!: string; + /** + * The number of screenshots in the bucket. + */ screenshotCount!: number; + /** + * The mode of the build. + */ mode!: BuildMode; + /** + * True if the bucket is valid. + * A bucket is considered valid if it's created from a "passed" test suite. + */ + valid!: boolean; static override get relationMappings(): RelationMappings { return { diff --git a/apps/backend/src/database/services/buildMetadata.ts b/apps/backend/src/database/services/buildMetadata.ts new file mode 100644 index 000000000..48ec4a949 --- /dev/null +++ b/apps/backend/src/database/services/buildMetadata.ts @@ -0,0 +1,91 @@ +export type BuildMetadata = { + testReport?: { + status: "passed" | "failed" | "timedout" | "interrupted"; + stats?: { + startTime?: string; + duration?: number; + tests?: number; + expected?: number; + unexpected?: number; + }; + }; + automationLibrary: { + name: string; + version: string; + }; + sdk: { + name: string; + version: string; + }; +}; + +export const BuildMetadataJsonSchema = { + type: ["object", "null"], + required: ["sdk", "automationLibrary"], + additionalProperties: false, + properties: { + testReport: { + oneOf: [ + { + type: "object", + additionalProperties: false, + required: ["status"], + properties: { + status: { + type: "string", + enum: ["passed", "failed", "timedout", "interrupted"], + }, + stats: { + type: "object", + additionalProperties: false, + properties: { + startTime: { + type: "string", + }, + duration: { + type: "number", + }, + tests: { + type: "integer", + }, + expected: { + type: "integer", + }, + unexpected: { + type: "integer", + }, + }, + }, + }, + }, + { type: "null" }, + ], + }, + automationLibrary: { + type: "object", + required: ["name", "version"], + additionalProperties: false, + properties: { + name: { + type: "string", + }, + version: { + type: "string", + }, + }, + }, + sdk: { + type: "object", + required: ["name", "version"], + additionalProperties: false, + properties: { + name: { + type: "string", + }, + version: { + type: "string", + }, + }, + }, + }, +}; diff --git a/apps/backend/src/database/testing/factory.ts b/apps/backend/src/database/testing/factory.ts index 8a194bc4c..5a36e8b33 100644 --- a/apps/backend/src/database/testing/factory.ts +++ b/apps/backend/src/database/testing/factory.ts @@ -111,6 +111,7 @@ export const ScreenshotBucket = defineFactory(models.ScreenshotBucket, () => ({ projectId: Project.associate("id"), complete: true, screenshotCount: 0, + valid: true, })); export const Build = defineFactory(models.Build, () => { diff --git a/apps/backend/src/web/api/v2/builds/update.ts b/apps/backend/src/web/api/v2/builds/update.ts index eced80dbd..dac7bb655 100644 --- a/apps/backend/src/web/api/v2/builds/update.ts +++ b/apps/backend/src/web/api/v2/builds/update.ts @@ -3,6 +3,7 @@ import express, { Router } from "express"; import { job as buildJob } from "@/build/index.js"; import { raw, transaction } from "@/database/index.js"; import { Build, BuildShard, Screenshot } from "@/database/models/index.js"; +import { BuildMetadata } from "@/database/services/buildMetadata"; import { insertFilesAndScreenshots } from "@/database/services/screenshots.js"; import { getRedisLock } from "@/util/redis"; import { repoAuth } from "@/web/middlewares/repoAuth.js"; @@ -13,6 +14,13 @@ import { UpdateRequest, validateUpdateRequest } from "../util"; const router = Router(); export default router; +/** + * Check if the bucket is valid from the metadata. + */ +function checkIsBucketValidFromMetadata(metadata: BuildMetadata | null) { + return !metadata?.testReport || metadata.testReport.status === "passed"; +} + async function handleUpdateParallel({ req, build, @@ -39,6 +47,7 @@ async function handleUpdateParallel({ ? BuildShard.query(trx).insert({ buildId: build.id, index: req.body.parallelIndex, + metadata: req.body.metadata ?? null, }) : null, Build.query(trx) @@ -57,13 +66,19 @@ async function handleUpdateParallel({ }); if (parallelTotal === patchedBuild.batchCount) { - const screenshotCount = await Screenshot.query(trx) - .where("screenshotBucketId", build.compareScreenshotBucketId) - .resultSize(); + const [screenshotCount, shards] = await Promise.all([ + Screenshot.query(trx) + .where("screenshotBucketId", build.compareScreenshotBucketId) + .resultSize(), + BuildShard.query(trx).select("metadata").where("buildId", build.id), + ]); + const valid = shards.every((shard) => + checkIsBucketValidFromMetadata(shard.metadata), + ); await Promise.all([ build .$relatedQuery("compareScreenshotBucket", trx) - .patch({ complete: true, screenshotCount }), + .patch({ complete: true, valid, screenshotCount }), // If the build was marked as partial, then it was obviously an error, we unmark it. build.partial ? Build.query(trx).where("id", build.id).patch({ partial: false }) @@ -95,9 +110,18 @@ async function handleUpdateSingle({ trx, }); - await build - .compareScreenshotBucket!.$query(trx) - .patchAndFetch({ complete: true, screenshotCount }); + const buildMetadata = req.body.metadata ?? null; + + await Promise.all([ + build.compareScreenshotBucket!.$query(trx).patchAndFetch({ + complete: true, + valid: checkIsBucketValidFromMetadata(buildMetadata), + screenshotCount, + }), + buildMetadata + ? build.$clone().$query(trx).patch({ metadata: buildMetadata }) + : null, + ]); }); await buildJob.push(build.id); } diff --git a/apps/backend/src/web/api/v2/util.ts b/apps/backend/src/web/api/v2/util.ts index 0d7248398..04f78bad5 100644 --- a/apps/backend/src/web/api/v2/util.ts +++ b/apps/backend/src/web/api/v2/util.ts @@ -14,6 +14,10 @@ import { ScreenshotMetadata, ScreenshotMetadataJsonSchema, } from "@/database/models/index.js"; +import { + BuildMetadata, + BuildMetadataJsonSchema, +} from "@/database/services/buildMetadata.js"; import { job as githubPullRequestJob } from "@/github-pull-request/job.js"; import { getRedisLock } from "@/util/redis/index.js"; import { SHA1_REGEX_STR, SHA256_REGEX_STR } from "@/web/constants"; @@ -217,6 +221,7 @@ export const validateUpdateRequest = validate({ minimum: 1, nullable: true, }, + metadata: BuildMetadataJsonSchema, }, }, }); @@ -239,6 +244,7 @@ export type UpdateRequest = Request< parallel?: boolean | null; parallelTotal?: number | null; parallelIndex?: number | null; + metadata?: BuildMetadata | null; } > & { authProject?: Project }; @@ -246,17 +252,17 @@ async function createBuild(params: { project: Project; commit: string; branch: string; - buildName?: string | null; - parallel?: { nonce: string } | null; - prNumber?: number | null; - prHeadCommit?: string | null; - referenceCommit?: string | null; - referenceBranch?: string | null; - mode?: BuildMode | null; - ciProvider?: string | null; - argosSdk?: string | null; - runId?: string | null; - runAttempt?: number | null; + buildName: string | null; + parallel: { nonce: string } | null; + prNumber: number | null; + prHeadCommit: string | null; + referenceCommit: string | null; + referenceBranch: string | null; + mode: BuildMode | null; + ciProvider: string | null; + argosSdk: string | null; + runId: string | null; + runAttempt: number | null; }) { const account = await params.project.$relatedQuery("account"); invariant(account, "Account should be fetched"); @@ -329,6 +335,7 @@ async function createBuild(params: { branch: params.branch, projectId: params.project.id, complete: false, + valid: false, mode, });