diff --git a/packages/steps/src/BuildStep.ts b/packages/steps/src/BuildStep.ts index 9c0dac2e9c..7b09b9ae75 100644 --- a/packages/steps/src/BuildStep.ts +++ b/packages/steps/src/BuildStep.ts @@ -333,15 +333,15 @@ export class BuildStep extends BuildStepOutputAccessor { ); } - public shouldExecuteStep(): boolean { + public shouldExecuteStep({ runByDefault }: { runByDefault: boolean }): boolean { if ( this.compositeFunctionScope && - !this.compositeFunctionScope.isActive(evaluateIfConditionExpression) + !this.compositeFunctionScope.isActive(evaluateIfConditionExpression, runByDefault) ) { return false; } if (!this.ifCondition) { - return !this.ctx.global.hasAnyPreviousStepFailed; + return runByDefault; } return this.evaluateIfCondition(this.ifCondition, { scope: this.compositeFunctionScope, diff --git a/packages/steps/src/BuildStepCompositeFunctionScope.ts b/packages/steps/src/BuildStepCompositeFunctionScope.ts index 0ebc905617..6ec7e40026 100644 --- a/packages/steps/src/BuildStepCompositeFunctionScope.ts +++ b/packages/steps/src/BuildStepCompositeFunctionScope.ts @@ -73,18 +73,18 @@ export class BuildStepCompositeFunctionScope { * Memoized: global status can change mid-expansion, and re-evaluating would flip a passed * success() gate and skip remaining always()/failure() inner steps. */ - public isActive(evaluate: EvaluateIfExpression): boolean { - if (this.parent && !this.parent.isActive(evaluate)) { + public isActive(evaluate: EvaluateIfExpression, runByDefault: boolean): boolean { + if (this.parent && !this.parent.isActive(evaluate, runByDefault)) { return false; } - this.cachedIsActive ??= this.evaluateCallIfCondition(evaluate); + this.cachedIsActive ??= this.evaluateCallIfCondition(evaluate, runByDefault); return this.cachedIsActive; } // Call-site if uses caller env/inputs/steps, not expanded inner steps. - private evaluateCallIfCondition(evaluate: EvaluateIfExpression): boolean { + private evaluateCallIfCondition(evaluate: EvaluateIfExpression, runByDefault: boolean): boolean { if (!this.ifCondition) { - return !this.ctx.hasAnyPreviousStepFailed; + return runByDefault; } const callerBase: JobInterpolationContext = { ...this.ctx.getInterpolationContext(), diff --git a/packages/steps/src/BuildWorkflow.ts b/packages/steps/src/BuildWorkflow.ts index e712fd6809..f7713bd8d7 100644 --- a/packages/steps/src/BuildWorkflow.ts +++ b/packages/steps/src/BuildWorkflow.ts @@ -76,7 +76,9 @@ export class BuildWorkflow { // occurrence; an anchor's `if:` cannot see its before-hooks' outputs. let shouldExecuteStep = false; try { - shouldExecuteStep = step.shouldExecuteStep(); + shouldExecuteStep = step.shouldExecuteStep({ + runByDefault: !this.ctx.hasAnyPreviousStepFailed, + }); } catch (err: any) { logConditionEvaluationError( step.ctx.logger, @@ -165,6 +167,7 @@ export class BuildWorkflow { * failures predating the call are ignored — "runs iff the anchor runs". * - `after`: runs unconditionally — past the anchor's own failure AND past an * earlier after-entry's failure. + * Passed as `runByDefault` so composite scopes share the same missing-`if:` rule. * A user `if:` is always evaluated against the real global context, so * `failure()` / `success()` keep their global meaning on both sides. */ @@ -221,28 +224,22 @@ export async function executeHookStepsAsync( let entryFailed = false; for (const step of entry.steps) { + // before skips on in-sequence failure; an entry with a passed if: only skips + // on within-entry failure; after always runs. + const runByDefault = + options.timing === 'after' || (entryHasExplicitCondition ? !entryFailed : !failedLocally); let shouldExecuteStep = false; - if (step.ifCondition) { - try { - shouldExecuteStep = step.shouldExecuteStep(); - } catch (err) { - logConditionEvaluationError( - step.ctx.logger, - err, - `step "${step.displayName}"`, - step.ifCondition - ); - recordFailure(err); - entryFailed = true; - } - } else { - // Before-side: a no-`if:` step runs unless this hook sequence has - // already failed. An entry whose explicit condition evaluated true - // behaves like a single step whose if: passed — the entry's no-`if:` - // steps ignore failures from EARLIER entries (only within-entry - // failures skip them). - shouldExecuteStep = - options.timing === 'after' || (entryHasExplicitCondition ? !entryFailed : !failedLocally); + try { + shouldExecuteStep = step.shouldExecuteStep({ runByDefault }); + } catch (err) { + logConditionEvaluationError( + step.ctx.logger, + err, + `step "${step.displayName}"`, + step.ifCondition + ); + recordFailure(err); + entryFailed = true; } if (!shouldExecuteStep) { step.skip(); diff --git a/packages/steps/src/__tests__/BuildStep-test.ts b/packages/steps/src/__tests__/BuildStep-test.ts index 75bf0df5e8..352214e685 100644 --- a/packages/steps/src/__tests__/BuildStep-test.ts +++ b/packages/steps/src/__tests__/BuildStep-test.ts @@ -1179,7 +1179,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { ], }); - expect(step.shouldExecuteStep()).toBe(false); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(false); }); it('returns true when if condition is always and previous steps failed', () => { @@ -1191,7 +1191,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition: '${ always() }', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns true when if condition is always and previous steps have not failed', () => { @@ -1202,7 +1202,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition: '${ always() }', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns false when if condition is success and previous steps failed', () => { @@ -1214,7 +1214,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition: '${ success() }', }); - expect(step.shouldExecuteStep()).toBe(false); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(false); }); it('returns true when a dynamic expression matches', () => { @@ -1231,7 +1231,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { }, ifCondition: '${ env.NODE_ENV === "production" && env.LOCAL_ENV === "true" }', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('can use the general interpolation context', () => { @@ -1245,7 +1245,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition: 'fromJSON(env.CONFIG_JSON).foo == "bar"', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns true when a simplified dynamic expression matches', () => { @@ -1259,7 +1259,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { }, ifCondition: "env.NODE_ENV === 'production'", }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns true when an input matches', () => { @@ -1282,7 +1282,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { ], ifCondition: 'inputs.foo1 === "bar"', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns true when an eas value matches', () => { @@ -1293,7 +1293,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition: 'eas.runtimePlatform === "linux"', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns true when if condition is success and previous steps have not failed', () => { @@ -1304,7 +1304,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition: '${ success() }', }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); }); it('returns true when if condition is failure and previous steps failed', () => { @@ -1317,7 +1317,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition, }); - expect(step.shouldExecuteStep()).toBe(true); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true); } }); @@ -1330,7 +1330,31 @@ describe(BuildStep.prototype.shouldExecuteStep, () => { command: 'echo 123', ifCondition, }); - expect(step.shouldExecuteStep()).toBe(false); + expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(false); } }); + + it('resolves a missing if condition with the provided run-by-default policy', () => { + const ctx = createGlobalContextMock(); + const step = new BuildStep(ctx, { + id: 'test1', + displayName: 'Test 1', + command: 'echo 123', + }); + expect(step.shouldExecuteStep({ runByDefault: false })).toBe(false); + ctx.markAsFailed(); + expect(step.shouldExecuteStep({ runByDefault: true })).toBe(true); + }); + + it('ignores the run-by-default value when an if condition is present', () => { + const ctx = createGlobalContextMock(); + ctx.markAsFailed(); + const step = new BuildStep(ctx, { + id: 'test1', + displayName: 'Test 1', + command: 'echo 123', + ifCondition: '${ success() }', + }); + expect(step.shouldExecuteStep({ runByDefault: true })).toBe(false); + }); }); diff --git a/packages/steps/src/__tests__/BuildStepCompositeFunctionScope-test.ts b/packages/steps/src/__tests__/BuildStepCompositeFunctionScope-test.ts index 54055e9431..daeb3079fe 100644 --- a/packages/steps/src/__tests__/BuildStepCompositeFunctionScope-test.ts +++ b/packages/steps/src/__tests__/BuildStepCompositeFunctionScope-test.ts @@ -53,4 +53,21 @@ describe(BuildStepCompositeFunctionScope, () => { interpolateJobContext({ target: '${{ steps.checkout.outputs.sha }}', context }) ).toBeUndefined(); }); + + describe('isActive', () => { + const neverEvaluate = (): boolean => { + throw new Error('a call without an if: must not evaluate an expression'); + }; + + it('resolves a call without an if: with the provided run-by-default value', () => { + expect(makeScope().isActive(neverEvaluate, false)).toBe(false); + expect(makeScope().isActive(neverEvaluate, true)).toBe(true); + }); + + it('memoizes the first evaluation; a later policy flip cannot re-gate the call', () => { + const scope = makeScope(); + expect(scope.isActive(neverEvaluate, true)).toBe(true); + expect(scope.isActive(neverEvaluate, false)).toBe(true); + }); + }); }); diff --git a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts index ca9a467338..d700aa668e 100644 --- a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts +++ b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts @@ -455,6 +455,53 @@ describe('BuildWorkflow hook execution', () => { ]); }); + it('within a single after entry, a no-if step following a failed sibling still runs', async () => { + const failingChild = recordingFunction('failing-child', { + failWith: new Error('child failed'), + }); + const okChild = recordingFunction('ok-child'); + const group = new BuildFunctionGroup({ + namespace: 'test', + id: 'group', + createBuildStepsFromFunctionGroupCall: globalCtx => [ + failingChild.createBuildStepFromFunctionCall(globalCtx, { id: 'failing-child' }), + okChild.createBuildStepFromFunctionCall(globalCtx, { id: 'ok-child' }), + ], + }); + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { after_install_node_modules: [{ uses: 'test/group' }] }, + externalFunctions: [anchorFunction(), failingChild, okChild], + externalFunctionGroups: [group], + }); + await expect(workflow.executeAsync()).rejects.toThrow('child failed'); + expect(executionLog).toEqual(['anchor', 'failing-child', 'ok-child']); + }); + + it('within a before entry whose if: passed, a failed sibling skips later no-if siblings', async () => { + const failingChild = recordingFunction('failing-child', { + failWith: new Error('child failed'), + }); + const okChild = recordingFunction('ok-child'); + const group = new BuildFunctionGroup({ + namespace: 'test', + id: 'group', + createBuildStepsFromFunctionGroupCall: globalCtx => [ + failingChild.createBuildStepFromFunctionCall(globalCtx, { id: 'failing-child' }), + okChild.createBuildStepFromFunctionCall(globalCtx, { id: 'ok-child' }), + ], + }); + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { before_install_node_modules: [{ uses: 'test/group', if: '${{ always() }}' }] }, + externalFunctions: [anchorFunction(), failingChild, okChild], + externalFunctionGroups: [group], + }); + await expect(workflow.executeAsync()).rejects.toThrow('child failed'); + expect(executionLog).toEqual(['failing-child']); + expect(hookStepStatuses(workflow)['ok-child']).toBe(BuildStepStatus.SKIPPED); + }); + it('an entry-level condition evaluation error fails the hook (and the job), not silently ignored', async () => { const { group, functions } = createGroup('group', ['child-one']); const workflow = await parseAsync({ diff --git a/packages/steps/src/__tests__/BuildWorkflow-test.ts b/packages/steps/src/__tests__/BuildWorkflow-test.ts index deff0bc1ab..fa6e7ea0f2 100644 --- a/packages/steps/src/__tests__/BuildWorkflow-test.ts +++ b/packages/steps/src/__tests__/BuildWorkflow-test.ts @@ -1,4 +1,4 @@ -import { instance, mock, verify, when } from 'ts-mockito'; +import { anything, instance, mock, verify, when } from 'ts-mockito'; import { createGlobalContextMock } from './utils/context'; import { BuildRuntimePlatform } from '../BuildRuntimePlatform'; @@ -13,10 +13,10 @@ describe(BuildWorkflow, () => { const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); const mockBuildStep4 = mock(); - when(mockBuildStep4.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep3.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep4.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); const buildSteps: BuildStep[] = [ instance(mockBuildStep1), @@ -38,9 +38,9 @@ describe(BuildWorkflow, () => { const mockBuildStep1 = mock(); const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); - when(mockBuildStep3.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); const buildSteps: BuildStep[] = [ instance(mockBuildStep1), @@ -62,10 +62,10 @@ describe(BuildWorkflow, () => { const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); const mockBuildStep4 = mock(); - when(mockBuildStep4.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep3.shouldExecuteStep()).thenReturn(false); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(false); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep4.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(false); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(false); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); const buildSteps: BuildStep[] = [ instance(mockBuildStep1), @@ -88,9 +88,9 @@ describe(BuildWorkflow, () => { const mockBuildStep1 = mock(); const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); - when(mockBuildStep3.shouldExecuteStep()).thenReturn(false); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(false); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(false); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(false); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep1.executeAsync()).thenReject(new Error('Step 1 failed')); const buildSteps: BuildStep[] = [ @@ -112,9 +112,9 @@ describe(BuildWorkflow, () => { const mockBuildStep1 = mock(); const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); - when(mockBuildStep3.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep1.executeAsync()).thenReject(new Error('Step 1 failed')); const buildSteps: BuildStep[] = [ @@ -136,9 +136,9 @@ describe(BuildWorkflow, () => { const mockBuildStep1 = mock(); const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); - when(mockBuildStep3.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(true); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(true); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep1.executeAsync()).thenReject(new Error('Step 1 failed')); when(mockBuildStep2.executeAsync()).thenReject(new Error('Step 2 failed')); when(mockBuildStep3.executeAsync()).thenReject(new Error('Step 3 failed')); @@ -175,7 +175,7 @@ describe(BuildWorkflow, () => { it('collects metrics for steps with __metricsId', async () => { const mockBuildStep = mock(); - when(mockBuildStep.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep.executeAsync()).thenResolve(); when(mockBuildStep.__metricsId).thenReturn('test-step-metrics'); @@ -198,7 +198,7 @@ describe(BuildWorkflow, () => { it('does not collect metrics for steps without __metricsId', async () => { const mockBuildStep = mock(); - when(mockBuildStep.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep.executeAsync()).thenResolve(); when(mockBuildStep.__metricsId).thenReturn(undefined); @@ -213,7 +213,7 @@ describe(BuildWorkflow, () => { it('collects failed result when step throws', async () => { const mockBuildStep = mock(); - when(mockBuildStep.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep.executeAsync()).thenReject(new Error('Step failed')); when(mockBuildStep.__metricsId).thenReturn('failing-step-metrics'); @@ -236,7 +236,7 @@ describe(BuildWorkflow, () => { it('does not collect metrics when step is skipped', async () => { const mockBuildStep = mock(); - when(mockBuildStep.shouldExecuteStep()).thenReturn(false); + when(mockBuildStep.shouldExecuteStep(anything())).thenReturn(false); when(mockBuildStep.__metricsId).thenReturn('skipped-step-metrics'); const buildSteps: BuildStep[] = [instance(mockBuildStep)]; @@ -252,7 +252,7 @@ describe(BuildWorkflow, () => { it('collects metrics with darwin platform when runtimePlatform is darwin', async () => { const mockBuildStep = mock(); - when(mockBuildStep.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep.executeAsync()).thenResolve(); when(mockBuildStep.__metricsId).thenReturn('darwin-step'); @@ -273,15 +273,15 @@ describe(BuildWorkflow, () => { const mockBuildStep2 = mock(); const mockBuildStep3 = mock(); - when(mockBuildStep1.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep1.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep1.executeAsync()).thenResolve(); when(mockBuildStep1.__metricsId).thenReturn('step-1'); - when(mockBuildStep2.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep2.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep2.executeAsync()).thenResolve(); when(mockBuildStep2.__metricsId).thenReturn(undefined); // No metricsId - when(mockBuildStep3.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep3.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep3.executeAsync()).thenResolve(); when(mockBuildStep3.__metricsId).thenReturn('step-3'); @@ -304,7 +304,7 @@ describe(BuildWorkflow, () => { it('does not throw when reportStepMetric is not provided on the provider', async () => { const mockBuildStep = mock(); - when(mockBuildStep.shouldExecuteStep()).thenReturn(true); + when(mockBuildStep.shouldExecuteStep(anything())).thenReturn(true); when(mockBuildStep.__metricsId).thenReturn('no-callback-test'); const buildSteps: BuildStep[] = [instance(mockBuildStep)];