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-7908-4931330c377d8c8698747bdad4411fda8b4c5b83 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 54a9a597d87eb63304af77c67ab744da37086d03 Author: Meng Wang <[email protected]> AuthorDate: Tue Aug 25 00:48:16 2026 +0000 test(frontend): cover the agent service's state accessors and failure paths (#7908) ### What changes were proposed in this PR? Extends `agent.service.spec.ts` over the two gaps the issue lists — the state accessors' untracked side, and the HTTP failure paths — with 24 tests. Measured locally with `--coverage --coverage-reporters=lcovonly`: | `agent.service.ts` | Before | After | | --- | --- | --- | | lines | 301/329 (91.49 %) | **324/329 (98.48 %)** | | branches | 147/186 | **168/186** | | functions | 90/103 | **102/103** | **Accessors — both arms of each `tracking ? … : …`**, by calling each for a tracked agent and for an unknown id: `getAgentState` / `isAgentConnected`, `getHeadId` (+ observable), `getVisibleSteps`, `getWorkflowObservable`, `getAgentWorkflowId`'s `agent?.delegate?.workflowId` chain (missing agent, no delegate, full), and `getAgentCount`. `setHoveredMessage` gets a non-null step that carries no operator access (the else-branch, distinct from the already-covered null case) and a no-tracking no-op; `getReActStepsByOperatorAccess` gets a step with no `operatorAccess`. `mapStateToAgentState` is fed `STOPPING`, `UNAVAILABLE` and an unrecognised value through `getAllAgents`. **Failure paths:** - `createAgent` and `updateAgentSettings` — the `err.error?.error || err.message || "…"` fallback chains. The nested and message arms use a real `HttpTestingController` flush; the default-label arm is only reachable for an error that is *not* an `HttpErrorResponse` (which always carries a `message`), so that one case throws a bare object through a `throwError` stub, noted in a comment. - `getReActSteps` — `catchError(() => of([]))` emits `[]` rather than propagating. - `syncAgentsWithBackend` — the `catchError` (a failed sync is treated as an empty backend), the `if (existingAgent)` false side (a backend agent not cached locally is not merged in), and the `if (tracking)` false side (an existing agent with no tracking entry still has its state updated). - `getAllAgents` — the non-pruning branch: a local agent the backend still reports is kept. - `stopGeneration` — both error handlers (a throwing websocket `send`, and the REST fallback failing). - `getOrCreateStateTracking` — the workflow-id back-fill arm, reached by re-entering through `ensureWorkflowPolling` after tracking was created without an id. Determinism: every request is flushed synchronously through `HttpTestingController` and `httpMock.verify()` runs in `afterEach`; errors are raised with `flush`/`throwError` rather than a live host; `console.error` spies are restored per test; each test gets a fresh service from `TestBed`, so the internal agent/tracking maps never leak. The remaining uncovered lines are the websocket transport paths, which the issue scoped out. No production code was changed. ### Any related issues, documentation, discussions? Closes #7889. ### How was this PR tested? `ng test --watch=false --include src/app/workspace/service/agent/agent.service.spec.ts` — 71 passed (47 before, 24 new), repeated 3× for stability; the whole `workspace/service/agent/**` folder stays green. `yarn format:ci` clean. Failure path verified by breaking one assertion in each of the 24 new tests: 24 failed / 47 passed, non-zero exit, then restored to green. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8 [1M context]) --- .../workspace/service/agent/agent.service.spec.ts | 339 +++++++++++++++++++++ 1 file changed, 339 insertions(+) diff --git a/frontend/src/app/workspace/service/agent/agent.service.spec.ts b/frontend/src/app/workspace/service/agent/agent.service.spec.ts index e2a6ba8674..ac98d65c46 100644 --- a/frontend/src/app/workspace/service/agent/agent.service.spec.ts +++ b/frontend/src/app/workspace/service/agent/agent.service.spec.ts @@ -125,6 +125,10 @@ describe("AgentService", () => { afterEach(() => { httpMock.verify(); + // Belt and braces: TestBed rebuilds the injector per test, so the spies installed + // on the service's own collaborators are already discarded with it. This keeps the + // spec safe if any of them ever moves to a shared provider. + vi.restoreAllMocks(); }); describe("createAgent", () => { @@ -965,4 +969,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"); + }); + }); + + describe("updateAgentSettings edge cases", () => { + it("returns the response without touching the cache when the agent is unknown", () => { + let updated: AgentSettingsApi | undefined; + service.updateAgentSettings("uncached", { maxSteps: 9 }).subscribe(s => (updated = s)); + + httpMock.expectOne(r => r.method === "PATCH" && r.url === "/api/agents/uncached/settings").flush({ maxSteps: 9 }); + + expect(updated).toEqual({ maxSteps: 9 }); + expect((service as any).agents.has("uncached")).toBe(false); + }); + + it("falls back to the default label when the failure carries neither field", () => { + vi.spyOn((service as any).http, "patch").mockReturnValue(throwError(() => ({}))); + + let message: string | undefined; + service + .updateAgentSettings("agent-1", { maxSteps: 3 }) + .subscribe({ error: (e: unknown) => (message = (e as Error).message) }); + + expect(message).toBe("Failed to update agent settings"); + expect(notification.error).toHaveBeenCalledWith("Failed to update agent settings"); + }); + }); + + describe("getReActSteps failure", () => { + it("emits an empty array instead of propagating the error", () => { + let steps: ReActStep[] | undefined; + service.getReActSteps("agent-1").subscribe(s => (steps = s)); + + httpMock + .expectOne(r => r.method === "GET" && r.url === "/api/agents/agent-1/react-steps") + .flush("boom", { status: 500, statusText: "Server Error" }); + + expect(steps).toEqual([]); + }); + }); + + describe("syncAgentsWithBackend branches", () => { + it("swallows a failed sync and treats the backend as empty", () => { + seedAgent("agent-1"); + + (service as any).syncAgentsWithBackend(); + httpMock + .expectOne(r => r.method === "GET" && r.url === "/api/agents") + .flush("boom", { status: 500, statusText: "Server Error" }); + + // catchError substitutes { agents: [] }, so the local agent is evicted. + expect((service as any).agents.size).toBe(0); + }); + + it("does not add a backend agent that is not already cached locally", () => { + seedAgent("agent-1"); + + (service as any).syncAgentsWithBackend(); + httpMock + .expectOne(r => r.method === "GET" && r.url === "/api/agents") + .flush({ agents: [{ ...apiAgent, id: "agent-2", state: "GENERATING" }] }); + + // agent-1 is gone (not on the backend) and agent-2 is not merged in: sync only + // updates agents it already knows about. + expect((service as any).agents.has("agent-1")).toBe(false); + expect((service as any).agents.has("agent-2")).toBe(false); + }); + + it("updates an existing agent's state even when it has no tracking entry", () => { + // An agent in the cache with no tracking entry exercises the `if (tracking)` guard's + // false side while still taking the `if (existingAgent)` true side. + (service as any).agents.set("agent-1", { id: "agent-1", name: "Bob", state: AgentState.AVAILABLE }); + + (service as any).syncAgentsWithBackend(); + httpMock + .expectOne(r => r.method === "GET" && r.url === "/api/agents") + .flush({ agents: [{ ...apiAgent, id: "agent-1", state: "GENERATING" }] }); + + expect((service as any).agents.get("agent-1").state).toBe(AgentState.GENERATING); + }); + }); + + describe("getAllAgents keeps agents still present on the backend", () => { + it("does not prune a local agent that the backend still reports", () => { + seedAgent("agent-1"); + let result: AgentInfo[] | undefined; + service.getAllAgents().subscribe(r => (result = r)); + + httpMock + .expectOne(r => r.method === "GET" && r.url === "/api/agents") + .flush({ + agents: [ + { ...apiAgent, id: "agent-1", state: "AVAILABLE" }, + { ...apiAgent, id: "agent-2", state: "GENERATING" }, + ], + }); + + expect(result?.map(a => a.id).sort()).toEqual(["agent-1", "agent-2"]); + expect((service as any).agents.has("agent-1")).toBe(true); + }); + }); + + describe("stopGeneration error handlers", () => { + it("logs when the websocket send throws", () => { + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + (service as any).agentStateTracking.set("agent-1", { + websocket: { + readyState: WebSocket.OPEN, + send: vi.fn(() => { + throw new Error("send failed"); + }), + }, + }); + + service.stopGeneration("agent-1"); + + expect(errSpy).toHaveBeenCalledWith("Failed to send stop command:", expect.any(Error)); + errSpy.mockRestore(); + }); + + it("logs when the REST fallback fails", () => { + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + (service as any).agentStateTracking.set("agent-1", { + websocket: { readyState: WebSocket.CLOSED, send: vi.fn() }, + }); + + service.stopGeneration("agent-1"); + httpMock + .expectOne(r => r.method === "POST" && r.url === "/api/agents/agent-1/stop") + .flush("boom", { status: 500, statusText: "Server Error" }); + + expect(errSpy).toHaveBeenCalledWith("Error stopping agent agent-1:", expect.anything()); + errSpy.mockRestore(); + }); + }); + + describe("getOrCreateStateTracking back-fills a workflow id", () => { + it("adds a workflow id onto tracking that was created without one", () => { + // First create tracking with no workflow id. + service.getAgentStateObservable("agent-1").subscribe(); + expect((service as any).agentStateTracking.get("agent-1").workflowId).toBeUndefined(); + + // ensureWorkflowPolling re-enters getOrCreateStateTracking with an id, hitting the + // else-if that back-fills it. + service.ensureWorkflowPolling("agent-1", 55); + expect((service as any).agentStateTracking.get("agent-1").workflowId).toBe(55); + + // The back-filled id now rides on outgoing requests. + service.getAgentSettings("agent-1").subscribe(); + const req = httpMock.expectOne(r => r.url === "/api/agents/agent-1/settings"); + expect(req.request.headers.get("X-Agent-Workflow-Id")).toBe("55"); + req.flush({}); + }); + }); });
