Ma77Ball commented on code in PR #6685:
URL: https://github.com/apache/texera/pull/6685#discussion_r3627120961
##########
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 { ValidationWorkflowService, Validation, ValidationError } from
"./validation-workflow.service";
Review Comment:
Good catch. Switched to `import type { Validation }`. `ValidationError` is
dropped from the imports too, since the refactor below removes the casts that
were its only use.
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts:
##########
@@ -228,3 +228,49 @@ describe("ValidationWorkflowService", () => {
expect(Object.entries(validationWorkflowService.getCurrentWorkflowValidationError().errors).length).toEqual(0);
});
});
+
+describe("ValidationWorkflowService.combineValidation", () => {
+ it("should return valid when no validations are provided", () => {
+ const result = ValidationWorkflowService.combineValidation();
+ expect(result.isValid).toBeTruthy();
+ // a valid result must not carry any messages
+ expect((result as any).messages).toBeUndefined();
Review Comment:
Done. This assertion now checks the full returned object with
`expect(result).toEqual({...})`, so no cast is needed and the valid branch (no
`messages` key) is verified directly.
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts:
##########
@@ -228,3 +228,49 @@ describe("ValidationWorkflowService", () => {
expect(Object.entries(validationWorkflowService.getCurrentWorkflowValidationError().errors).length).toEqual(0);
});
});
+
+describe("ValidationWorkflowService.combineValidation", () => {
+ it("should return valid when no validations are provided", () => {
+ const result = ValidationWorkflowService.combineValidation();
+ expect(result.isValid).toBeTruthy();
+ // a valid result must not carry any messages
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return valid when all provided validations are valid", () => {
+ const a: Validation = { isValid: true };
+ const b: Validation = { isValid: true };
+ const result = ValidationWorkflowService.combineValidation(a, b);
+ expect(result.isValid).toBeTruthy();
+ expect((result as any).messages).toBeUndefined();
Review Comment:
Same here: asserting the full object shape removes the cast and checks the
union contract directly.
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts:
##########
@@ -228,3 +228,49 @@ describe("ValidationWorkflowService", () => {
expect(Object.entries(validationWorkflowService.getCurrentWorkflowValidationError().errors).length).toEqual(0);
});
});
+
+describe("ValidationWorkflowService.combineValidation", () => {
+ it("should return valid when no validations are provided", () => {
+ const result = ValidationWorkflowService.combineValidation();
+ expect(result.isValid).toBeTruthy();
+ // a valid result must not carry any messages
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return valid when all provided validations are valid", () => {
+ const a: Validation = { isValid: true };
+ const b: Validation = { isValid: true };
+ const result = ValidationWorkflowService.combineValidation(a, b);
+ expect(result.isValid).toBeTruthy();
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return invalid and only include messages from invalid
validations", () => {
+ const valid: Validation = { isValid: true };
+ const invalid: Validation = { isValid: false, messages: { inputs:
"requires at least 1 inputs" } };
+ const result = ValidationWorkflowService.combineValidation(valid, invalid);
+ expect(result.isValid).toBeFalsy();
+ // valid validation contributes no messages, so only the invalid one's
message is kept
+ expect((result as ValidationError).messages).toEqual({ inputs: "requires
at least 1 inputs" });
+ });
Review Comment:
Done: replaced the `ValidationError` cast with a full-object assertion of
the invalid shape.
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts:
##########
@@ -228,3 +228,49 @@ describe("ValidationWorkflowService", () => {
expect(Object.entries(validationWorkflowService.getCurrentWorkflowValidationError().errors).length).toEqual(0);
});
});
+
+describe("ValidationWorkflowService.combineValidation", () => {
+ it("should return valid when no validations are provided", () => {
+ const result = ValidationWorkflowService.combineValidation();
+ expect(result.isValid).toBeTruthy();
+ // a valid result must not carry any messages
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return valid when all provided validations are valid", () => {
+ const a: Validation = { isValid: true };
+ const b: Validation = { isValid: true };
+ const result = ValidationWorkflowService.combineValidation(a, b);
+ expect(result.isValid).toBeTruthy();
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return invalid and only include messages from invalid
validations", () => {
+ const valid: Validation = { isValid: true };
+ const invalid: Validation = { isValid: false, messages: { inputs:
"requires at least 1 inputs" } };
+ const result = ValidationWorkflowService.combineValidation(valid, invalid);
+ expect(result.isValid).toBeFalsy();
+ // valid validation contributes no messages, so only the invalid one's
message is kept
+ expect((result as ValidationError).messages).toEqual({ inputs: "requires
at least 1 inputs" });
+ });
+
+ it("should merge messages from multiple invalid validations", () => {
+ const invalidA: Validation = { isValid: false, messages: { inputs:
"missing input" } };
+ const invalidB: Validation = { isValid: false, messages: { required:
"field is required" } };
+ const result = ValidationWorkflowService.combineValidation(invalidA,
invalidB);
+ expect(result.isValid).toBeFalsy();
+ expect((result as ValidationError).messages).toEqual({
+ inputs: "missing input",
+ required: "field is required",
+ });
+ });
Review Comment:
Same fix: the merged-messages case now asserts the complete invalid object,
no cast.
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.spec.ts:
##########
@@ -228,3 +228,49 @@ describe("ValidationWorkflowService", () => {
expect(Object.entries(validationWorkflowService.getCurrentWorkflowValidationError().errors).length).toEqual(0);
});
});
+
+describe("ValidationWorkflowService.combineValidation", () => {
+ it("should return valid when no validations are provided", () => {
+ const result = ValidationWorkflowService.combineValidation();
+ expect(result.isValid).toBeTruthy();
+ // a valid result must not carry any messages
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return valid when all provided validations are valid", () => {
+ const a: Validation = { isValid: true };
+ const b: Validation = { isValid: true };
+ const result = ValidationWorkflowService.combineValidation(a, b);
+ expect(result.isValid).toBeTruthy();
+ expect((result as any).messages).toBeUndefined();
+ });
+
+ it("should return invalid and only include messages from invalid
validations", () => {
+ const valid: Validation = { isValid: true };
+ const invalid: Validation = { isValid: false, messages: { inputs:
"requires at least 1 inputs" } };
+ const result = ValidationWorkflowService.combineValidation(valid, invalid);
+ expect(result.isValid).toBeFalsy();
+ // valid validation contributes no messages, so only the invalid one's
message is kept
+ expect((result as ValidationError).messages).toEqual({ inputs: "requires
at least 1 inputs" });
+ });
+
+ it("should merge messages from multiple invalid validations", () => {
+ const invalidA: Validation = { isValid: false, messages: { inputs:
"missing input" } };
+ const invalidB: Validation = { isValid: false, messages: { required:
"field is required" } };
+ const result = ValidationWorkflowService.combineValidation(invalidA,
invalidB);
+ expect(result.isValid).toBeFalsy();
+ expect((result as ValidationError).messages).toEqual({
+ inputs: "missing input",
+ required: "field is required",
+ });
+ });
+
+ it("should let a later invalid validation override an earlier message with
the same key", () => {
+ const first: Validation = { isValid: false, messages: { inputs: "first
message" } };
+ const second: Validation = { isValid: false, messages: { inputs: "second
message" } };
+ const result = ValidationWorkflowService.combineValidation(first, second);
+ expect(result.isValid).toBeFalsy();
+ // the spread merge keeps the last writer for a duplicate key
+ expect((result as ValidationError).messages).toEqual({ inputs: "second
message" });
+ });
Review Comment:
Done: the duplicate-key ("last writer wins") case now asserts the full
invalid object directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
