zyratlo commented on code in PR #5262: URL: https://github.com/apache/texera/pull/5262#discussion_r3482833932
########## frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.spec.ts: ########## @@ -0,0 +1,328 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { TestBed } from "@angular/core/testing"; +import { NotebookMigrationService } from "./notebook-migration.service"; +import { HttpClient } from "@angular/common/http"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; +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 { firstValueFrom, throwError } from "rxjs"; + +// NotebookMigrationLLM is constructed inside sendToAIGenerateWorkflow. Mock the +// whole module so the LLM lifecycle is driven by `mockLLM` instead of real +// network / AI-SDK calls. vi.hoisted lets the mock instance be referenced from +// the (hoisted) vi.mock factory. +const { mockLLM } = vi.hoisted(() => ({ + mockLLM: { + initialize: vi.fn(), + verifyConnection: vi.fn(), + convertNotebookToWorkflow: vi.fn(), + close: vi.fn(), + }, +})); +vi.mock("./migration-llm", () => ({ + NotebookMigrationLLM: vi.fn(() => mockLLM), +})); + +describe("NotebookMigrationService", () => { + let service: NotebookMigrationService; + let httpMock: HttpTestingController; + let mockNotificationService: { success: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn> }; + // Mutable so individual describe blocks can flip the flag mid-spec by + // reassigning `mockGuiConfigService.env.pythonNotebookMigrationEnabled`. + // The service stores a reference to this object, so mutations are observed + // on the next read of `this.enabled`. + let mockGuiConfigService: { env: { pythonNotebookMigrationEnabled: boolean } }; + + beforeEach(() => { + mockNotificationService = { + success: vi.fn(), + error: vi.fn(), + }; + mockGuiConfigService = { env: { pythonNotebookMigrationEnabled: true } }; + + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [ + NotebookMigrationService, + { provide: NotificationService, useValue: mockNotificationService }, + { provide: GuiConfigService, useValue: mockGuiConfigService }, + // Stub so the real WorkflowUtilService (and its OperatorMetadataService, + // which fires GET /api/resources/operator-metadata on construction) is + // never built. The service only passes it to NotebookMigrationLLM, which + // no test exercises. + { provide: WorkflowUtilService, useValue: {} }, + ], + }); + + service = TestBed.inject(NotebookMigrationService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + vi.restoreAllMocks(); + }); + + // getAvailableModels + it("should fetch and map available models", async () => { + const mockResponse = { + data: [ + { id: "gpt-4", object: "", created: 0, owned_by: "" }, + { id: "gpt-3.5", object: "", created: 0, owned_by: "" }, + ], + object: "", + }; + + const promise = firstValueFrom(service.getAvailableModels()); + + const req = httpMock.expectOne(req => req.url.includes("/models")); + expect(req.request.method).toBe("GET"); + req.flush(mockResponse); + + const models = await promise; + expect(models.length).toBe(2); + expect(models[0].name).toBe("gpt-4"); + }); + + it("should return empty array on getAvailableModels error", async () => { + const promise = firstValueFrom(service.getAvailableModels()); + + const req = httpMock.expectOne(req => req.url.includes("/models")); + req.error(new ErrorEvent("Network error")); + + expect(await promise).toEqual([]); + }); + + // sendNotebookToJupyter + it("should send notebook successfully and return 1", async () => { + const mockNotebook: any = { cells: [] }; + + const promise = service.sendNotebookToJupyter(mockNotebook); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); + + expect(req.request.method).toBe("POST"); + + req.flush({ success: true }); + + const result = await promise; + + expect(result).toBe(1); + expect(mockNotificationService.success).toHaveBeenCalled(); + }); + + it("should handle error when sending notebook and return 0", async () => { + const mockNotebook: any = { cells: [] }; + + const promise = service.sendNotebookToJupyter(mockNotebook); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/set-notebook")); + + req.error(new ErrorEvent("Server error")); + + const result = await promise; + + expect(result).toBe(0); + expect(mockNotificationService.error).toHaveBeenCalled(); + }); + + it("includes the Error message in the failure toast when an Error is thrown", async () => { + // HttpTestingController's req.error yields an HttpErrorResponse (not an Error + // instance), so spy on http.post directly to exercise the `error instanceof + // Error` branch. No request reaches the testing backend, so verify() stays happy. + vi.spyOn(TestBed.inject(HttpClient), "post").mockReturnValue(throwError(() => new Error("network down"))); + + const result = await service.sendNotebookToJupyter({ cells: [] } as any); + + expect(result).toBe(0); + expect(mockNotificationService.error).toHaveBeenCalledWith(expect.stringContaining("network down")); + }); + + // jupyter URL methods (HttpClient so the JwtModule interceptor attaches the auth token) + it("should return Jupyter URL when the request succeeds", async () => { + const promise = service.getJupyterURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-url")); + expect(req.request.method).toBe("GET"); + req.flush({ success: true, url: "http://jupyter"; }); + + expect(await promise).toBe("http://jupyter";); + }); + + it("should return null when the Jupyter URL request fails", async () => { + const promise = service.getJupyterURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-url")); + req.flush({ success: false }, { status: 500, statusText: "Server Error" }); + + expect(await promise).toBeNull(); + }); + + it("should return null when the Jupyter URL response is 200 but unsuccessful", async () => { + const promise = service.getJupyterURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-url")); + req.flush({ success: false }); + + expect(await promise).toBeNull(); + }); + + it("should return iframe URL when the request succeeds", async () => { + const promise = service.getJupyterIframeURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + expect(req.request.method).toBe("GET"); + req.flush({ success: true, url: "http://iframe"; }); + + expect(await promise).toBe("http://iframe";); + }); + + it("should return null when the iframe URL request fails", async () => { + const promise = service.getJupyterIframeURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + req.flush({ success: false }, { status: 500, statusText: "Server Error" }); + + expect(await promise).toBeNull(); + }); + + it("should return null when the iframe URL response is 200 but unsuccessful", async () => { + const promise = service.getJupyterIframeURL(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/get-jupyter-iframe-url")); + req.flush({ success: false }); + + expect(await promise).toBeNull(); + }); + + // mapping logic + it("should set and get mapping", () => { + const mockMapping: any = { + cell_to_operator: { a: 1 }, + operator_to_cell: { b: 2 }, + }; + + service.setMapping("test", mockMapping); + + expect(service.hasMapping("test")).toBe(true); + expect(service.getMapping("test")).toEqual(mockMapping); + }); + + it("should delete mapping", () => { + service.setMapping("test", { cell_to_operator: {}, operator_to_cell: {} }); + + service.deleteMapping("test"); + + expect(service.hasMapping("test")).toBe(false); + }); + + // storeNotebookAndMapping + it("should call storeNotebookAndMapping API", () => { + service.storeNotebookAndMapping(1, 1, {}, {}).subscribe(); + + const req = httpMock.expectOne(req => req.url.includes("/notebook-migration/store-notebook-and-mapping")); + + expect(req.request.method).toBe("POST"); + }); Review Comment: Flushing added for consistency with the other HTTP tests here. To clarify the stated reasoning though: httpMock.verify() does not fail in this case. expectOne(...) matches and consumes the request, so it's no longer outstanding when verify() runs in afterEach — the test passed in CI without the flush. The change is still worthwhile because it completes the request and avoids a dangling subscription. Refactored in [5a06993](https://github.com/apache/texera/pull/5262/commits/5a06993bcf764bdfb80ac82b4148cf66710d2ebd) -- 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]
