Copilot commented on code in PR #6685:
URL: https://github.com/apache/texera/pull/6685#discussion_r3626779445
##########
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 as above: asserting the full object shape avoids the `any` cast and
more directly checks the discriminated-union contract of `Validation`.
##########
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:
Since `Validation` is a discriminated union (`isValid: true` vs `isValid:
false`), you can avoid the `ValidationError` cast by asserting the full invalid
object 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" });
+ });
+
+ 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:
This test can also avoid the `ValidationError` cast by asserting the full
returned object. It keeps the intent (merge invalid messages) while staying
type-safe.
##########
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:
`Validation`/`ValidationError` are only used as types in this spec file;
using a type-only import clarifies intent and avoids accidentally treating them
as runtime values.
##########
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:
These assertions can be simplified and made type-safe by asserting the full
returned object shape, avoiding the `any` cast to access `messages` on a valid
result.
##########
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:
Similarly here, asserting the complete invalid object avoids the cast and
directly checks the "last writer wins" behavior for duplicate keys.
--
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]
