Skip to content

Commit 3baba22

Browse files
Frooodlejbrunton96
andauthored
Mark the pipeline step that actually failed and retire stale test results (#7852)
# Description of Changes `PolicyRun.currentStep` is a 1-based cursor (0 before the run starts). `PipelineBuilder` compared it against the graph's 0-based step index, so the badges landed one node late. Test a corrupt PDF through Rotate → Split: the rotate call is the one that returns 400, but the graph puts a green tick on Rotate and the error control on Split, and the strip reads "Failed after 1 of 2 steps" when no step completed. The same arithmetic is why a node added after a successful test shows as done before it has ever run: run state is derived positionally and nothing retires a result when the chain changes. **Changes** - `stepRunState` converts the cursor to a 0-based index before comparing. - The failed summary reports steps that actually completed, not the cursor. - A test result is cleared when the step chain it ran against is edited, so badges cannot certify a chain that was never executed. **Before:** step 1 fails → step 1 shows done, step 2 shows failed, "Failed after 1 of 2 steps". **After:** step 1 fails → step 1 shows failed, step 2 shows nothing, "Failed after 0 of 2 steps". Three cases added to `PipelineBuilder.test.tsx` covering a first-step failure, a mid-chain run, and editing the chain after a passing test. All three fail on `main` and pass here. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [x] I have performed a self-review of my own code - [x] Every comment I added says something the code does not ([guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/CODE_COMMENTS.md)) - [x] My changes generate no new warnings ### Testing (if applicable) - [x] `npx vitest run src/portal` passes: 90 files, 583 tests - [x] `tsc --noEmit` (proprietary variant), `oxlint --max-warnings=0` and `oxfmt --check` all clean Co-authored-by: James Brunton <jbrunton96@gmail.com>
1 parent 3ab5f55 commit 3baba22

2 files changed

Lines changed: 96 additions & 3 deletions

File tree

frontend/editor/src/portal/views/PipelineBuilder.test.tsx

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,17 @@ const savePipeline = vi.fn();
3434
const deletePipeline = vi.fn();
3535
const triggerPipeline = vi.fn();
3636
const fetchRun = vi.fn();
37+
const runPipelineTest = vi.fn();
38+
const fetchRunOutput = vi.fn();
3739
vi.mock("@portal/api/pipelines", () => ({
3840
fetchPipeline: (id: string) => fetchPipeline(id),
3941
fetchTriggers: () => fetchTriggers(),
4042
savePipeline: (policy: unknown) => savePipeline(policy),
4143
deletePipeline: (id: string) => deletePipeline(id),
4244
triggerPipeline: (id: string) => triggerPipeline(id),
4345
fetchRun: (runId: string) => fetchRun(runId),
46+
runPipelineTest: (...args: unknown[]) => runPipelineTest(...args),
47+
fetchRunOutput: (...args: unknown[]) => fetchRunOutput(...args),
4448
}));
4549

4650
const uploadPipelineAsset = vi.fn();
@@ -331,6 +335,8 @@ describe("PipelineBuilder", () => {
331335
deletePipeline.mockReset();
332336
triggerPipeline.mockReset();
333337
fetchRun.mockReset();
338+
runPipelineTest.mockReset();
339+
runPipelineTest.mockResolvedValue({ runId: "run-test-1" });
334340
fetchSources.mockReset();
335341
fetchPipeline.mockResolvedValue(POLICY);
336342
fetchTriggers.mockResolvedValue([]);
@@ -889,6 +895,76 @@ describe("PipelineBuilder", () => {
889895
).toBeInTheDocument();
890896
});
891897

898+
function twoStepPolicy(): Policy {
899+
return {
900+
...POLICY,
901+
steps: [
902+
{ operation: "/api/v1/misc/compress-pdf", parameters: {} },
903+
{ operation: "/api/v1/misc/extract-images", parameters: {} },
904+
] as unknown as Policy["steps"],
905+
};
906+
}
907+
908+
async function runTestWith(view: Record<string, unknown>) {
909+
fetchRun.mockResolvedValue({
910+
runId: "run-test-1",
911+
stepCount: 2,
912+
outputs: [],
913+
error: null,
914+
...view,
915+
});
916+
renderBuilder("/processor/pipelines/plc-1");
917+
await screen.findByText("portal.pipelines.builder.testRun");
918+
const picker = document.querySelector<HTMLInputElement>(
919+
'input[type="file"][accept="application/pdf"]',
920+
);
921+
if (!picker) throw new Error("test-run file input not rendered");
922+
fireEvent.change(picker, {
923+
target: {
924+
files: [new File(["x"], "in.pdf", { type: "application/pdf" })],
925+
},
926+
});
927+
await waitFor(() => expect(runPipelineTest).toHaveBeenCalledTimes(1));
928+
}
929+
930+
it("blames the step that actually failed, not the one after it", async () => {
931+
fetchPipeline.mockResolvedValue(twoStepPolicy());
932+
933+
await runTestWith({ status: "FAILED", currentStep: 1, error: "boom" });
934+
935+
await screen.findByLabelText("portal.pipelines.graph.showError");
936+
expect(screen.queryByText("portal.pipelines.graph.run.done")).toBeNull();
937+
});
938+
939+
it("marks only the steps that finished before the one still running", async () => {
940+
fetchPipeline.mockResolvedValue(twoStepPolicy());
941+
942+
await runTestWith({ status: "RUNNING", currentStep: 2 });
943+
944+
await waitFor(() =>
945+
expect(
946+
screen.getAllByText("portal.pipelines.graph.run.done"),
947+
).toHaveLength(1),
948+
);
949+
});
950+
951+
it("retires a test result once the chain it ran against changes", async () => {
952+
fetchPipeline.mockResolvedValue(twoStepPolicy());
953+
954+
await runTestWith({ status: "COMPLETED", currentStep: 2 });
955+
await waitFor(() =>
956+
expect(
957+
screen.getAllByText("portal.pipelines.graph.run.done"),
958+
).toHaveLength(2),
959+
);
960+
961+
await addTool("OCR");
962+
963+
await waitFor(() =>
964+
expect(screen.queryByText("portal.pipelines.graph.run.done")).toBeNull(),
965+
);
966+
});
967+
892968
it("uploads a step's supporting file and saves it as an asset binding", async () => {
893969
renderBuilder("/processor/pipelines/new");
894970

frontend/editor/src/portal/views/PipelineBuilder.tsx

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -725,6 +725,18 @@ export function PipelineBuilder() {
725725
}, [seeded, snapshot]);
726726
const dirty = baseline.current !== null && baseline.current !== snapshot;
727727

728+
const stepsSignature = JSON.stringify(stepSnapshot);
729+
const testedStepsSignature = useRef<string | null>(null);
730+
useEffect(() => {
731+
if (
732+
testedStepsSignature.current !== null &&
733+
testedStepsSignature.current !== stepsSignature
734+
) {
735+
testedStepsSignature.current = null;
736+
setTestRun(null);
737+
}
738+
}, [stepsSignature]);
739+
728740
// Each validity condition is defined exactly once here, then consumed both by the graph (which
729741
// flags each end) and by the blocker list below.
730742
const sourceChosen = input.sourceId !== "";
@@ -956,6 +968,7 @@ export function PipelineBuilder() {
956968
if (testing) return;
957969
setTesting(true);
958970
setTestRun(null);
971+
testedStepsSignature.current = stepsSignature;
959972
setRunResult(null);
960973
try {
961974
const { steps: testSteps, assets } = buildTestSteps();
@@ -1167,8 +1180,9 @@ export function PipelineBuilder() {
11671180
// the cursor itself is whatever the run currently is.
11681181
function stepRunState(index: number): GraphStepContent["runState"] {
11691182
if (!testRun) return undefined;
1170-
if (index < testRun.currentStep) return "done";
1171-
if (index > testRun.currentStep) return undefined;
1183+
const activeIndex = testRun.currentStep - 1;
1184+
if (index < activeIndex) return "done";
1185+
if (index > activeIndex) return undefined;
11721186
if (testRun.status === "FAILED") return "failed";
11731187
if (testRun.status === "COMPLETED") return "done";
11741188
return "running";
@@ -1205,7 +1219,10 @@ export function PipelineBuilder() {
12051219
: testRun.status === "COMPLETED"
12061220
? ("completed" as const)
12071221
: ("running" as const),
1208-
completedSteps: testRun.currentStep,
1222+
completedSteps:
1223+
testRun.status === "FAILED"
1224+
? Math.max(0, testRun.currentStep - 1)
1225+
: testRun.currentStep,
12091226
stepCount: testRun.stepCount,
12101227
error: testRun.error,
12111228
outputs: testRun.outputs ?? [],

0 commit comments

Comments
 (0)