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-7685-0635af174364f428fcf6c1282099e621d606265d in repository https://gitbox.apache.org/repos/asf/texera.git
commit fbee5c7259538369c2cc6d0e654d48db25b8c9a2 Author: Meng Wang <[email protected]> AuthorDate: Sat Aug 15 04:54:55 2026 +0000 test(frontend): cover the remaining branch cases in three workspace services (#7685) ### What changes were proposed in this PR? Covers the missing branch cases in the three workspace services. No production code was changed. **`UiUdfParametersParserService`** (+9) — the guards for degenerate sources: empty and whitespace-only input, a class with no body, an `open()` with no body, a single-statement `open()` (the trailing-separator ternary's empty arm), a declaration on a final line with no trailing newline, a call not reached through `self`, and a call with a positional argument after a named one. Also the identifier rules: punctuation-only, digit-leading, and Python-keyword names. **`ValidationWorkflowService`** (+3) — a stale operator id, an operator type with no schema, and the workflow validation error stream. **`SharedModelChangeHandler`** (+3) — the spec's 29 existing tests all originate locally, so `transaction.local === false` was never exercised. A second `Y.Doc` now stands in for a peer: mutating it and syncing the diff back produces a genuinely remote transaction (which `yDoc.transact` cannot fake). One test adds an operator remotely, another applies a remote property change and asserts the local-only awareness update does *not* run. Three of the cases the issue lists turned out not to be reachable through the public API; the tests assert the behaviour that actually happens instead, with a comment at each site: - The parser's `|| "parameter"` fallback cannot run: `\W` characters are each replaced with `_`, so a punctuation-only name yields `___`, and an empty name is rejected earlier by `computeParameterInsertion`. - `ValidationWorkflowService`'s `operator === undefined` guards are shadowed by the graph's own lookup, which throws `operator <id> does not exist` first. - Its `operatorSchema === undefined` guards are unreachable because `addOperator` rejects an unknown operator type at insertion time — so the test asserts that rejection. ### Any related issues, documentation, discussions? Closes #7683 ### How was this PR tested? Unit tests, run locally in `frontend/` (all green; failure paths were verified by breaking assertions to confirm the suites go red): ``` ng test --watch=false --include .../ui-udf-parameters-parser.service.spec.ts # 38 passed ng test --watch=false --include .../validation-workflow.service.spec.ts # 10 passed ng test --watch=false --include .../shared-model-change-handler.spec.ts # 31 passed prettier --write <specs> # clean eslint <specs> # clean ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8 [1M context]) --- .../ui-udf-parameters-parser.service.spec.ts | 83 ++++++++++++++++++++++ .../validation/validation-workflow.service.spec.ts | 36 ++++++++++ .../model/shared-model-change-handler.spec.ts | 64 +++++++++++++++++ 3 files changed, 183 insertions(+) diff --git a/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-parser.service.spec.ts b/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-parser.service.spec.ts index 66a808c108..75cf20648e 100644 --- a/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-parser.service.spec.ts +++ b/frontend/src/app/workspace/service/code-editor/ui-udf-parameters-parser.service.spec.ts @@ -375,6 +375,89 @@ describe("UiUdfParametersParserService.computeParameterInsertion", () => { }); }); +// The parser's conditionals are mostly guards for degenerate sources: empty input, a +// method with no body, a single-statement open(), a name that sanitizes away, and a +// declaration on the last line. These drive the arm each guard exists for. +describe("UiUdfParametersParserService degenerate sources", () => { + let service: UiUdfParametersParserService; + + beforeEach(() => { + service = new UiUdfParametersParserService(); + }); + + it("should return no parameters for empty or whitespace-only source", () => { + expect(service.parse("")).toEqual([]); + expect(service.parse(" \n\t\n")).toEqual([]); + }); + + it("should return no parameters when the class has no body", () => { + expect(service.parse("class ProcessTupleOperator(UDFOperatorV2):")).toEqual([]); + }); + + it("should return no parameters when open() has no body", () => { + expect(service.parse(pythonLines("class ProcessTupleOperator(UDFOperatorV2):", " def open(self):"))).toEqual([]); + }); + + it("should insert into an open() that holds a single statement", () => { + // statements.length === 1, so the trailing-separator ternary takes its empty arm + const code = pythonLines("class ProcessTupleOperator(UDFOperatorV2):", " def open(self):", " pass"); + + const updated = insertParameter(service, code, "threshold"); + + expect(service.parse(updated).map(p => p.attribute.attributeName)).toEqual(["threshold"]); + }); + + it("should sanitize a punctuation-only name into a usable identifier", () => { + // \W characters each become "_", so the name still yields a valid identifier; + // the `|| "parameter"` fallback is unreachable because an empty name is rejected earlier + const updated = insertParameter(service, PASS_ONLY_OPEN, "!!!"); + + expect(updated).toContain("self.___ = "); + expect(updated).toContain('name="!!!"'); + }); + + it("should prefix a name that starts with a digit", () => { + const updated = insertParameter(service, PASS_ONLY_OPEN, "1st"); + + expect(updated).toContain("self._1st = "); + }); + + it("should suffix a name that collides with a Python keyword", () => { + const updated = insertParameter(service, PASS_ONLY_OPEN, "class"); + + expect(updated).toContain("self.class_ = "); + }); + + it("should handle a declaration on a final line with no trailing newline", () => { + // lineEnd() takes its `newline === -1` arm when the statement ends the file + const code = "class ProcessTupleOperator(UDFOperatorV2):\n def open(self):\n pass"; + + const updated = insertParameter(service, code, "threshold"); + + expect(service.parse(updated).map(p => p.attribute.attributeName)).toEqual(["threshold"]); + }); + + it("should ignore a UiParameter call that is not reached through self", () => { + const code = pythonLines( + "class ProcessTupleOperator(UDFOperatorV2):", + " def open(self):", + ' other.UiParameter(name="x", type=AttributeType.DOUBLE)' + ); + + expect(service.parse(code)).toEqual([]); + }); + + it("should ignore a UiParameter call with a positional argument after a named one", () => { + const code = pythonLines( + "class ProcessTupleOperator(UDFOperatorV2):", + " def open(self):", + ' self.x = self.UiParameter(name="x", AttributeType.DOUBLE).value' + ); + + expect(service.parse(code)).toEqual([]); + }); +}); + function insertParameter( service: UiUdfParametersParserService, code: string, 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 3fe5944c14..a755fa488a 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 @@ -264,4 +264,40 @@ describe("ValidationWorkflowService", () => { expect(validation.messages["inputs"]).toContain("requires 1 input, has 2"); } }); + + // A stale operator id surfaces as an explicit error rather than a downstream undefined + // dereference. The graph's own lookup rejects it before the service's guards are reached, + // so that is the message asserted here. + it("should throw for an operator id that is not in the graph", () => { + expect(() => validationWorkflowService.validateOperator("no-such-operator")).toThrowError( + "operator no-such-operator does not exist" + ); + }); + + // The service's own `operatorSchema === undefined` guards are not reachable through the public + // API: the graph rejects an unknown operator type at insertion time, so a schema-less operator + // never makes it in. + it("should reject an operator whose type has no schema at insertion time", () => { + const unknownTypeOperator = { + ...mockScanPredicate, + operatorID: "unknown-type-operator", + operatorType: "NoSuchOperatorType", + }; + + expect(() => workflowActionservice.addOperator(unknownTypeOperator, mockPoint)).toThrowError( + "operator type NoSuchOperatorType is invalid" + ); + }); + + it("should expose the workflow validation error stream", () => { + const emissions: unknown[] = []; + const subscription = validationWorkflowService + .getWorkflowValidationErrorStream() + .subscribe(value => emissions.push(value)); + + workflowActionservice.addOperator(mockScanPredicate, mockPoint); + + expect(emissions.length).toBeGreaterThan(0); + subscription.unsubscribe(); + }); }); diff --git a/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts index af7ad659d2..c363a9e456 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts @@ -19,6 +19,7 @@ import { TestBed } from "@angular/core/testing"; import * as joint from "jointjs"; +import * as Y from "yjs"; import { OperatorMetadataService } from "../../operator-metadata/operator-metadata.service"; import { StubOperatorMetadataService } from "../../operator-metadata/stub-operator-metadata.service"; import { JointUIService } from "../../joint-ui/joint-ui.service"; @@ -542,4 +543,67 @@ describe("SharedModelChangeHandler", () => { sub.unsubscribe(); }); }); + // Every change so far originated locally. A change that arrives from a peer runs the same + // observers with `transaction.local === false` — the arm that decides whether local awareness + // and undo state are touched. A second Y.Doc stands in for the peer: mutating it and syncing + // the diff back produces a genuinely remote transaction, which `yDoc.transact` cannot fake. + describe("remote changes", () => { + /** Runs `mutate` on a peer document and syncs the result in as a remote update. */ + function applyAsRemote(mutate: (peerDoc: Y.Doc) => void): void { + const localDoc = texeraGraph.sharedModel.yDoc; + const peerDoc = new Y.Doc(); + try { + Y.applyUpdate(peerDoc, Y.encodeStateAsUpdate(localDoc)); + mutate(peerDoc); + // Send back only what the local doc is missing, as a real peer would. + Y.applyUpdate(localDoc, Y.encodeStateAsUpdate(peerDoc, Y.encodeStateVector(localDoc))); + } finally { + peerDoc.destroy(); + } + } + + it("adds an operator that arrives from another client", () => { + let added: OperatorPredicate | undefined; + const sub = texeraGraph.getOperatorAddStream().subscribe(o => (added = o)); + + applyAsRemote(peerDoc => + peerDoc.transact(() => { + peerDoc.getMap("operatorIDMap").set(mockScanPredicate.operatorID, createYTypeFromObject(mockScanPredicate)); + peerDoc.getMap("elementPositionMap").set(mockScanPredicate.operatorID, mockPoint); + }) + ); + + expect(added?.operatorID).toBe(mockScanPredicate.operatorID); + expect(jointGraph.getCell(mockScanPredicate.operatorID)).toBeTruthy(); + sub.unsubscribe(); + }); + + it("applies a remote property change without updating local awareness", () => { + addOperatorWithPosition(mockScanPredicate); + // The awareness update needs both a local transaction and this operator being the one + // under edit. Setting the field directly on the awareness the handler reads leaves + // `transaction.local` as the only thing that can keep the call away, so the assertion + // below cannot pass vacuously. (updateSharedModelAwareness does not populate it here: + // with no provider connected its local state stays empty.) + texeraGraph.sharedModel.awareness.setLocalStateField("currentlyEditing", mockScanPredicate.operatorID); + const awarenessSpy = vi.spyOn(texeraGraph, "updateSharedModelAwareness"); + let changed = false; + const sub = texeraGraph.getOperatorPropertyChangeStream().subscribe(() => (changed = true)); + + applyAsRemote(peerDoc => + peerDoc.transact(() => { + const operator = peerDoc.getMap("operatorIDMap").get(mockScanPredicate.operatorID) as Y.Map<unknown>; + (operator.get("operatorProperties") as Y.Map<unknown>).set("tableName", "from-peer"); + }) + ); + + expect(changed).toBe(true); + expect(texeraGraph.getOperator(mockScanPredicate.operatorID).operatorProperties).toEqual({ + tableName: "from-peer", + }); + // the awareness update is the local-only arm of onOperatorPropertyChanged + expect(awarenessSpy).not.toHaveBeenCalled(); + sub.unsubscribe(); + }); + }); });
