This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8604-28594efe6cf7628be81bd9ff8939199417773ecd in repository https://gitbox.apache.org/repos/asf/texera.git
commit c16e152f2f2b7b239a8660b77b9b1e539c8c2236 Author: Suyash Jain <[email protected]> AuthorDate: Sun Sep 20 18:04:05 2026 +0000 test(validation): cover ValidationWorkflowService.combineValidation (#8604) ### What changes were proposed in this PR? `ValidationWorkflowService.combineValidation` is what folds the per-check results (JSON-schema validation and operator-connection validation) into the single verdict the workflow editor acts on: ```ts public static combineValidation(...validations: Validation[]): Validation ``` It had no direct test. This PR adds one, covering the three decisions the implementation actually makes: - **Validity is an AND** across all validations, independent of argument order. - **Messages are merged only from the validations that failed.** The merge is guarded on `validation.isValid` rather than on the `messages` key being absent, so a stray `messages` field on something reporting `isValid: true` is dropped rather than leaking into the combined result. - **A later failure wins** when two failing validations use the same message key, since the merge is a spread in argument order. Two shape details are asserted as well, because call sites depend on them: - The valid branch returns `{ isValid }` only — it omits `messages` entirely rather than returning an empty map, so reading `messages` off a valid result yields `undefined`. - A no-argument call is vacuously valid. No production code is changed; this is a test-only PR. ### Any related issues, documentation, discussions? Closes #6684 ### How was this PR tested? Nine cases were added to `validation-workflow.service.spec.ts` in a new top-level `describe`. `combineValidation` is static, so the block needs no `TestBed` setup. | Case | Asserts | | --- | --- | | no arguments | vacuously valid, `messages` is `undefined` | | all valid | result is exactly `{ isValid: true }` | | one invalid | invalid, carries that validation's messages | | invalid first / invalid last | invalid either way (order independence) | | several invalid | message maps are merged | | key collision | the later failure's message wins | | stray `messages` on a valid validation | dropped from the result | | invalid with an empty message map | invalid, `messages` is `{}` | | any combination | the input validations are not mutated | ``` npx ng test --watch=false --include="**/validation-workflow.service.spec.ts" # Test Files 1 passed (1) # Tests 19 passed (19) <- 10 existing + 9 added here ``` `npx prettier --check` reports no diff on the changed file. ### Was this PR authored or co-authored using generative AI tooling? Yes, partially. I (Suyash Jain) worked on this PR together with Claude Code as a pair-programming assistant. The added specs were run locally against the existing suite before opening this PR. --- .../validation/validation-workflow.service.spec.ts | 92 +++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts b/frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts index a755fa488a..0ccb522afd 100644 --- a/frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts +++ b/frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts @@ -18,7 +18,7 @@ */ import { inject, TestBed } from "@angular/core/testing"; -import { ValidationWorkflowService } from "./validation-workflow.service"; +import { Validation, ValidationError, ValidationWorkflowService } from "./validation-workflow.service"; import { mockPoint, mockResultPredicate, @@ -301,3 +301,93 @@ describe("ValidationWorkflowService", () => { subscription.unsubscribe(); }); }); + +describe("ValidationWorkflowService.combineValidation", () => { + const invalid = (messages: Record<string, string>): Validation => ({ isValid: false, messages }); + + it("should be valid when given no validations at all", () => { + const combined = ValidationWorkflowService.combineValidation(); + + expect(combined.isValid).toBe(true); + // The valid branch returns { isValid } only, so consumers reading `messages` + // off a valid result get undefined rather than an empty object. + expect((combined as ValidationError).messages).toBeUndefined(); + }); + + it("should be valid, with no messages, when every validation is valid", () => { + const combined = ValidationWorkflowService.combineValidation({ isValid: true }, { isValid: true }); + + expect(combined).toEqual({ isValid: true }); + }); + + it("should be invalid and carry the messages when a single validation is invalid", () => { + const combined = ValidationWorkflowService.combineValidation( + { isValid: true }, + invalid({ jsonSchema: "property 'x' is required" }) + ); + + expect(combined).toEqual({ isValid: false, messages: { jsonSchema: "property 'x' is required" } }); + }); + + it("should stay invalid regardless of where the invalid validation sits", () => { + const failure = invalid({ connection: "operator has no input" }); + + expect(ValidationWorkflowService.combineValidation(failure, { isValid: true }).isValid).toBe(false); + expect(ValidationWorkflowService.combineValidation({ isValid: true }, failure).isValid).toBe(false); + }); + + it("should merge the messages of several invalid validations", () => { + const combined = ValidationWorkflowService.combineValidation( + invalid({ jsonSchema: "property 'x' is required" }), + { isValid: true }, + invalid({ connection: "operator has no input" }) + ); + + expect(combined).toEqual({ + isValid: false, + messages: { + jsonSchema: "property 'x' is required", + connection: "operator has no input", + }, + }); + }); + + it("should let a later message win when two invalid validations share a key", () => { + const combined = ValidationWorkflowService.combineValidation( + invalid({ connection: "first" }), + invalid({ connection: "second" }) + ); + + expect(combined).toEqual({ isValid: false, messages: { connection: "second" } }); + }); + + it("should ignore messages attached to a validation that reports itself valid", () => { + // The Validation union gives the valid arm no `messages`, but the merge is + // guarded on isValid rather than on the key being absent, so a stray field + // is dropped instead of leaking into the combined result. + const validWithStrayMessages = { isValid: true, messages: { ignored: "not a real error" } } as Validation; + + const combined = ValidationWorkflowService.combineValidation( + validWithStrayMessages, + invalid({ connection: "operator has no input" }) + ); + + expect(combined).toEqual({ isValid: false, messages: { connection: "operator has no input" } }); + }); + + it("should report invalid with an empty message map when the failing validation has none", () => { + const combined = ValidationWorkflowService.combineValidation(invalid({})); + + expect(combined).toEqual({ isValid: false, messages: {} }); + }); + + it("should not mutate the validations it was given", () => { + const first = invalid({ jsonSchema: "property 'x' is required" }); + const second = invalid({ connection: "operator has no input" }); + + ValidationWorkflowService.combineValidation(first, second); + + expect(first).toEqual({ isValid: false, messages: { jsonSchema: "property 'x' is required" } }); + expect(second).toEqual({ isValid: false, messages: { connection: "operator has no input" } }); + }); +});
