aglinxinyuan commented on PR #8178: URL: https://github.com/apache/texera/pull/8178#issuecomment-5479746619
Thanks for the review — two applied, two refused. Pushed as `7fab99fa` and `59d5bbcc`. **Applied.** - **`:181`** (subscription never unsubscribed) — folded the four identical `getEditorState(...).subscribe(...)` reads into one `publishedEditorState(operatorId)` helper that pipes `take(1)`. The operator id stays an explicit parameter at each call site, since one of them deliberately passes a *foreign* operator to check the co-editor-close path. - **`:209`** (global `rxjsConfig.onUnhandledError` mutated) — applied in the part that has a real failure mode: the existing `try`/`finally` is now backed by an `onTestFinished(...)` registered *before* the swap, covering the one exit the `finally` doesn't. I did **not** hoist the swap into `beforeEach`/`afterEach`, which would widen the window from one test to the whole file. **Refused: switching to Vitest fake timers for the `setTimeout(0)` flush.** There's no nondeterminism to remove. rxjs 7.8.2's `reportUnhandledError` schedules its report via `timeoutProvider.setTimeout(handler)` **synchronously inside `Subject.next()`** — i.e. before the test's own `setTimeout(resolve, 0)` exists — and Node fires equal-delay timers in insertion order, so the report always lands first. Installing a fake clock would replace one narrow, correct wait with control over every timer in the module for the duration of the test. **Refused: the "parallel spec files could observe this mutation" half of `:209`.** Not reachable in this repo, though **my first stated reason for that was wrong and I've since corrected it in the spec.** I originally wrote that each spec file gets its own module registry. It doesn't: the Angular unit-test builder defaults `isolate: false`, so one worker shares a **single** module registry — and therefore one `rxjs` `config` object — across the files it runs. The conclusion holds (the swap is restored before the test ends, twice over now, so no sibling file can observe it) but the mechanism is the opposite of what I said, and a wrong rationale sitting in the spec as a comment is worse than none. Commit `59d5bbcc` is that correction, comment-only. 8 spec files pass, `yarn format:ci` clean, production diff empty. Unrelated to your comments, one figure in the PR body needs a caveat I'll add: the `ui-udf-parameters-parser.service.ts` number was measured against the full suite, not the 8-file `--include` list quoted under "How was this PR tested?", and those two scopes don't yield the same `DA` line set. -- 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]
