mengw15 commented on code in PR #7908:
URL: https://github.com/apache/texera/pull/7908#discussion_r3842311575
##########
frontend/src/app/workspace/service/agent/agent.service.spec.ts:
##########
@@ -965,4 +965,339 @@ describe("AgentService", () => {
expect(target).toEqual({ agentId: "agent-1", messageId: "m1", stepId: 4
});
});
});
+
+ //
---------------------------------------------------------------------------
+ // State accessors: the untracked side of each (an id with no tracking entry)
+ //
---------------------------------------------------------------------------
+ describe("state accessors without tracking", () => {
+ it("getAgentState returns the tracked value, or UNAVAILABLE for an unknown
id", () => {
+ // Subscribing to the observable getter is what creates the tracking
entry.
+ service.getAgentStateObservable("agent-1").subscribe();
+ (service as
any).agentStateTracking.get("agent-1").stateSubject.next(AgentState.GENERATING);
+
+ let tracked: AgentState | undefined;
+ service.getAgentState("agent-1").subscribe(s => (tracked = s));
+ expect(tracked).toBe(AgentState.GENERATING);
+
+ let unknown: AgentState | undefined;
+ service.getAgentState("ghost").subscribe(s => (unknown = s));
+ expect(unknown).toBe(AgentState.UNAVAILABLE);
+ });
+
+ it("isAgentConnected is false for an unavailable/unknown agent and true
otherwise", () => {
+ let unknown: boolean | undefined;
+ service.isAgentConnected("ghost").subscribe(v => (unknown = v));
+ expect(unknown).toBe(false);
+
+ service.getAgentStateObservable("agent-1").subscribe();
+ (service as
any).agentStateTracking.get("agent-1").stateSubject.next(AgentState.AVAILABLE);
+ let connected: boolean | undefined;
+ service.isAgentConnected("agent-1").subscribe(v => (connected = v));
+ expect(connected).toBe(true);
+ });
+
+ it("getHeadId / getHeadIdObservable return the tracked HEAD, or null when
untracked", () => {
+ const emitted: (string | null)[] = [];
+ service.getHeadIdObservable("agent-1").subscribe(v => emitted.push(v));
+ (service as
any).agentStateTracking.get("agent-1").headIdSubject.next("m1-2");
+
+ expect(emitted).toEqual([null, "m1-2"]);
+ expect(service.getHeadId("agent-1")).toBe("m1-2");
+ expect(service.getHeadId("ghost")).toBeNull();
+ });
+
+ it("getVisibleSteps returns the tracked snapshot, or [] when untracked",
() => {
+ service.getReActStepsObservable("agent-1").subscribe();
+ const step = { messageId: "m1", stepId: 0 } as unknown as ReActStep;
+ (service as
any).agentStateTracking.get("agent-1").reActStepsSubject.next([step]);
+
+ expect(service.getVisibleSteps("agent-1")).toEqual([step]);
+ expect(service.getVisibleSteps("ghost")).toEqual([]);
+ });
+
+ it("getWorkflowObservable emits the tracked workflow, or null when
untracked", () => {
+ service.getAgentStateObservable("agent-1").subscribe();
+ (service as
any).agentStateTracking.get("agent-1").workflowSubject.next(stubWorkflow);
+ let tracked: Workflow | null | undefined;
+ service.getWorkflowObservable("agent-1").subscribe(w => (tracked = w));
+ expect(tracked).toBe(stubWorkflow);
+
+ let untracked: Workflow | null | undefined = stubWorkflow;
+ service.getWorkflowObservable("ghost").subscribe(w => (untracked = w));
+ expect(untracked).toBeNull();
+ });
+
+ it("getAgentWorkflowId walks the delegate chain: missing agent, no
delegate, full", () => {
+ expect(service.getAgentWorkflowId("ghost")).toBeUndefined();
+
+ (service as any).agents.set("no-delegate", { id: "no-delegate", name:
"x" });
+ expect(service.getAgentWorkflowId("no-delegate")).toBeUndefined();
+
+ seedAgent("with-wf", 42);
+ expect(service.getAgentWorkflowId("with-wf")).toBe(42);
+ });
+
+ it("getAgentCount reports the number of registered agents", () => {
+ let empty: number | undefined;
+ service.getAgentCount().subscribe(c => (empty = c));
+ expect(empty).toBe(0);
+
+ seedAgent("agent-1");
+ seedAgent("agent-2");
+ let count: number | undefined;
+ service.getAgentCount().subscribe(c => (count = c));
+ expect(count).toBe(2);
+ });
+ });
+
+ describe("setHoveredMessage guards", () => {
+ it("emits empty arrays for a non-null step that carries no operator
access", () => {
+ let latest:
+ | { viewedOperatorIds: string[]; addedOperatorIds: string[];
modifiedOperatorIds: string[] }
+ | undefined;
+ service.getHoveredMessageOperatorsObservable("agent-1").subscribe(v =>
(latest = v));
+
+ // A real step with no operatorAccess map takes the else-branch,
distinct from
+ // the null-step case already covered above.
+ service.setHoveredMessage("agent-1", { messageId: "m1" } as unknown as
ReActStep);
+
+ expect(latest).toEqual({ viewedOperatorIds: [], addedOperatorIds: [],
modifiedOperatorIds: [] });
+ });
+
+ it("is a no-op when the agent has no tracking", () => {
+ // No tracking entry exists for this id, so the method must return
without throwing.
+ expect(() => service.setHoveredMessage("ghost", { messageId: "m1" } as
unknown as ReActStep)).not.toThrow();
+ });
+ });
+
+ describe("getReActStepsByOperatorAccess without operator access", () => {
+ it("skips steps whose operatorAccess is absent", () => {
+ let result: { viewedBy: ReActStep[]; modifiedBy: ReActStep[] } |
undefined;
+ service.getReActStepsByOperatorAccess("agent-1", "op-7").subscribe(r =>
(result = r));
+
+ httpMock
+ .expectOne(r => r.method === "GET" && r.url ===
"/api/agents/agent-1/react-steps")
+ .flush({
+ state: "AVAILABLE",
+ steps: [{ messageId: "no-access", timestamp:
"2026-06-11T00:00:00.000Z" }],
+ });
+
+ expect(result).toEqual({ viewedBy: [], modifiedBy: [] });
+ });
+ });
+
+ describe("mapStateToAgentState", () => {
+ it("maps STOPPING, UNAVAILABLE and unrecognised backend states", () => {
+ let mapped: AgentInfo[] | undefined;
+ service.getAllAgents().subscribe(a => (mapped = a));
+
+ httpMock
+ .expectOne(r => r.method === "GET" && r.url === "/api/agents")
+ .flush({
+ agents: [
+ { ...apiAgent, id: "s", state: "STOPPING" },
+ { ...apiAgent, id: "u", state: "UNAVAILABLE" },
+ { ...apiAgent, id: "z", state: "NOT_A_REAL_STATE" },
+ ],
+ });
+
+ const byId = new Map(mapped!.map(a => [a.id, a.state]));
+ expect(byId.get("s")).toBe(AgentState.STOPPING);
+ expect(byId.get("u")).toBe(AgentState.UNAVAILABLE);
+ // Anything unrecognised falls through the default arm to UNAVAILABLE.
+ expect(byId.get("z")).toBe(AgentState.UNAVAILABLE);
+ });
+ });
+
+ //
---------------------------------------------------------------------------
+ // HTTP failure paths and their fallback chains
+ //
---------------------------------------------------------------------------
+ describe("createAgent failure fallback chain", () => {
+ it("prefers the nested error.error message", () => {
+ let message: string | undefined;
+ service.createAgent("gpt-5-mini").subscribe({ error: (e: unknown) =>
(message = (e as Error).message) });
+ httpMock
+ .expectOne(r => r.method === "POST" && r.url === "/api/agents")
+ .flush({ error: "nested boom" }, { status: 400, statusText: "Bad
Request" });
+
+ expect(message).toBe("nested boom");
+ expect(notification.error).toHaveBeenCalledWith("nested boom");
+ });
+
+ it("falls back to err.message when there is no nested error", () => {
+ let message: string | undefined;
+ service.createAgent("gpt-5-mini").subscribe({ error: (e: unknown) =>
(message = (e as Error).message) });
+ // A body with no `.error` field leaves err.error?.error undefined, so
the
+ // HttpErrorResponse's own message is used.
+ httpMock
+ .expectOne(r => r.method === "POST" && r.url === "/api/agents")
+ .flush({}, { status: 500, statusText: "Server Error" });
+
+ expect(message).toContain("Http failure response");
+ expect(notification.error).toHaveBeenCalledWith(message);
+ });
+
+ it("falls back to the default label when the error carries neither field",
() => {
+ // An HttpErrorResponse always has a message, so the default literal is
only
+ // reachable for a non-HTTP error; a bare object exercises that last arm.
+ vi.spyOn((service as any).http, "post").mockReturnValue(throwError(() =>
({})));
+
+ let message: string | undefined;
+ service.createAgent("gpt-5-mini").subscribe({ error: (e: unknown) =>
(message = (e as Error).message) });
+
+ expect(message).toBe("Failed to create agent");
+ expect(notification.error).toHaveBeenCalledWith("Failed to create
agent");
+ });
Review Comment:
Added `vi.restoreAllMocks()` to the spec's `afterEach` in dd6eaffe — the
issue asked for it and it costs nothing.
On the leak itself, I checked rather than assumed, and it does not happen
today. TestBed rebuilds the injector between tests, so `(service as any).http`
is a different `HttpClient` instance in the next test and the spy is discarded
with the old one. A probe that installs an unrestored `http.post` spy in one
test and inspects the next reports:
```
same HttpClient instance across tests: false
post is a mock here: false
POST reached HttpTestingController: 1
createAgent succeeded: true
```
Your premise about `restoreMocks` not being set in vitest.config.ts is
correct, so the guard is worth having — it just protects against a future
change (a collaborator moving to a shared provider) rather than fixing a
current failure. Coverage is unchanged: 324/329 lines, 168/186 branches.
--
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]
