Copilot commented on code in PR #6600:
URL: https://github.com/apache/texera/pull/6600#discussion_r3613769673
##########
frontend/src/test-zone-setup.ts:
##########
@@ -23,20 +23,57 @@
* call chain, Angular's `fakeAsync` throws
* `Expected to be running in 'ProxyZone'`.
*
- * Wrap Vitest's `it` so each test body runs inside a freshly-forked
- * ProxyZone. This is a setupFile (referenced from `vitest.config.ts`),
- * so it executes once per test file before any spec body runs.
+ * Wrap Vitest's `it` so each test body runs inside a ProxyZone. The proxy
+ * is forked ONCE from the ROOT zone and reused for every test, with its
+ * delegate reset between tests — the same shape zone.js uses for its own
+ * jasmine/jest integrations and its shared-proxy helper.
+ *
+ * Forking from `Zone.root` (rather than `Zone.current`, as before) is what
+ * keeps the proxy exactly one level deep. When an async spec resolves, its
+ * continuation can leave the forked ProxyZone as `Zone.current`; the next
+ * `Zone.current.fork(...)` then nested a proxy inside that one, and across
+ * the many spec files a Vitest worker runs the chain grew without bound.
+ * Every `zone.run()` recurses through the whole
+ * `ProxyZoneSpec.onInvoke -> _ZoneDelegate.invoke` delegate chain, so once
+ * it is deep enough the stack overflows with `RangeError: Maximum call
+ * stack size exceeded` in whichever unrelated spec happens to be running.
+ * See apache/texera#6593.
+ *
+ * This is a setupFile (referenced from `vitest.config.ts`), so it executes
+ * once per test file before any spec body runs.
*/
import "zone.js/testing";
+type ProxyZone = { run: <T>(fn: () => T) => T };
+type ProxyZoneSpecInstance = { resetDelegate: () => void };
+
type ZoneType = {
- current: { fork: (spec: object) => { run: <T>(fn: () => T) => T } };
- ProxyZoneSpec: new () => object;
+ root: { fork: (spec: object) => ProxyZone };
+ ProxyZoneSpec: new () => ProxyZoneSpecInstance;
};
declare const Zone: ZoneType;
-const ProxyZoneSpec = (Zone as unknown as { ProxyZoneSpec: new () => object
}).ProxyZoneSpec;
+const ProxyZoneSpec = Zone.ProxyZoneSpec;
+
+// Fork a single ProxyZone from the root zone and reuse it for every test.
+let sharedProxyZoneSpec: ProxyZoneSpecInstance | null = null;
+let sharedProxyZone: ProxyZone | null = null;
+
+function getProxyZone(): ProxyZone {
+ let spec = sharedProxyZoneSpec;
+ let zone = sharedProxyZone;
+ if (!spec || !zone) {
+ spec = new ProxyZoneSpec();
+ zone = Zone.root.fork(spec);
+ sharedProxyZoneSpec = spec;
+ sharedProxyZone = zone;
+ }
+ // Clear any delegate a prior test (e.g. one that threw inside fakeAsync)
+ // may have left set, so each test starts from a clean proxy state.
+ spec.resetDelegate();
+ return zone;
+}
Review Comment:
Reusing a single shared ProxyZone across all tests introduces a race if any
tests run concurrently (e.g., via `it.concurrent` / `test.concurrent`): one
test can reset or replace the ProxyZone delegate while another test is still
executing inside it. To make this robust under concurrency, prefer forking a
*fresh* ProxyZone from `Zone.root` per test (still bounded depth), or
explicitly ensure concurrent tests are wrapped with an isolated proxy (or
disallowed) so delegates cannot collide.
##########
frontend/src/test-zone-setup.ts:
##########
@@ -47,7 +84,7 @@ function wrapInProxyZone<T extends ItFn>(target: T): T {
name,
function wrapper(this: unknown, ...args: unknown[]) {
return new Promise<void>((resolve, reject) => {
- const zone = Zone.current.fork(new ProxyZoneSpec());
+ const zone = getProxyZone();
Review Comment:
Reusing a single shared ProxyZone across all tests introduces a race if any
tests run concurrently (e.g., via `it.concurrent` / `test.concurrent`): one
test can reset or replace the ProxyZone delegate while another test is still
executing inside it. To make this robust under concurrency, prefer forking a
*fresh* ProxyZone from `Zone.root` per test (still bounded depth), or
explicitly ensure concurrent tests are wrapped with an isolated proxy (or
disallowed) so delegates cannot collide.
##########
frontend/src/test-zone-setup.spec.ts:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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 { fakeAsync, tick, waitForAsync } from "@angular/core/testing";
+
+/**
+ * Guards the ProxyZone that `test-zone-setup.ts` installs around every spec.
+ *
+ * The flake it fixes (RangeError from unbounded proxy nesting, see
+ * apache/texera#6593) only surfaces once enough spec files have run on one
+ * Vitest worker, so it is not reproducible from a single spec. These tests
+ * instead pin the invariant that prevents it: the proxy is forked directly
+ * from the root zone (exactly one level deep) and stays functional for
+ * Angular's fakeAsync/waitForAsync, both of which require an active ProxyZone.
+ */
+
+// `Zone` is a global installed by `zone.js/testing`. Declare the slice used
here.
+declare const Zone: {
+ root: unknown;
+ current: { parent: unknown; get: (key: string) => unknown };
+};
+
+describe("test-zone-setup", () => {
+ it("runs each spec body inside a ProxyZone forked directly from the root
zone", () => {
Review Comment:
This spec relies on the global Vitest configuration to have already loaded
`test-zone-setup.ts` as a setup file. That makes the test less self-contained
and can fail in alternative runners/configurations (or if `setupFiles` ordering
changes). Consider explicitly importing the setup module in this spec (and
making `test-zone-setup.ts` idempotent via a global guard to avoid
double-wrapping) or at least adding an explicit assertion/message that the
setup file is expected to be loaded via Vitest config.
--
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]
