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-8066-5dd43cfe9f68f80589e26d41d4767989f86e9f35 in repository https://gitbox.apache.org/repos/asf/texera.git
commit bb56bba107d02616100edd0d1166c6e4d32fc796 Author: Xinyuan Lin <[email protected]> AuthorDate: Sat Aug 29 06:55:04 2026 +0000 test(frontend): cover the build-version script, user config and agent registration (#8066) ### What changes were proposed in this PR? Five existing frontend specs extended. **+16 fully-covered lines and +8 branch arms.** | File | Codecov lines | Branch arms | |---|---|---| | `build-version.js` | 8/15 → **15/15** | 2/3 → **4/4** | | `user-config.service.ts` | 60/63 → **63/63** | 31/33 → **33/33** | | `agent-registration.component.html` | 46/48 → **48/48** | 7/10 → **10/10** | | `code-editor.service.ts` | + 1 line | + 1 arm | | `notebook-migration.service.ts` | + 1 line | — | `build-version.js` carries most of it: lines 42–46 and 53 were never executed and 52 was partial. Its arm total rises from 3 to 4 rather than staying fixed, because loading the script as raw CJS also records the `buildNumber = generate(version)` default-parameter branch that the transformed import never emitted — worth stating, since a reader comparing arm totals would otherwise think the denominator moved for no reason. `agent-registration.component.html` gains **zero** on plain line-hit (48/48 both before and after) and moves only on Codecov's metric, by completing three branch arms. The two numbers are not interchangeable and both are given. **Deliberately excluded: every `stub-*.service.ts`.** Those six files hold 29 further missed lines — more than this whole PR — but they are test doubles, not production code, so covering them would be padding. One of them (`stub-user-project.service.ts`, 18 missed lines) is being deleted outright by the open project-removal work. ### Verification **21 mutations, no survivors.** Every one was re-derived from scratch on the shipping tree: snapshot first, anchor asserted to occur exactly once, one mutant at a time, hash-equality plus an empty non-spec diff verified immediately before each run, reverted from the snapshot rather than from git. **19 of the 21 die to exactly one test** with the rest of the file green. The other two are named rather than rounded up: the `writeFileSync` payload exchange also kills "announces the build number it produced" (the manifest stops being parseable JSON), and the error-ternary exchange at `agent-registration.component.ts:104` also kills a sibling. The baseline was re-measured rather than inherited: specs backed up to a scratch dir, each restored **by exact single file path** (never a directory), `git status --porcelain --untracked-files=all` confirmed empty, then the literal CI command body run — `nx test --watch=false --progress=false --coverage --coverage-reporters=lcovonly` from `frontend/`, which is what `yarn --cwd frontend run test:ci` executes and whose lcov is uploaded under the `frontend` flag. Specs were then restored and the restored diff byte-verified against the saved patch before the second run. Both lcovs were parsed with a script implementing Codecov's rule directly: a line counts only if its `DA` hit is non-zero **and** every `BRDA` arm on it is taken. Whole suite: **5208 passed + 1 skipped → 5220 passed + 1 skipped**, 204/204 files both times. The single skip is pre-existing and untouched. One correction to an earlier draft of this bundle: it implied the agent-registration spec started from a different test count than it did. The real per-spec counts are 7→10, 20→22, 4→5, 37→39, 15→19. `yarn format:ci` passes. `frontend/junit.xml` and `frontend/coverage/` are regenerated by every run and are not committed. No production file is touched. ### Any related issues, documentation, discussions? Closes #8064 ### How was this PR tested? ``` npx ng test --watch=false --include="**/build-version.spec.ts" --include="**/user-config.service.spec.ts" --include="**/code-editor.service.spec.ts" --include="**/notebook-migration.service.spec.ts" --include="**/agent-registration.component.spec.ts" ``` ``` Test Files 5 passed (5) Tests 95 passed (95) ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../user/config/user-config.service.spec.ts | 28 ++++ .../agent-registration.component.spec.ts | 92 ++++++++++++++ .../code-editor/code-editor.service.spec.ts | 17 +++ .../notebook-migration.service.spec.ts | 23 ++++ frontend/src/build-version.spec.ts | 141 +++++++++++++++++++++ 5 files changed, 301 insertions(+) diff --git a/frontend/src/app/common/service/user/config/user-config.service.spec.ts b/frontend/src/app/common/service/user/config/user-config.service.spec.ts index 0aceaf9e07..085c9812ba 100644 --- a/frontend/src/app/common/service/user/config/user-config.service.spec.ts +++ b/frontend/src/app/common/service/user/config/user-config.service.spec.ts @@ -197,6 +197,34 @@ describe("UserConfigService", () => { }); }); + // updateEntry is private and validates its own arguments rather than trusting + // its callers. Both guards below are unreachable through the public API -- + // fetchKey/set/delete reject a blank key before they ever tap into + // updateEntry, and delete() returns early when the key is absent -- so the + // contract updateEntry keeps for itself is pinned here by calling it directly. + describe("updateEntry (private, called directly)", () => { + it("rejects a blank key even though no public caller can supply one", () => { + expect(() => (service as any).updateEntry(" ", "v")).toThrowError(/key cannot be empty/); + // The guard runs before any mutation, so nothing was written under a + // trimmed or raw form of the blank key. + expect(service.getDict()).toEqual({}); + }); + + it("does not fire dictionaryChanged when told to delete a key that is absent", () => { + const next = vi.fn(); + const sub = (service as any).dictionaryChangedSubject.subscribe(next); + + (service as any).updateEntry("absent", undefined); + + // Asserting on the subject, not just on getDict(): a version that deleted + // and notified unconditionally would leave the dictionary looking identical + // while still waking every subscriber. + expect(next).not.toHaveBeenCalled(); + expect(service.getDict()).toEqual({}); + sub.unsubscribe(); + }); + }); + describe("user-change reactions", () => { it("re-fetches the dictionary when a logged-in user is emitted on userChanged", () => { stubUserService.userChangeSubject.next(MOCK_USER); diff --git a/frontend/src/app/workspace/component/agent/agent-panel/agent-registration/agent-registration.component.spec.ts b/frontend/src/app/workspace/component/agent/agent-panel/agent-registration/agent-registration.component.spec.ts index 16c926d31e..daa220bf39 100644 --- a/frontend/src/app/workspace/component/agent/agent-panel/agent-registration/agent-registration.component.spec.ts +++ b/frontend/src/app/workspace/component/agent/agent-panel/agent-registration/agent-registration.component.spec.ts @@ -28,6 +28,7 @@ import { NotificationService } from "../../../../../common/service/notification/ import { WorkflowActionService } from "../../../../service/workflow-graph/model/workflow-action.service"; import { ComputingUnitStatusService } from "../../../../../common/service/computing-unit/computing-unit-status/computing-unit-status.service"; import { ComputingUnitState } from "../../../../../common/type/computing-unit-connection.interface"; +import { NzTooltipDirective } from "ng-zorro-antd/tooltip"; import { commonTestProviders } from "../../../../../common/testing/test-utils"; const MODEL: ModelType = { id: "gpt", name: "GPT", description: "desc", icon: "robot" }; @@ -100,6 +101,17 @@ describe("AgentRegistrationComponent", () => { expect(notifyError).toHaveBeenCalledWith("Failed to fetch models: boom"); }); + it("stringifies a rejection that is not an Error into the same message", () => { + // The test above covers `error.message`; an rxjs source is free to reject + // with anything, and a bare string has no `.message`, so the component + // falls back to String(error) instead of interpolating `undefined`. + fetchModelTypes.mockReturnValue(throwError(() => "backend unreachable")); + fixture.detectChanges(); + + expect(component.hasLoadingError).toBe(true); + expect(notifyError).toHaveBeenCalledWith("Failed to fetch models: backend unreachable"); + }); + it("marks the computing unit connected only when the status is Running", () => { getStatus.mockReturnValue(of(ComputingUnitState.Running)); fixture.detectChanges(); @@ -214,5 +226,85 @@ describe("AgentRegistrationComponent", () => { expect((cards[1].nativeElement as HTMLElement).classList).toContain("selected"); expect((cards[0].nativeElement as HTMLElement).classList).not.toContain("selected"); }); + + it("writes what the user types in the name box back into customAgentName", async () => { + // The name box is `[(ngModel)]`-bound. Every other test in this file only + // assigns `customAgentName` on the instance, which is the direction the box + // is never driven in; typing is what feeds the name createAgent() sends. + fetchModelTypes.mockReturnValue(of([MODEL])); + // The input is `[disabled]="!selectedModelType"`, so pick a model before the + // first render; ngModel refuses to push a value into a disabled control. + component.selectModelType(MODEL.id); + fixture.detectChanges(); + // ngModel pushes the field into the DOM on a microtask rather than + // synchronously inside detectChanges, so drain it before reading the box. + await new Promise(resolve => setTimeout(resolve, 0)); + + const input = fixture.debugElement.query(By.css("input[nz-input]")).nativeElement as HTMLInputElement; + expect(input.disabled).toBe(false); + expect(input.value).toBe("Texera Agent"); + + // Typed rather than assigned -- the write-back is the untested direction. + input.value = "My Analyst"; + input.dispatchEvent(new Event("input")); + fixture.detectChanges(); + + expect(component.customAgentName).toBe("My Analyst"); + }); + + it("labels the submit button Creating... while a creation is in flight", () => { + fetchModelTypes.mockReturnValue(of([MODEL])); + fixture.detectChanges(); + + const button = fixture.debugElement.query(By.css("button[nz-button]")).nativeElement as HTMLElement; + expect(button.textContent).toContain("Create Agent"); + // The spinner is bound off the same flag as the label. Asserting only the + // label leaves `[nzLoading]` free to be bound inverted, which would spin + // the button whenever nothing is happening and stop spinning during the + // one request it exists to cover. + expect(button.classList).not.toContain("ant-btn-loading"); + + component.isCreating = true; + fixture.detectChanges(); + + expect(button.textContent).toContain("Creating..."); + expect(button.textContent).not.toContain("Create Agent"); + expect(button.classList).toContain("ant-btn-loading"); + }); + + it("keeps the submit button shut, and says why, until a model and a computing unit are both there", () => { + // canCreate() is unit-tested above; what is pinned here is the template + // actually honouring it. Nothing else in this file reads the button's + // gate, so an inverted `[disabled]` binding -- Create enabled exactly + // when creation is impossible -- passes every other test: createAgent() + // re-checks selectedModelType and isCreating but never + // computingUnitConnected, so the click would reach the backend. + fetchModelTypes.mockReturnValue(of([MODEL])); + fixture.detectChanges(); + + const buttonEl = fixture.debugElement.query(By.css("button[nz-button]")); + const button = buttonEl.nativeElement as HTMLButtonElement; + const tooltip = buttonEl.injector.get(NzTooltipDirective); + + // getStatus defaults to Pending, so the gate legitimately starts shut. + expect(component.canCreate()).toBe(false); + expect(button.disabled).toBe(true); + // Both arms of the tooltip ternary are already executed by this suite, so + // only comparing the strings notices them being handed to the wrong side. + expect(tooltip.title).toBe("Connect to a computing unit first"); + // The banner carries the same explanation. Its *ngIf arms are likewise + // both executed by the suite, so only checking WHICH state renders it + // notices the condition being inverted. + expect(fixture.debugElement.query(By.css("nz-alert"))).toBeTruthy(); + + component.computingUnitConnected = true; + component.selectedModelType = MODEL.id; + fixture.detectChanges(); + + expect(component.canCreate()).toBe(true); + expect(button.disabled).toBe(false); + expect(tooltip.title).toBe(""); + expect(fixture.debugElement.query(By.css("nz-alert"))).toBeNull(); + }); }); }); diff --git a/frontend/src/app/workspace/service/code-editor/code-editor.service.spec.ts b/frontend/src/app/workspace/service/code-editor/code-editor.service.spec.ts index 500e5fe2ec..6f514b46c1 100644 --- a/frontend/src/app/workspace/service/code-editor/code-editor.service.spec.ts +++ b/frontend/src/app/workspace/service/code-editor/code-editor.service.spec.ts @@ -44,6 +44,23 @@ describe("CodeEditorService", () => { expect(value).toBe(false); }); + it("seeds the state when setEditorState runs before any getEditorState", () => { + // Nothing has asked for "fresh" yet, so setEditorState has no subject to push + // onto and must create one already carrying the requested value. Every other + // test in this file calls getEditorState first, which pre-creates the subject + // seeded to false, so this is the only path that exercises the seeding. + service.setEditorState("fresh", true); + + let value: boolean | undefined; + service.getEditorState("fresh").subscribe(v => (value = v)); + expect(value).toBe(true); + + // The seeded subject is the same one getEditorState handed out -- a later + // write reaches the existing subscriber rather than a replacement. + service.setEditorState("fresh", false); + expect(value).toBe(false); + }); + it("should track state independently for different operator IDs", () => { let valueA: boolean | undefined; let valueB: boolean | undefined; diff --git a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts index 7ffa452a48..3dbe846203 100644 --- a/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts +++ b/frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts @@ -24,6 +24,7 @@ import { HttpClientTestingModule, HttpTestingController } from "@angular/common/ import { NotificationService } from "src/app/common/service/notification/notification.service"; import { GuiConfigService } from "src/app/common/service/gui-config.service"; import { WorkflowUtilService } from "../workflow-graph/util/workflow-util.service"; +import { NotebookMigrationLLM } from "./migration-llm"; import { firstValueFrom, throwError } from "rxjs"; describe("NotebookMigrationService", () => { @@ -297,6 +298,28 @@ describe("NotebookMigrationService", () => { expect(result).toEqual({ success: true, deleted: 1 }); }); + // Every other test that touches the LLM replaces createMigrationLLM() with a + // fake, so the seam's own body -- the one place that decides which collaborators + // the real client is wired to, and in which order -- is never run. This calls it + // directly. It stays safe for the module graph: the service already imports + // NotebookMigrationLLM at its own top, so "ai" is loaded here either way, and + // the constructor only assigns its two arguments. Deliberately no vi.mock("ai") + // in this file -- migration-llm.spec.ts owns that, and module mocks leak between + // files under `isolate: false`. + it("wires a real client to the injected config and workflow-util service", () => { + const llm = (service as any).createMigrationLLM(); + + expect(llm).toBeInstanceOf(NotebookMigrationLLM); + // Identity, not shape: the two constructor parameters are both plain objects + // here, so only `toBe` notices if they are ever passed in the wrong order. + expect((llm as any).config).toBe(mockGuiConfigService); + expect((llm as any).workflowUtilService).toBe(TestBed.inject(WorkflowUtilService)); + }); + + it("hands out a fresh client per call so one conversion cannot see another's state", () => { + expect((service as any).createMigrationLLM()).not.toBe((service as any).createMigrationLLM()); + }); + // sendToAIGenerateWorkflow (enabled) — drives the NotebookMigrationLLM lifecycle. // The service builds the client through its createMigrationLLM() seam, so stub // that with a plain fake. This keeps the real NotebookMigrationLLM (and its "ai" diff --git a/frontend/src/build-version.spec.ts b/frontend/src/build-version.spec.ts index a27f1f6f29..def462cc3f 100644 --- a/frontend/src/build-version.spec.ts +++ b/frontend/src/build-version.spec.ts @@ -17,6 +17,8 @@ * under the License. */ +import { createRequire } from "node:module"; +import { dirname, join } from "node:path"; import { renderVersionArtifacts } from "../build-version"; describe("build-version: renderVersionArtifacts", () => { @@ -77,3 +79,142 @@ describe("build-version: renderVersionArtifacts", () => { }); }); }); + +// The suite above covers the pure renderer. What actually runs in CI is the +// script's entry point: `node build-version.js`, whose main() decides WHICH of +// the two rendered artifacts goes to WHICH path. Angular's production +// fileReplacements reads src/environments/version.prod.ts and the running app +// fetches src/assets/version.json, so crossing those two writes would ship a +// bundle whose version banner never matches its manifest -- and nothing else in +// the repo would notice, because both files are gitignored build outputs. +// +// main() is deliberately not exported (module.exports exposes only +// renderVersionArtifacts), so the only way in without editing the script is to +// load it the way Node does when it is the program: build a Module, install it +// as process.mainModule, and let the `require.main === module` guard fire. +describe("build-version: main()", () => { + // A CJS require. It resolves to the same module instances build-version.js + // itself requires, which is what makes the fs stub below land on the object + // the script destructures `writeFileSync` out of. (An `import * as fs` would + // not: esbuild refuses assignment to an import binding, and it would be a + // different namespace object anyway.) + // + // Anchored on the Vitest root -- frontend/, where build-version.js sits -- + // rather than on import.meta.url, which is NOT stable here: the unit-test + // builder rewrites this spec to a synthetic frontend/spec-build-version.js + // under `--coverage` and leaves it at src/build-version.spec.ts without, so a + // "../" relative to it lands in a different directory in each mode. + const requireCjs = createRequire(join(process.cwd(), "build-version.spec.anchor.cjs")); + const NodeModule = requireCjs("node:module") as any; + const fs = requireCjs("fs") as { writeFileSync: unknown }; + const scriptPath: string = requireCjs.resolve("./build-version.js"); + + type Write = { path: string; data: string }; + + // Runs build-version.js exactly as `node build-version.js` would, with + // writeFileSync and console.log captured instead of hitting the disk. + function loadScript(asMain: boolean): { writes: Write[]; logs: string[] } { + const writes: Write[] = []; + const logs: string[] = []; + + const realWriteFileSync = fs.writeFileSync; + const realMainModule = process.mainModule; + const cachedEntry = NodeModule._cache[scriptPath]; + const logSpy = vi.spyOn(console, "log").mockImplementation((...args: unknown[]) => { + logs.push(args.join(" ")); + }); + + // Stubbed before the load, because the script destructures writeFileSync at + // module-evaluation time -- a later swap would be invisible to it. + fs.writeFileSync = (target: unknown, data: unknown) => + void writes.push({ path: String(target), data: String(data) }); + + try { + // A fresh Module: the guard compares object identity, so the script must + // be evaluated again rather than served from the require cache. + delete NodeModule._cache[scriptPath]; + const scriptModule = new NodeModule(scriptPath, null); + scriptModule.filename = scriptPath; + scriptModule.paths = NodeModule._nodeModulePaths(dirname(scriptPath)); + // `require.main` is whatever process.mainModule holds. When the script is + // NOT the program, some OTHER module is -- that is the situation the guard + // exists for -- so stand a different module up rather than leaving + // process.mainModule unset, which would let a guard as loose as + // `require.main != null` pass for the wrong reason. + process.mainModule = asMain ? scriptModule : new NodeModule(scriptPath + ".importer.js", null); + NodeModule._cache[scriptPath] = scriptModule; + scriptModule.load(scriptPath); + return { writes, logs }; + } finally { + fs.writeFileSync = realWriteFileSync; + process.mainModule = realMainModule; + if (cachedEntry) { + NodeModule._cache[scriptPath] = cachedEntry; + } else { + delete NodeModule._cache[scriptPath]; + } + logSpy.mockRestore(); + } + } + + it("sends the bundle constant and the manifest to the two paths the prod build reads", () => { + const { writes } = loadScript(true); + + expect(writes.length).toBe(2); + + // Each artifact is matched to its own destination, not merely counted: a + // version that wrote the right number of files to the right two paths with + // the contents swapped would still be broken. + const prodTsWrite = writes.find(w => w.path.endsWith("version.prod.ts")); + const manifestWrite = writes.find(w => w.path.endsWith("version.json")); + expect(prodTsWrite).toBeDefined(); + expect(manifestWrite).toBeDefined(); + + // The whole path, not a tail of it. A tail match cannot tell + // frontend/src/environments/version.prod.ts apart from the same suffix one + // directory up, and only the first is where Angular's fileReplacements + // looks and where the app fetches the manifest from -- a base directory the + // script resolved wrongly would ship the static "dev" version silently. + // Anchored on the script's own directory rather than process.cwd() so it + // holds in both runner modes. + expect(prodTsWrite!.path).toBe(join(dirname(scriptPath), "src", "environments", "version.prod.ts")); + expect(prodTsWrite!.data).toContain("export const Version"); + expect(prodTsWrite!.data).toContain("AUTO-GENERATED"); + + expect(manifestWrite!.path).toBe(join(dirname(scriptPath), "src", "assets", "version.json")); + const manifest = JSON.parse(manifestWrite!.data); + + // The version comes from package.json, not from a literal in the script. + const pkgVersion: string = requireCjs("./package.json").version; + expect(manifest.version).toBe(pkgVersion); + + // An anchor that does NOT come out of main()'s own output. Every other + // assertion here reads the build number back from what main() wrote, so + // they agree with each other no matter what it was; only this one says what + // the value has to BE. The generator's contract is `<version>.<digits>`, so + // a main() that passed a constant, or passed the version as its own build + // number, fails here and nowhere else. + expect(manifest.buildNumber.startsWith(`${pkgVersion}.`)).toBe(true); + expect(manifest.buildNumber.slice(pkgVersion.length + 1)).toMatch(/^\d+$/); + + // The comparison the running app makes: the number baked into the bundle + // and the number served in the manifest have to be the same one. + expect(prodTsWrite!.data).toContain(JSON.stringify(manifest.buildNumber)); + expect(prodTsWrite!.data).toContain(JSON.stringify(manifest.version)); + }); + + it("announces the build number it produced", () => { + const { writes, logs } = loadScript(true); + const manifest = JSON.parse(writes.find(w => w.path.endsWith("version.json"))!.data); + expect(logs).toEqual([`build-version: ${manifest.buildNumber}`]); + }); + + it("writes nothing when the script is merely required rather than run", () => { + // The other half of the `require.main === module` guard: importing the + // module for its renderer -- which this spec's own top-level import does -- + // must not touch the working tree. + const { writes, logs } = loadScript(false); + expect(writes).toEqual([]); + expect(logs).toEqual([]); + }); +});
