diff --git a/LICENSE b/LICENSE index a3ec98ddeb9..8d2b1ab4737 100644 --- a/LICENSE +++ b/LICENSE @@ -238,6 +238,13 @@ This product bundles code derived from inner-class: Source: https://github.com/sebkeim/inner-class License: MIT License (licenses-3rd-party-code/sebkeim-inner-class.txt) +This product bundles a patch to Vitest's @vitest/spy package, applied by Yarn to the +copy the frontend unit tests install: + - frontend/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch + Copyright (c) 2021-Present VoidZero Inc. and Vitest contributors + Source: https://github.com/vitest-dev/vitest + License: MIT License (licenses-3rd-party-code/vitest.txt) + This product bundles the ScalaPB options definition: - common/workflow-core/src/main/protobuf/scalapb/scalapb.proto Source: https://github.com/scalapb/ScalaPB diff --git a/frontend/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch b/frontend/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch new file mode 100644 index 00000000000..6db737a6702 --- /dev/null +++ b/frontend/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch @@ -0,0 +1,41 @@ +diff --git a/dist/index.js b/dist/index.js +index 6d907fc02f65ca9b3959f75c623e83ca866a457c..d29a2b2b310b19ffb13bbe82d99877e7317c919c 100644 +--- a/dist/index.js ++++ b/dist/index.js +@@ -5,7 +5,12 @@ const MOCK_RESTORE = new Set(); + // Jest keeps the state in a separate WeakMap which is good for memory, + // but it makes the state slower to access and return different values + // if you stored it before calling `mockClear` where it will be recreated ++// Patched (Texera): mocks are held through WeakRefs, as in Vitest 5, so a mock nothing else references ++// is collected along with what it recorded or spied on. Drop once Angular's builder allows Vitest 5. + const REGISTERED_MOCKS = new Set(); ++const MOCK_FINALIZER = new FinalizationRegistry((ref) => { ++ REGISTERED_MOCKS.delete(ref); ++}); + const MOCK_CONFIGS = new WeakMap(); + function createMockInstance(options = {}) { + const { originalImplementation, restore, mockImplementation, resetToMockImplementation, resetToMockName } = options; +@@ -33,7 +38,9 @@ function createMockInstance(options = {}) { + config.mockName = mock.name || "vi.fn()"; + } + MOCK_CONFIGS.set(mock, config); +- REGISTERED_MOCKS.add(mock); ++ const ref = new WeakRef(mock); ++ REGISTERED_MOCKS.add(ref); ++ MOCK_FINALIZER.register(mock, ref); + mock._isMockFunction = true; + mock.getMockImplementation = () => { + // Jest only returns `config.mockImplementation` here, +@@ -471,10 +478,10 @@ function restoreAllMocks() { + MOCK_RESTORE.clear(); + } + function clearAllMocks() { +- REGISTERED_MOCKS.forEach((mock) => mock.mockClear()); ++ REGISTERED_MOCKS.forEach((ref) => ref.deref()?.mockClear()); + } + function resetAllMocks() { +- REGISTERED_MOCKS.forEach((mock) => mock.mockReset()); ++ REGISTERED_MOCKS.forEach((ref) => ref.deref()?.mockReset()); + } + function throwConstructorError(shorthand) { + throw new TypeError(`Cannot use \`${shorthand}\` when called with \`new\`. Use \`mockImplementation\` with a \`class\` keyword instead. See https://vitest.dev/api/mock#class-support for more information.`); diff --git a/frontend/package.json b/frontend/package.json index 2d9905478e1..975db1ebd92 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -84,7 +84,8 @@ "jschardet": "portal:./tools/jschardet-stub", "@angular-devkit/build-angular": "21.2.19", "@angular/build": "21.2.19", - "@ngtools/webpack": "21.2.19" + "@ngtools/webpack": "21.2.19", + "@vitest/spy@npm:4.1.11": "patch:@vitest/spy@npm%3A4.1.11#~/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch" }, "devDependencies": { "@angular-builders/custom-webpack": "21.1.0", diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts index 3abaddd342f..b1aaef76363 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.spec.ts @@ -420,6 +420,15 @@ describe("JupyterPanelService", () => { expect(mockWorkflow.highlightLinks).not.toHaveBeenCalled(); }); + it("stops listening for notebook messages when its injector is destroyed", () => { + const removeListener = vi.spyOn(window, "removeEventListener"); + + TestBed.resetTestingModule(); + + // window outlives the service, so a listener left behind would keep it reachable + expect(removeListener).toHaveBeenCalledWith("message", (service as any).handleNotebookMessage); + }); + // handleNotebookMessage must only act on cellClicked messages that come from // our own iframe (event.source) AND carry the Jupyter origin. it("handleNotebookMessage highlights only for messages from the iframe at the Jupyter origin", async () => { diff --git a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts index 7b56bca5323..02a99923feb 100644 --- a/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts +++ b/frontend/src/app/workspace/service/jupyter-panel/jupyter-panel.service.ts @@ -17,7 +17,7 @@ * under the License. */ -import { Injectable } from "@angular/core"; +import { Injectable, OnDestroy } from "@angular/core"; import { BehaviorSubject, catchError, map, of } from "rxjs"; import { WorkflowActionService } from "../workflow-graph/model/workflow-action.service"; import { OperatorLink } from "../../types/workflow-common.interface"; @@ -35,7 +35,7 @@ import { GuiConfigService } from "../../../common/service/gui-config.service"; @Injectable({ providedIn: "root", }) -export class JupyterPanelService { +export class JupyterPanelService implements OnDestroy { private jupyterNotebookPanelVisible = new BehaviorSubject(false); public jupyterNotebookPanelVisible$ = this.jupyterNotebookPanelVisible.asObservable(); @@ -64,6 +64,10 @@ export class JupyterPanelService { window.addEventListener("message", this.handleNotebookMessage); } + ngOnDestroy(): void { + window.removeEventListener("message", this.handleNotebookMessage); + } + private get enabled(): boolean { return this.config.env.pythonNotebookMigrationEnabled; } diff --git a/frontend/src/app/workspace/service/workflow-graph/model/shared-model.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/shared-model.spec.ts index b6cf119b0dd..c65bc2096f7 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/shared-model.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/shared-model.spec.ts @@ -110,28 +110,28 @@ describe("SharedModel", () => { }); describe("destroy", () => { - // `destroy` only disconnects when the provider both wants to connect and is connected. - const cases: { shouldConnect: boolean; wsconnected: boolean; disconnects: boolean }[] = [ - { shouldConnect: true, wsconnected: true, disconnects: true }, - { shouldConnect: false, wsconnected: true, disconnects: false }, - { shouldConnect: true, wsconnected: false, disconnects: false }, - ]; - - cases.forEach(({ shouldConnect, wsconnected, disconnects }) => { - it(`${disconnects ? "disconnects" : "does not disconnect"} when shouldConnect=${shouldConnect} and wsconnected=${wsconnected}`, () => { - const sharedModel = build(1, user); - const provider = sharedModel.wsProvider; - // spied rather than called for real: the flags below are set by hand, so an - // actual disconnect would run against a socket that was never opened - const disconnect = vi.spyOn(provider, "disconnect").mockImplementation(() => {}); - provider.shouldConnect = shouldConnect; - provider.wsconnected = wsconnected; - - sharedModel.destroy(); - model = undefined; // already destroyed; keep afterEach from destroying twice - - expect(disconnect).toHaveBeenCalledTimes(disconnects ? 1 : 0); - }); + it("destroys the provider, not just disconnects it", () => { + const sharedModel = build(1, user); + const destroy = vi.spyOn(sharedModel.wsProvider, "destroy"); + + sharedModel.destroy(); + model = undefined; // already destroyed; keep afterEach from destroying twice + + // disconnect() alone leaves the provider's connection-check interval running, and that + // interval keeps the provider and its doc alive for as long as the page is open + expect(destroy).toHaveBeenCalledTimes(1); + }); + + it("stops a provider that never connected from retrying", () => { + const sharedModel = build(1, user); + const provider = sharedModel.wsProvider; + // the server was unreachable: the provider is still trying to connect + provider.wsconnected = false; + + sharedModel.destroy(); + model = undefined; // already destroyed; keep afterEach from destroying twice + + expect(provider.shouldConnect).toBe(false); }); }); }); diff --git a/frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts b/frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts index c3179d6d5ae..010caee0595 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts @@ -151,7 +151,11 @@ export class SharedModel { public destroy(): void { this.awareness.destroy(); try { - if (this.wsProvider.shouldConnect && this.wsProvider.wsconnected) this.wsProvider.disconnect(); + // destroy(), not just disconnect(): the provider also runs a connection-check interval, holds + // a BroadcastChannel subscription and listens on the doc and awareness, and only destroy() + // releases those. It disconnects too, which also stops a never-connected provider from + // retrying forever. + this.wsProvider.destroy(); } catch (e) {} this.yDoc.destroy(); } diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts index 984ff04ff9c..437ffa31e86 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts @@ -78,6 +78,13 @@ describe("WorkflowActionService", () => { jointGraph = (service as any).jointGraph; }); + // Some tests stub the provider's connect/disconnect. Put the real ones back before TestBed + // teardown destroys the shared model (this hook runs before the file-level teardown), or the + // provider is never closed, keeps redialling, and keeps this test's service alive with it. + afterEach(() => { + vi.restoreAllMocks(); + }); + /** * The shared document has finished its first sync with the room. A seed of the workflow's * settings or form definition waits for this, so that a co-editor's newer value is already in @@ -94,6 +101,15 @@ describe("WorkflowActionService", () => { expect(injectedService).toBeTruthy(); })); + it("destroys the shared model when its injector is destroyed", () => { + const destroy = vi.spyOn(texeraGraph.sharedModel, "destroy"); + + TestBed.resetTestingModule(); + + // the shared model's provider runs timers that would otherwise keep this service alive + expect(destroy).toHaveBeenCalledTimes(1); + }); + // The operator canvas and the Form View hand one open workflow between them; the arriving view // asks this before loading, because seeding a second document for the same workflow would leave // the co-editing room and rejoin it. diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts index f73b97c89af..2aa8540be82 100644 --- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts +++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts @@ -17,7 +17,7 @@ * under the License. */ -import { Injectable } from "@angular/core"; +import { Injectable, OnDestroy } from "@angular/core"; import * as joint from "jointjs"; import * as Y from "yjs"; @@ -93,7 +93,7 @@ export const DEFAULT_WORKFLOW = { @Injectable({ providedIn: "root", }) -export class WorkflowActionService { +export class WorkflowActionService implements OnDestroy { private readonly texeraGraph: WorkflowGraph; private readonly jointGraph: joint.dia.Graph; private readonly jointGraphWrapper: JointGraphWrapper; @@ -174,6 +174,12 @@ export class WorkflowActionService { this.handleJointElementDrag(); } + // A root service is only destroyed with its injector: on app teardown, and after every TestBed test. + // Left open, the shared model's provider timers keep this service, and everything it reaches, alive. + ngOnDestroy(): void { + this.destroySharedModel(); + } + private observeContentMeta(): void { // Detach the observer from the previous shared model before attaching to the new one, so // re-attaching on each opened workflow does not stack listeners on the same map. diff --git a/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts b/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts index e6176767909..c430ba26ade 100644 --- a/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts +++ b/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts @@ -17,7 +17,7 @@ * under the License. */ -import { discardPeriodicTasks, fakeAsync, flushMicrotasks, TestBed, tick } from "@angular/core/testing"; +import { fakeAsync, flushMicrotasks, TestBed, tick } from "@angular/core/testing"; import { Subscription } from "rxjs"; import { WorkflowWebsocketService, @@ -540,12 +540,26 @@ describe("WorkflowWebsocketService", () => { tick(WS_HEARTBEAT_INTERVAL_MS); expect(heartbeats()).toBe(2); } finally { - heartbeatService.closeWebsocket(); - // the constructor's interval is never unsubscribed, so drain it or fakeAsync fails the test - discardPeriodicTasks(); + heartbeatService.ngOnDestroy(); } })); + it("stops the heartbeat and closes the socket when destroyed", fakeAsync(() => { + // built inside the fake zone for the same reason as above + const heartbeatService = new WorkflowWebsocketService(TestBed.inject(GuiConfigService)); + heartbeatService.openWebsocket(1, 1, 1); + flushMicrotasks(); + const send = vi.spyOn(heartbeatService, "send"); + + heartbeatService.ngOnDestroy(); + tick(WS_HEARTBEAT_INTERVAL_MS * 2); + + // A heartbeat to a closed socket is dropped without a trace, so watch the attempt: a timer + // still firing after destroy keeps the service, and everything it reaches, alive. + expect(send).not.toHaveBeenCalled(); + expect(openedSockets[0].readyState).toBe(FakeWebSocket.CLOSED); + })); + it("adopts the worker count carried by a ClusterStatusUpdateEvent", async () => { service.openWebsocket(1, 1, 1); await Promise.resolve(); diff --git a/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts b/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts index 9aba41d253e..b9b1cdc5d8d 100644 --- a/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts +++ b/frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts @@ -17,7 +17,7 @@ * under the License. */ -import { Injectable } from "@angular/core"; +import { Injectable, OnDestroy } from "@angular/core"; import { BehaviorSubject, concatWith, @@ -51,7 +51,7 @@ export const WS_RECONNECT_INTERVAL_MS = 3000; @Injectable({ providedIn: "root", }) -export class WorkflowWebsocketService { +export class WorkflowWebsocketService implements OnDestroy { private static readonly TEXERA_WEBSOCKET_ENDPOINT = "wsapi/workflow-websocket"; public numWorkers: number = -1; @@ -61,10 +61,16 @@ export class WorkflowWebsocketService { private statusUpdateSubscription?: Subscription; private readonly webSocketResponseSubject: Subject = new Subject(); private readonly connectionStatusSubject = new BehaviorSubject(false); + private readonly heartbeatSubscription: Subscription; constructor(private config: GuiConfigService) { // setup heartbeat - interval(WS_HEARTBEAT_INTERVAL_MS).subscribe(_ => this.send("HeartBeatRequest", {})); + this.heartbeatSubscription = interval(WS_HEARTBEAT_INTERVAL_MS).subscribe(_ => this.send("HeartBeatRequest", {})); + } + + ngOnDestroy(): void { + this.heartbeatSubscription.unsubscribe(); + this.closeWebsocket(); } /** Emit `true` when the socket is up, `false` when it is closed or lost. */ diff --git a/frontend/src/jsdom-selector-cache-setup.ts b/frontend/src/jsdom-selector-cache-setup.ts new file mode 100644 index 00000000000..d917d608e5c --- /dev/null +++ b/frontend/src/jsdom-selector-cache-setup.ts @@ -0,0 +1,47 @@ +/** + * 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. + */ + +/** + * Empties jsdom's CSS-selector cache between tests. + * + * jsdom answers `querySelector` through nwsapi, which memoizes every selector together with the + * nodes it last matched, for the lifetime of the document. The unit-test builder runs a worker's + * spec files against one document, and TestBed queries a fresh `#root` per fixture, so every + * finished fixture, and through its `__ngContext__` the whole TestBed graph, stays cached until the + * worker exits. The tables are emptied in place because jsdom's engine reads the exported objects. + * + * `beforeEach` runs after the previous test's TestBed teardown, whose own queries would otherwise be + * cached again; `afterAll` covers the file's last test. This is a setupFile (see `vitest.config.ts`). + */ +type NwsapiCaches = { selectResolvers: Record; matchResolvers: Record }; + +function resetSelectorCache(): void { + const document = globalThis.document as unknown as Record; + if (!document) return; + // jsdom keeps its implementation object behind the DOM wrapper, under a symbol it does not export. + const implSymbol = Object.getOwnPropertySymbols(document).find(symbol => symbol.description === "impl"); + const nwsapi = implSymbol && document[implSymbol]?._nwsapi; + if (!nwsapi) return; + for (const cache of [nwsapi.selectResolvers, nwsapi.matchResolvers]) { + for (const selector of Object.keys(cache)) delete cache[selector]; + } +} + +beforeEach(resetSelectorCache); +afterAll(resetSelectorCache); diff --git a/frontend/vitest.config.ts b/frontend/vitest.config.ts index 5498e3043c0..12a66fcef52 100644 --- a/frontend/vitest.config.ts +++ b/frontend/vitest.config.ts @@ -32,8 +32,11 @@ export default defineConfig({ globals: true, // Wrap `it`/`test` so each spec body runs inside an Angular ProxyZone, // which Angular's `fakeAsync` requires. Karma+Jasmine installed this - // implicitly; the @angular/build:unit-test path doesn't. - setupFiles: ["src/test-zone-setup.ts"], + // implicitly; the @angular/build:unit-test path doesn't. Then drop + // jsdom's selector cache between tests: the builder runs a worker's spec + // files against one document (`isolate: false`), and the cache otherwise + // keeps every finished fixture's TestBed graph alive until the worker exits. + setupFiles: ["src/test-zone-setup.ts", "src/jsdom-selector-cache-setup.ts"], // Headroom over Vitest's defaults (5s test / 10s hook) for the shared // macos-latest runners, whose wall time swings ~2x run to run: the same // test that takes ~400ms on ubuntu has been observed at 11s+ in a diff --git a/frontend/yarn.lock b/frontend/yarn.lock index 3e5244eaf9a..318efaaffc9 100644 --- a/frontend/yarn.lock +++ b/frontend/yarn.lock @@ -6980,6 +6980,13 @@ __metadata: languageName: node linkType: hard +"@vitest/spy@patch:@vitest/spy@npm%3A4.1.11#~/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch": + version: 4.1.11 + resolution: "@vitest/spy@patch:@vitest/spy@npm%3A4.1.11#~/.yarn/patches/@vitest-spy-npm-4.1.11-ce87517a53.patch::version=4.1.11&hash=f495f3" + checksum: 10c0/8dfbfa67a088a45304dfc5615103e429d58014869bd598db40c4f3f537ab751e82ea215b995fbd6a7447f873efd36fb3192580c3664eee560f7a957276976e41 + languageName: node + linkType: hard + "@vitest/utils@npm:4.1.11": version: 4.1.11 resolution: "@vitest/utils@npm:4.1.11" diff --git a/licenses-3rd-party-code/vitest.txt b/licenses-3rd-party-code/vitest.txt new file mode 100644 index 00000000000..0e5771dddf4 --- /dev/null +++ b/licenses-3rd-party-code/vitest.txt @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2021-Present VoidZero Inc. and Vitest contributors + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE.