aglinxinyuan commented on code in PR #8178:
URL: https://github.com/apache/texera/pull/8178#discussion_r3896206425
##########
frontend/src/app/workspace/component/codearea-custom-template/codearea-custom-template.component.spec.ts:
##########
@@ -147,6 +147,75 @@ describe("CodeareaCustomTemplateComponent", () => {
expect(remoteComponent.componentRef).toBeDefined();
});
+ it("tears this client's editor down when any co-editor closes one,
whichever operator it names", () => {
+ // The mirror image of the open case above, and the half that was
missing: a co-editor
+ // closing the dialog has to close it here too, or this client keeps
typing into an editor
+ // the other side has already dismissed.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+ remoteComponent.openEditor();
+ expect(remoteComponent.isEditorOpen).toBe(true);
+
+ // The subscriber binds the payload to `_` and discards it, so a
deliberately foreign operator
+ // id is the honest fixture: naming this panel's own operator would
advertise a targeting
+ // filter the component does not have, and would leave a later filter
free to be added without
+ // any test noticing.
+ const destroySpy = vi.spyOn(remoteComponent.componentRef!, "destroy");
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+
+ // The spy is what proves teardown. The shared flag below is reachable
*without* any teardown:
+ // a subscriber that merely published `false` for this operator would
satisfy it through
+ // ngOnInit's getEditorState subscription while the dialog stayed on
screen. vi.spyOn calls
+ // through, so the real destroy still runs and the flag assertions still
describe the result.
+ expect(destroySpy).toHaveBeenCalledTimes(1);
+ expect(remoteComponent.isEditorOpen).toBe(false);
+ let published: boolean | undefined;
+ codeEditorService.getEditorState(highlightedOperatorId()).subscribe(v =>
(published = v));
+ expect(published).toBe(false);
+ });
+
+ it("stays quiet when a co-editor closes an editor this panel never
opened", async () => {
+ // componentRef is still undefined here; the optional call is what keeps
a close broadcast
+ // from throwing in every panel that happens to be mounted but closed. A
close broadcast
+ // reaches EVERY mounted codearea, so most recipients are in exactly
this state.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+
+ // A throw inside a subscriber does NOT propagate out of Subject.next()
- RxJS swallows it and
+ // reports it on its unhandled-error channel one macrotask later. So
`expect(...).not.toThrow()`
+ // would pass even with the optional call removed; watch that channel
instead.
+ const unhandled = vi.fn();
+ const previousHandler = rxjsConfig.onUnhandledError;
+ rxjsConfig.onUnhandledError = unhandled;
+ try {
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+ await new Promise(resolve => setTimeout(resolve, 0));
+ } finally {
+ rxjsConfig.onUnhandledError = previousHandler;
+ }
Review Comment:
Applied in part, in `7fab99fa`. The existing `try`/`finally` is now backed
by an `onTestFinished(...)` registered *before* the swap, which covers the one
exit the `finally` does not. I did not hoist the swap into
`beforeEach`/`afterEach` — that widens the window from a single test to the
whole file.
On the parallel-spec-files half: not reachable here, but **my first stated
reason for that was wrong and I corrected it in `59d5bbcc`**. I had written
that each spec file gets its own module registry. It does not — the Angular
unit-test builder defaults `isolate: false`, so one worker shares a *single*
registry, and therefore one `rxjs` `config` object, across the files it runs.
The conclusion stands (the handler is restored before the test ends, now twice
over) but the mechanism is the opposite of what I claimed, and a wrong
rationale sitting in the spec as a comment is worse than none.
##########
frontend/src/app/workspace/component/codearea-custom-template/codearea-custom-template.component.spec.ts:
##########
@@ -147,6 +147,75 @@ describe("CodeareaCustomTemplateComponent", () => {
expect(remoteComponent.componentRef).toBeDefined();
});
+ it("tears this client's editor down when any co-editor closes one,
whichever operator it names", () => {
+ // The mirror image of the open case above, and the half that was
missing: a co-editor
+ // closing the dialog has to close it here too, or this client keeps
typing into an editor
+ // the other side has already dismissed.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+ remoteComponent.openEditor();
+ expect(remoteComponent.isEditorOpen).toBe(true);
+
+ // The subscriber binds the payload to `_` and discards it, so a
deliberately foreign operator
+ // id is the honest fixture: naming this panel's own operator would
advertise a targeting
+ // filter the component does not have, and would leave a later filter
free to be added without
+ // any test noticing.
+ const destroySpy = vi.spyOn(remoteComponent.componentRef!, "destroy");
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+
+ // The spy is what proves teardown. The shared flag below is reachable
*without* any teardown:
+ // a subscriber that merely published `false` for this operator would
satisfy it through
+ // ngOnInit's getEditorState subscription while the dialog stayed on
screen. vi.spyOn calls
+ // through, so the real destroy still runs and the flag assertions still
describe the result.
+ expect(destroySpy).toHaveBeenCalledTimes(1);
+ expect(remoteComponent.isEditorOpen).toBe(false);
+ let published: boolean | undefined;
+ codeEditorService.getEditorState(highlightedOperatorId()).subscribe(v =>
(published = v));
+ expect(published).toBe(false);
+ });
+
+ it("stays quiet when a co-editor closes an editor this panel never
opened", async () => {
+ // componentRef is still undefined here; the optional call is what keeps
a close broadcast
+ // from throwing in every panel that happens to be mounted but closed. A
close broadcast
+ // reaches EVERY mounted codearea, so most recipients are in exactly
this state.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+
+ // A throw inside a subscriber does NOT propagate out of Subject.next()
- RxJS swallows it and
+ // reports it on its unhandled-error channel one macrotask later. So
`expect(...).not.toThrow()`
+ // would pass even with the optional call removed; watch that channel
instead.
+ const unhandled = vi.fn();
+ const previousHandler = rxjsConfig.onUnhandledError;
+ rxjsConfig.onUnhandledError = unhandled;
+ try {
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+ await new Promise(resolve => setTimeout(resolve, 0));
+ } finally {
+ rxjsConfig.onUnhandledError = previousHandler;
Review Comment:
Refused, with the premise checked. There is no nondeterminism here 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)` is created — and Node
fires equal-delay timers in insertion order, so the report always lands first.
Installing a fake clock would replace one narrow, already-deterministic wait
with control over every timer in the module for the duration of the test. That
is a wider global than the one being removed.
##########
frontend/src/app/workspace/component/codearea-custom-template/codearea-custom-template.component.spec.ts:
##########
@@ -147,6 +147,75 @@ describe("CodeareaCustomTemplateComponent", () => {
expect(remoteComponent.componentRef).toBeDefined();
});
+ it("tears this client's editor down when any co-editor closes one,
whichever operator it names", () => {
+ // The mirror image of the open case above, and the half that was
missing: a co-editor
+ // closing the dialog has to close it here too, or this client keeps
typing into an editor
+ // the other side has already dismissed.
+ const closed = new Subject<{ operatorId: string }>();
+ vi.spyOn(TestBed.inject(CoeditorPresenceService),
"getCoeditorClosedCodeEditorSubject").mockReturnValue(
+ closed.asObservable()
+ );
+
+ const remoteFixture =
TestBed.createComponent(CodeareaCustomTemplateComponent);
+ const remoteComponent = remoteFixture.componentInstance;
+ remoteComponent.field = { props: {}, formControl: new FormControl() } as
any;
+ remoteFixture.detectChanges();
+ remoteComponent.openEditor();
+ expect(remoteComponent.isEditorOpen).toBe(true);
+
+ // The subscriber binds the payload to `_` and discards it, so a
deliberately foreign operator
+ // id is the honest fixture: naming this panel's own operator would
advertise a targeting
+ // filter the component does not have, and would leave a later filter
free to be added without
+ // any test noticing.
+ const destroySpy = vi.spyOn(remoteComponent.componentRef!, "destroy");
+ closed.next({ operatorId: "a-completely-unrelated-operator" });
+
+ // The spy is what proves teardown. The shared flag below is reachable
*without* any teardown:
+ // a subscriber that merely published `false` for this operator would
satisfy it through
+ // ngOnInit's getEditorState subscription while the dialog stayed on
screen. vi.spyOn calls
+ // through, so the real destroy still runs and the flag assertions still
describe the result.
+ expect(destroySpy).toHaveBeenCalledTimes(1);
+ expect(remoteComponent.isEditorOpen).toBe(false);
+ let published: boolean | undefined;
+ codeEditorService.getEditorState(highlightedOperatorId()).subscribe(v =>
(published = v));
+ expect(published).toBe(false);
Review Comment:
Applied in `7fab99fa`. The four identical
`getEditorState(...).subscribe(...)` reads are folded into one
`publishedEditorState(operatorId)` helper that pipes `take(1)`, so each read
completes instead of leaving a live subscription. The operator id stays an
explicit parameter at every call site, because one of them deliberately passes
a *foreign* operator to exercise the co-editor-close path.
--
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]
