You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/08/15 04:55:17 UTC
(texera) branch gh-readonly-queue/main/pr-7685-0635af174364f428fcf6c1282099e621d606265d created (now fbee5c7259)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-7685-0635af174364f428fcf6c1282099e621d606265d
in repository https://gitbox.apache.org/repos/asf/texera.git
at fbee5c7259 test(frontend): cover the remaining branch cases in three workspace services (#7685)
This branch includes the following new commits:
new fbee5c7259 test(frontend): cover the remaining branch cases in three workspace services (#7685)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(texera) 01/01: test(frontend): cover the remaining branch cases in three workspace services (#7685)
Posted by gi...@apache.org.
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 <me...@uci.edu>
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();
+ });
+ });
});