Yicong-Huang commented on code in PR #5241: URL: https://github.com/apache/texera/pull/5241#discussion_r3309001803
########## frontend/src/browser-buffer-polyfill.ts: ########## @@ -0,0 +1,44 @@ +/** + * 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. + */ + +// monaco-editor-wrapper + monaco-vscode-files-service-override dereference +// Node's Buffer (including Buffer.allocUnsafe) at module-evaluation time. +// Under jsdom-mode Vitest, the test runs on Node and Buffer is built-in. In +// browser-mode (Playwright/Chromium), Buffer doesn't exist, so importing +// monaco-editor-wrapper crashes at load. The `buffer` npm package is the +// browser polyfill of Node's API; expose its `Buffer` (and a minimal +// `process` shim) on the global so monaco's internals find them at the same +// names they'd find Node's. Has to run before any monaco import is +// evaluated, which is why this is wired into vitest.browser.config.ts as the +// FIRST setupFile — setupFiles run in order, before any test module loads. +// +// We use a namespace import (rather than a named or default import) because +// Vite's dep-optimizer rewrites `buffer@5`'s CJS exports in a way that +// exposes neither a `Buffer` named export nor a `default` export at +// module-eval time. The namespace object always has the optimizer-injected +// shape, so we read `Buffer` off it dynamically. +import * as bufferModule from "buffer"; Review Comment: I also have this concern. In addition, why do we need to have a polyfill file for buffer? ########## frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.browser.spec.ts: ########## @@ -0,0 +1,323 @@ +/** + * 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. + */ + +// Browser-mode companion to code-editor.component.spec.ts. The sibling jsdom +// spec covers the constructor, language detection, getFileSuffixByLanguage, +// onFocus, getCoeditorCursorStyles, and the accept/reject annotation paths, +// but cannot reach anything gated on a real Monaco editor — the +// `initAndStart` subscribe body, `initializeDiffEditor`, AI-action run +// callbacks, `handleTypeAnnotation`'s position branch, and the resize +// handler. This spec drives those by swapping the component's editorWrapper +// for a fake-with-real-DOM and running in vitest's Playwright/Chromium +// browser mode, where monaco-editor's codingame fork can be imported without +// jsdom's missing-canvas / Node-Buffer-allocation tripwires (see the +// nodePolyfills entry in vitest.browser.config.ts). + +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { FormControl } from "@angular/forms"; +import { BehaviorSubject, of } from "rxjs"; +import * as Y from "yjs"; + +import { CodeEditorComponent } from "./code-editor.component"; +import { WorkflowActionService } from "../../service/workflow-graph/model/workflow-action.service"; +import { WorkflowVersionService } from "../../../dashboard/service/user/workflow-version/workflow-version.service"; +import { AIAssistantService } from "../../service/ai-assistant/ai-assistant.service"; +import { OperatorMetadataService } from "../../service/operator-metadata/operator-metadata.service"; +import { StubOperatorMetadataService } from "../../service/operator-metadata/stub-operator-metadata.service"; +import { mockOperatorMetaData } from "../../service/operator-metadata/mock-operator-metadata.data"; +import { mockJavaUDFPredicate, mockPoint } from "../../service/workflow-graph/model/mock-workflow-data"; +import { OperatorSchema } from "../../types/operator-schema.interface"; +import { commonTestProviders } from "../../../common/testing/test-utils"; +import * as monaco from "monaco-editor"; + +// y-monaco's MonacoBinding wires real listeners against the YText and a real +// monaco TextModel. Our fake editor returns a stub model, so the binding's +// constructor would throw on `model.onDidChangeContent(...)`. The component +// only depends on the binding's `destroy()` (called in ngOnDestroy) and the +// fact that the constructor was called with the right shape of args, so a +// recording stub is sufficient. +// `vi.mock` is hoisted to the top of the file, so any closure variables it +// references must be declared inside `vi.hoisted` — a plain top-level `const` +// is evaluated AFTER the mock factory runs, leaving `monacoBindingCalls` +// undefined at the moment MonacoBinding's constructor would try to push. +const { monacoBindingCalls } = vi.hoisted(() => ({ + monacoBindingCalls: [] as unknown[][], +})); +vi.mock("y-monaco", () => ({ + MonacoBinding: class { + constructor(...args: unknown[]) { + monacoBindingCalls.push(args); + } + destroy = vi.fn(); + }, +})); + +// Re-use the augmented stub from the jsdom spec so the component constructor +// can resolve its highlighted operator regardless of operatorType. +const baseSchema = mockOperatorMetaData.operators.find(op => op.operatorType === "PythonUDF"); +if (!baseSchema) { + throw new Error( + "CodeEditorComponent browser spec setup expected a PythonUDF schema in mockOperatorMetaData — fixture has drifted." + ); +} +const synthesizeSchema = (operatorType: string): OperatorSchema => ({ ...baseSchema, operatorType }); +const augmentedSchemas: OperatorSchema[] = [...mockOperatorMetaData.operators, synthesizeSchema("PythonUDFV2")]; +class AugmentedStubMetadataService extends StubOperatorMetadataService { + private readonly augmentedMetadata = of({ ...mockOperatorMetaData, operators: augmentedSchemas }); + override getOperatorMetadata(): typeof this.augmentedMetadata { + return this.augmentedMetadata; + } + override getOperatorSchema(operatorType: string): OperatorSchema { + const schema = augmentedSchemas.find(op => op.operatorType === operatorType); + if (!schema) throw new Error(`unknown operatorType ${operatorType}`); + return schema; + } + override operatorTypeExists(operatorType: string): boolean { + return augmentedSchemas.some(op => op.operatorType === operatorType); + } +} + +// A minimal, recording Monaco editor stand-in. Returns realistic values +// where the component reads from it (`getScrolledVisiblePosition`, +// `getSelection`, `getModel`) and records what the component asks it to do +// (`addAction`, `updateOptions`, `layout`). The component does not introspect +// any of these beyond truthiness, so the stub does not need to be a real +// IStandaloneCodeEditor — TypeScript's structural check is the only gate. +interface FakeEditor { Review Comment: why are we testing a fake editor? -- 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]
