You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/08/31 01:07:44 UTC
[PR] test(frontend): close eight small service and component gaps [texera]
aglinxinyuan opened a new pull request, #8178:
URL: https://github.com/apache/texera/pull/8178
### What changes were proposed in this PR?
Eight small frontend gaps, each too small to be worth a PR alone. **+9 fully-covered lines and +4 branch arms.**
| File | Codecov | Closed |
|---|---|---|
| `dashboard-entry.ts` | 117/119 → **119/119** | 205, 206 |
| `settings.component.ts` | 27/28 → **28/28** | 78 |
| `codearea-custom-template.component.ts` | 30/31 → **31/31** | 72 |
| `result-panel-modal.component.html` | 23/24 → **24/24** | 56 |
| `breakpoint-condition-input.component.html` | 8/9 → **9/9** | 27 (arms 0/2 → **2/2**) |
| `hub-workflow-detail.component.html` | 45/46 → **46/46** | 27 |
| `search.component.ts` | 54/56 → 55/56 | 119 |
| `ui-udf-parameters-parser.service.ts` | 135/142 → 136/142 | 364 |
Six of the eight reach 100%.
### Verification
Measured against the **full** suite with no `--include` filter at all — 203 files / 5227 tests before, 203 / 5242 after, both green — because the `DA` line set for a file depends on which specs are in the run, so a filtered before/after pair can fabricate a delta. Parsed from `lcov.info` with Codecov's rule applied directly: a line counts only if its `DA` hit is non-zero **and** every `BRDA` arm on it is taken.
Two reviewers returned ten findings; all repaired. **The repair pass moved coverage by exactly zero** — its three added tests are mutation-strength, not coverage. The builder's figures reproduced line for line.
### Two equivalent mutants, reported rather than papered over
- `ui-udf-parameters-parser.service.ts:364` — `return parts.length ? parts : undefined` → `return parts;` survives the full suite. Both callers of `readMemberPath` treat `[]` and `undefined` identically, so it is not killable. No test was written to pretend otherwise.
- `:349` — relaxing `parts?.length !== 2` to `parts.length < 2` survives for the same reason.
Seven of the sixteen baseline gap lines across these files are given up rather than chased.
Two files touched only by repaired specs are **not** claimed as gains: `result-panel-modal.component.ts` (46/47) and `hub-workflow-detail.component.ts` (98/99) are unchanged.
`yarn format:ci` passes. `frontend/junit.xml` is regenerated by every run, is not gitignored, and is not committed. No production file is touched.
### Any related issues, documentation, discussions?
Closes #8176
### How was this PR tested?
```
npx ng test --watch=false --include="**/ui-udf-parameters-parser.service.spec.ts" --include="**/dashboard-entry.spec.ts" --include="**/search.component.spec.ts" --include="**/settings.component.spec.ts" --include="**/codearea-custom-template.component.spec.ts" --include="**/result-panel-modal.component.spec.ts" --include="**/breakpoint-condition-input.component.spec.ts" --include="**/hub-workflow-detail.component.spec.ts"
```
```
Test Files 8 passed (8)
```
Re-run after rebasing onto current `main`.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(frontend): close eight small service and component gaps [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8178:
URL: https://github.com/apache/texera/pull/8178#issuecomment-5472514726
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/8178?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 93.96%. Comparing base ([`70c2114`](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`38befe9`](https://app.codecov.io/gh/apache/texera/commit/38befe987a3d8548002f6906343006fd3de7d999?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8178 +/- ##
============================================
+ Coverage 93.94% 93.96% +0.01%
Complexity 4759 4759
============================================
Files 1185 1185
Lines 48069 48069
Branches 5359 5359
============================================
+ Hits 45159 45168 +9
+ Misses 1480 1473 -7
+ Partials 1430 1428 -2
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.00% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `99.32% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `89.99% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.67% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `86.86% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.91% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `96.53% <ø> (+0.04%)` | :arrow_up: | |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.31% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.87% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/8178/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.19% <ø> (ø)` | | Carriedforward from [70c2114](https://app.codecov.io/gh/apache/texera/commit/70c21145887920528d7d5540e3fb790b43e8b759?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/8178?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(frontend): close eight small service and component gaps [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8178:
URL: https://github.com/apache/texera/pull/8178#issuecomment-5472461950
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@mengw15`, `@juliethecao`
You can notify them by mentioning `@mengw15`, `@juliethecao` in a comment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(frontend): close eight small service and component gaps [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8178:
URL: https://github.com/apache/texera/pull/8178#discussion_r3893941822
##########
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:
Mutating `rxjsConfig.onUnhandledError` changes a global RxJS setting. If the test runner executes spec files in parallel, another test that triggers an RxJS unhandled error during this window could be captured by `unhandled` and fail this test (or vice versa). Prefer isolating this by running the timer/error-flush deterministically under fake timers (minimizing the window), and/or centralizing the handler swap in `beforeEach/afterEach` for the file so it’s consistently restored even if the test aborts early.
##########
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:
This subscription is never unsubscribed, which can leak across tests if the underlying observable is long-lived (and can make later tests more order-dependent). Prefer a one-shot read (e.g., `take(1)` or converting to a promise via `firstValueFrom`) so the test asserts the current value without leaving a live subscription behind.
##########
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:
The test relies on a real macrotask (`setTimeout(0)`) to flush RxJS’s unhandled-error reporting, which can introduce avoidable nondeterminism and slowdowns across the suite. Consider using Vitest fake timers for this test and advancing timers explicitly so the behavior is deterministic and doesn’t wait on real time.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(frontend): close eight small service and component gaps [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #8178:
URL: https://github.com/apache/texera/pull/8178
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(frontend): close eight small service and component gaps [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(frontend): close eight small service and component gaps [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org