Show earliest running/scheduled attempt. recording: http://shortn/_eGtC6s9TD9 for CL: https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/4112342 Bug:b/254657225 Change-Id: Id352248b2ba9acf1097ae96cba27d36d2274aa36 Reviewed-on: https://chromium-review.googlesource.com/c/infra/gerrit-plugins/buildbucket/+/4113102 Commit-Queue: Joanna Wang <jojwang@chromium.org> Reviewed-by: Gavin Mak <gavinmak@google.com>
diff --git a/web/checks-fetcher.ts b/web/checks-fetcher.ts index 1009344..abee999 100644 --- a/web/checks-fetcher.ts +++ b/web/checks-fetcher.ts
@@ -582,7 +582,7 @@ } /** - * Returns an array of attempts normalized agains ancestors. + * Returns an array of attempts normalized against ancestors. * * If builds have ancestors, we want to normalize their attempts * with their ancestor attempts. @@ -602,37 +602,52 @@ let normalizeComplete = true; const ancestorId: string = - firstAttempt.build.ancestorIds[ - firstAttempt.build.ancestorIds.length - 1 - ].toString(); + firstAttempt.build.ancestorIds[0].toString(); // A build can have several ancestorIds. All ancestors of any build should be // associated with the same change and patchset and therefore present in buildsByID. - // `ancestorIds` is ordered top-to-bottom, making the last value the build's - // immediate parent. + // `ancestorIds` is ordered top-to-bottom, making the first value the build's + // root ancestor. // https://source.chromium.org/chromium/infra/infra/+/main:recipes-py/recipe_proto/go.chromium.org/luci/buildbucket/proto/build.proto;l=321;drc=428eeaebf2e1de58ed1c7cae50daba3384d730cd - const parentBuilder = buildsByID.get(ancestorId)?.builder.builder; - const parentAttempts = attemptsByBuilder[parentBuilder as string]; - if (parentAttempts.length == attempts.length) { + const ancestorBuilder = buildsByID.get(ancestorId)?.builder.builder; + const ancestorAttempts = attemptsByBuilder[ancestorBuilder as string]; + if (ancestorAttempts.length == attempts.length) { return attempts } - const parentAttemptIds = parentAttempts.map( + const ancestorAttemptIds = ancestorAttempts.map( attempt => attempt.build.id ); const normalizedAttempts: (Attempt | undefined)[] = - parentAttemptIds.map(() => undefined); - for (const attempt of attempts) { + ancestorAttemptIds.map(() => undefined); + + let attemptsSlice: undefined | number; + let slicedAttempts = attempts; + for (const [i, attempt] of attempts.entries()) { + const currentAncestorId: string = attempt.build.ancestorIds?.[0] as string; + + // If currentAncestorId is not in buildsByID, then it was removed + // because there are earlier attempts we want to show that are more relevant. + // Any child builds of this ancestor should also not be shown. Do not add + // them to the `normalizedAttempts` and remove them from `attempts`. + if (!buildsByID.has(currentAncestorId)) { + attemptsSlice = i; + break; + } + // We expect all build attempts for any builder have the same ancestor builders. // But config changes might have occured and ancestors may have changes. - const currentAncestorId: string = - attempt.build.ancestorIds?.[attempt.build.ancestorIds.length - 1] as string; - if (buildsByID.get(currentAncestorId)?.builder.builder !== parentBuilder) { + if (buildsByID.get(currentAncestorId)?.builder.builder !== ancestorBuilder) { normalizeComplete = false; break; } - const i = parentAttemptIds.indexOf(currentAncestorId); - normalizedAttempts[i] = attempt; + + const ancestorI = ancestorAttemptIds.indexOf(currentAncestorId); + normalizedAttempts[ancestorI] = attempt; } + if (attemptsSlice !== undefined) { + slicedAttempts = attempts.slice(0, attemptsSlice); + } + if (!normalizeComplete) { return attempts; } @@ -650,9 +665,7 @@ if ( normalizedAttempts[normalizedAttempts.length - 1] === undefined && - attempts[attempts.length - 1].build.status === - BuildStatus.SUCCESS - ) { + slicedAttempts[slicedAttempts.length - 1].build.status === BuildStatus.SUCCESS) { let i = normalizedAttempts.length - 1; while (normalizedAttempts[i] === undefined) { i--; @@ -675,6 +688,27 @@ const runs: CheckRun[] = []; let incompleteTestVariantResults = false; + for (const [builder, builderAttempts] of Object.entries(attemptsByBuilder)) { + let i = builderAttempts.length - 1; + + // For root builders we want to throw away scheduled/started if there are + // earlier attempts still running. + if (!builderAttempts[i].build.ancestorIds && [ + BuildStatus.SCHEDULED, BuildStatus.STARTED].includes(builderAttempts[i].build.status)) { + // Find the index of first attempt that is still started/scheduled. + // Rare edge case: for builders that have completed attempts (b), between two + // started/scheduled attempts (a) and (c), this will return the index of (c). + // (b) is the latest complete run, so we want to show that in gerrit. + // The result is (c) will be what is visible in the Checks summary, but if it is blocked on + // (a), it may not show any updates for awhile. + while (i > 0 && [BuildStatus.SCHEDULED, BuildStatus.STARTED].includes( + builderAttempts[i-1].build.status)) { + i--; + } + attemptsByBuilder[builder] = builderAttempts.slice(0, i + 1); + } + } + const buildsByID: Map<string, Build> = new Map(); for (const builderAttempts of Object.values(attemptsByBuilder)) { for (const attempt of builderAttempts.values()) {
diff --git a/web/test/checks-fetcher_test.ts b/web/test/checks-fetcher_test.ts index 67da992..3e82a5f 100644 --- a/web/test/checks-fetcher_test.ts +++ b/web/test/checks-fetcher_test.ts
@@ -696,7 +696,7 @@ const attemptsAncestors: Attempt[] = [ { build: { - id: '3', + id: '6', status: BuildStatus.FAILURE, builder: {builder: 'ancestors'}, createTime: '', @@ -707,8 +707,41 @@ }, { build: { + id: '5', + status: BuildStatus.FAILURE, + builder: {builder: 'ancestors'}, + createTime: '', + startTime: '', + endTime: '', + }, + isQuickRun: false, + }, + { + build: { + id: '4', + status: BuildStatus.SUCCESS, + builder: {builder: 'ancestors'}, + createTime: '', + startTime: '', + endTime: '', + }, + isQuickRun: false, + }, + { + build: { + id: '3', + status: BuildStatus.STARTED, + builder: {builder: 'ancestors'}, + createTime: '', + startTime: '', + endTime: '', + }, + isQuickRun: false, + }, + { + build: { id: '2', - status: BuildStatus.FAILURE, + status: BuildStatus.STARTED, builder: {builder: 'ancestors'}, createTime: '', startTime: '', @@ -719,7 +752,7 @@ { build: { id: '1', - status: BuildStatus.SUCCESS, + status: BuildStatus.SCHEDULED, builder: {builder: 'ancestors'}, createTime: '', startTime: '', @@ -728,6 +761,45 @@ isQuickRun: false, }, ]; + // We want to hide builds that are children of irrelevant builds. + const attemptsIrr: Attempt[] = [ + { + build: { + id: '101', + status: BuildStatus.SUCCESS, + builder: {builder: 'maybeirr'}, + createTime: '', + startTime: '', + endTime: '', + ancestorIds: ['4', '404'], // Ensure the first ancestorId is used. + }, + isQuickRun: false, + }, + { + build: { + id: '102', + status: BuildStatus.STARTED, + builder: {builder: 'maybeirr'}, + createTime: '', + startTime: '', + endTime: '', + ancestorIds: ['3', '404'], + }, + isQuickRun: false, + }, + { + build: { + id: '103', + status: BuildStatus.SCHEDULED, + builder: {builder: 'maybeirr'}, + createTime: '', + startTime: '', + endTime: '', + ancestorIds: ['2', '404'], + }, + isQuickRun: false, + }, + ]; // We want to hide failures that are no longer relevant or may // have another attempt soon. const attemptsFailures: Attempt[] = [ @@ -739,7 +811,7 @@ createTime: '', startTime: '', endTime: '', - ancestorIds: ['404', '3'], // Ensure the last ancestorId is used. + ancestorIds: ['6', '404'], // Ensure the last ancestorId is used. }, isQuickRun: false, }, @@ -751,7 +823,7 @@ createTime: '', startTime: '', endTime: '', - ancestorIds: ['405', '2'], + ancestorIds: ['5', '405'], }, isQuickRun: false, }, @@ -766,7 +838,7 @@ createTime: '', startTime: '', endTime: '', - ancestorIds: ['404', '2'] + ancestorIds: ['5', '405'] }, isQuickRun: false, }, @@ -782,7 +854,7 @@ createTime: '', startTime: '', endTime: '', - ancestorIds: ['404', '3'], + ancestorIds: ['6', '404'], }, isQuickRun: false, }, @@ -808,6 +880,7 @@ createTime: '', startTime: '', endTime: '', + ancestorIds: ['6'], }, isQuickRun: false, }, @@ -819,36 +892,47 @@ successes: attemptsSuccess, modified: attemptsModified, extra: attemptsExtra, + maybeirr: attemptsIrr, }); - assert.strictEqual(runs.length, 3 + 3 + 2 + 2 + 1); - const runsAncestors = runs.slice(0, 3); + assert.strictEqual(runs.length, 4 + 4 + 2 + 2 + 1 + 4); + const runsAncestors = runs.slice(0, 4); const actualAncestors = runsAncestors.map(run => {return [run.checkName, run.status]}); const expectedAncestors = [['ancestors', RunStatus.COMPLETED], ['ancestors', RunStatus.COMPLETED], - ['ancestors', RunStatus.COMPLETED]]; + ['ancestors', RunStatus.COMPLETED], + ['ancestors', RunStatus.RUNNING]]; assert.deepEqual(expectedAncestors, actualAncestors); - const runsFailures = runs.slice(3, 6); + const runsFailures = runs.slice(4, 8); const actualFailures = runsFailures.map(run => {return [run.checkName, run.status]}); const expectedFailures = [['failures', RunStatus.COMPLETED], - ['failures', RunStatus.COMPLETED], - ['failures', RunStatus.RUNNABLE]]; + ['failures', RunStatus.COMPLETED], + ['failures', RunStatus.RUNNABLE], + ['failures', RunStatus.RUNNABLE]]; assert.deepEqual(expectedFailures, actualFailures); - const runsSuccesses = runs.slice(6, 8); + const runsSuccesses = runs.slice(8, 10); const actualSuccesses = runsSuccesses.map(run => {return [run.checkName, run.status]}); const expectedSuccesses = [['successes', RunStatus.RUNNABLE], ['successes', RunStatus.COMPLETED]]; assert.deepEqual(expectedSuccesses, actualSuccesses); - const runsModified = runs.slice(8, 10); + const runsModified = runs.slice(10, 12); const actualModified = runsModified.map(run => {return [run.checkName, run.status]}); const expectedModified = [['modified', RunStatus.COMPLETED], ['modified', RunStatus.COMPLETED]]; assert.deepEqual(expectedModified, actualModified); - const runExtra = runs[10]; + const runExtra = runs[12]; assert.deepEqual([runExtra.checkName, runExtra.status], ['extra', RunStatus.COMPLETED]); + + const runsIrr = runs.slice(13, 17); + const actualIrr = runsIrr.map(run => {return [run.checkName, run.status]}); + const expectedIrr = [['maybeirr', RunStatus.RUNNABLE], + ['maybeirr', RunStatus.RUNNABLE], + ['maybeirr', RunStatus.COMPLETED], + ['maybeirr', RunStatus.RUNNING]]; + assert.deepEqual(expectedIrr, actualIrr); }); test('convertAttemptsToRuns creates statuses for SCHEDULED and STARTED builds', async () => { @@ -861,7 +945,7 @@ endTime: '', }; const startedBuild: Build = { - builder: {builder: 'foo'}, + builder: {builder: 'foo_2'}, id: '12346', status: BuildStatus.STARTED, createTime: '', @@ -870,10 +954,8 @@ }; const runs = await fetcher.convertAttemptsToRuns(4, 1234, 'repo', 4, { - foo: [ - {isQuickRun: false, build: scheduledBuild}, - {isQuickRun: false, build: startedBuild}, - ], + foo: [{isQuickRun: false, build: scheduledBuild}], + foo_2: [{isQuickRun: false, build: startedBuild}], }); assert.deepEqual(runs.length, 2); assert.deepEqual(runs[0].status, RunStatus.SCHEDULED);