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/30 18:55:49 UTC
[PR] test(frontend): cover the share-access, card-item and list-item components [texera]
aglinxinyuan opened a new pull request, #8133:
URL: https://github.com/apache/texera/pull/8133
### What changes were proposed in this PR?
Five existing dashboard/workspace specs extended, 202 tests to 236.
| File | Codecov lines | Branch arms |
|---|---|---|
| `share-access.component.ts` | 159/169 → **169/169** | 88/98 → **98/98** |
| `share-access.component.html` | 66/73 → **73/73** | 10/10 |
| `user-model.component.ts` | 79/82 → **82/82** | 38/42 → 40/42 |
| `user-model.component.html` | 30/34 → **34/34** | 6/6 |
| `user-workflow-list-item.component.ts` | 36/41 → **41/41** | 22/24 → **24/24** |
| `user-workflow-list-item.component.html` | 68/71 → **71/71** | 4/4 |
| `card-item.component.ts` | 181/190 → 188/190 | 95/102 → 100/102 |
| `card-item.component.html` | 103/106 → 105/106 | 10/14 → 12/14 |
| `workspace.component.ts` | 129/133 → 130/133 | 35/42 → 36/42 |
**+42 fully-covered lines and +22 branch arms** (851/913 → 893/913). Six files reach 100%. The plain line-hit metric moves +21 — half the Codecov gain — because many of these lines were already executed and flip only by completing a branch arm; the two numbers are not interchangeable and both are given.
### The headline target contributes nothing, and that is the main finding
`workspace.component.html` shows **0 of 14 lines** on Codecov and was the reason this bundle was picked. It stays at 0/14.
`workspace.component.spec.ts:164` calls `TestBed.overrideComponent(WorkspaceComponent, { set: { imports: [], providers: [], schemas: [CUSTOM_ELEMENTS_SCHEMA] } })`. That forces a JIT recompile, so the istanbul-instrumented AOT template function is never executed — measured, not inferred: the spec runs 24 tests green and every one of the 15 statements in the template reads `hits=0`, along with the `@ViewChild` view-query and the `beforeunload` host-binding handler at `FNDA:0`.
Unblocking it means providing each child's transitive dependencies. The eight child components declare **62 constructor dependencies** between them (workflow-editor 19, menu 22, result-panel 9), before their `providedIn: 'root'` services — JointJS paper init, Monaco, the websocket services. Overriding each *child* instead would keep the parent template instrumented but still construct each child class and run its `ngAfterViewInit` against DOM an emptied template does not have. `vitest.config.ts` already documents this suite OOM-ing workers at 2 GB (#7975). Refused as an accepted limitation rather than attempted.
### A trap that would have wasted the work
Angular's generated listener source-spans **chain from the end of the previous listener attribute**, so a zero-hit line in an `.html` file is frequently not the handler written on that line.
`card-item.component.html:105` reads as `(keydown.enter)="confirmUpdateCustomName(entry.name)"`, and an existing passing test already dispatches exactly that key event — yet the line showed zero. The `statementMap` explains it: the `keydown.enter` function is declared at `104:56` and is already covered; line 105's zero statement is the `(click)="$event.stopPropagation()"` handler. Every `.html` line in this PR was planned from `--coverage-reporters=json` and the `statementMap`/`fnMap`, never from the template source.
### Verification
**50 semantic mutants, all 50 killed.** Each applied one at a time, with a sha256 snapshot check, an empty-production-diff check and a "no non-spec file modified" check before every compile, reverted from the scratch snapshot and re-verified after. The builder's 31-mutant table was re-derived from scratch rather than carried over, with every kill credit re-read from the run log.
Two reviewers returned ten findings; all repaired. **The repair pass moved coverage by exactly zero** — its three new tests and ~15 new assertions exercise lines the bundle already reached. What changed is that those lines are now *pinned* rather than merely executed, and that is stated rather than sold as extra coverage.
One further trap confirmed and worth carrying: the `DA` line set for a file depends on which specs are in the run, so the before and after measurements must use an identical `--include` filter. Both runs here used the same five spec files.
`card-item.component.ts:310` sits inside a `setTimeout` arrow body — istanbul instruments arrow bodies, so unlike the Scala modules' `$anonfun` rule it does count, but the test must not call `detectChanges` after `onEditDescription()` or the `@ViewChild` resolves and the false arm is lost. It uses `fakeAsync` + `tick(0)`.
`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 #8131
### How was this PR tested?
```
npx ng test --watch=false --include="**/share-access.component.spec.ts" --include="**/card-item.component.spec.ts" --include="**/user-model.component.spec.ts" --include="**/user-workflow-list-item.component.spec.ts" --include="**/workspace.component.spec.ts"
```
```
Test Files 5 passed (5)
```
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): cover the share-access, card-item and list-item components [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8133:
URL: https://github.com/apache/texera/pull/8133#issuecomment-5470622309
<!-- 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:** `@tanishqgandhi1908`, `@zyratlo`
You can notify them by mentioning `@tanishqgandhi1908`, `@zyratlo` 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): cover the share-access, card-item and list-item components [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #8133:
URL: https://github.com/apache/texera/pull/8133#issuecomment-5479746349
Thanks for the review — three applied, two refused, and one of the applications turned out to cost something I had to put back. Pushed as `9154c653` and `3a7d25f4`.
**Applied.**
- **`share-access:740`** (DOM-order-dependent `[privateButton, publicButton]` destructure) — added an `accessButton("Private" | "Public")` helper that filters `button.access-button` by its visible `.button-text-header` text and asserts the match is unique. **No `data-testid`** — that label already exists in the shipped template.
- **`share-access:75`/`:100`** (shared mutable `fixture`) — applied in the narrow sense: the shared `fixture` is now cleared in the existing `beforeEach`, alongside the `TestBed.resetTestingModule()` already there, with a note on the declaration.
- **`user-workflow-list-item:507`** (tooltip-text selection) — added `onlyByTooltip(pred)`, which wraps the pre-existing `byTooltip()` and asserts exactly one match, and used it at the three sites this PR introduces.
**The label refactor dropped a mutation kill, and that's worth flagging.** Replacing the positional destructure with a label lookup removed the DOM-**order** constraint that the destructure had been carrying implicitly — a mutant swapping the two buttons in the template survived. Rather than revert the selector, I added a test that reads the two `.button-text-header` labels in document order and asserts `["Private", "Public"]`, so order and identity now fail independently. Net effect: the fragility you flagged is gone *and* the ordering contract is explicit instead of accidental.
**Refused: moving `vi.useFakeTimers()` after `await createFixture()`.** `frontend/vitest.config.ts` sets no `fakeTimers` block, so the default `toFake` list applies — `setTimeout`/`setInterval`/`setImmediate`/`Date`, but **not** `queueMicrotask` or `process.nextTick`. The only `await` inside `createFixture` is `TestBed.configureTestingModule(...).compileComponents()`, which resolves on the microtask queue. So installing the clock first can't interfere with fixture setup, and it *does* keep any timer the component schedules during creation on the fake clock — which is the more hermetic order, not the less.
**Refused: `setupComponent` returning `{ fixture, component }`.** The stated benefit is future parallelisation, and it isn't available here: the Angular unit-test builder forks one Vitest worker per spec **file**, and specs within a file run sequentially in a single module registry. Returning the fixture would change nothing about concurrency, leaving a style preference that touches every call site in a 780-line spec.
**Refused: `data-testid` / accessible-label hooks.** Both are production template edits, and this PR is test-only with an empty production diff — which is the basis it was approved on. Where a stable non-production hook already existed (`share-access`'s label text) the underlying concern *was* addressed. For the history action the only alternative is `nzType="history"`, which is a template attribute exactly like the tooltip and pins the glyph rather than the text a user actually reads — not more stable, just different.
5 spec files pass, `yarn format:ci` clean, production diff empty and independently re-checked at the merge base.
--
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): cover the share-access, card-item and list-item components [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #8133:
URL: https://github.com/apache/texera/pull/8133#discussion_r3896208652
##########
frontend/src/app/workspace/component/workspace.component.spec.ts:
##########
@@ -380,6 +380,48 @@ describe("WorkspaceComponent", () => {
vi.useRealTimers();
}
});
+
+ it("does not persist an edit made by a signed-out visitor", async () => {
+ // A guest can still edit the canvas; persisting on their behalf would write to whatever
+ // workflow id the URL happens to carry.
+ vi.useFakeTimers();
+ try {
+ const workflowChanged$ = new Subject<void>();
+ await createFixture();
Review Comment:
Refused, with the premise checked against this runner's config. `frontend/vitest.config.ts` sets no `fakeTimers` block, so Vitest's default `toFake` list applies: `setTimeout`/`clearTimeout`, `setInterval`/`clearInterval`, `setImmediate`/`clearImmediate` and `Date` — **not** `queueMicrotask` and not `process.nextTick`. The only `await` inside `createFixture` is `TestBed.configureTestingModule(...).compileComponents()`, which resolves on the microtask queue.
So the clock cannot interfere with fixture setup, and installing it first *does* keep any timer the component schedules during creation on the fake clock. The current order is the more hermetic one.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -711,4 +719,266 @@ describe("ShareAccessComponent", () => {
expect(datasetServiceSpy.updateDatasetPublicity).not.toHaveBeenCalled();
});
});
+
+ /**
+ * Everything above drives the component's methods directly. The template decides which control
+ * reaches which of those methods, and with what argument — the publish pair and the per-row
+ * access controls are near-symmetric, so a crossed binding would look right on screen and do the
+ * opposite thing.
+ */
+ describe("template wiring", () => {
+ /** Makes the current user the owner, which is what enables the write-gated controls. */
+ function asOwner(): void {
+ accessServiceSpy.getOwner.mockReturnValue(of("me@example.com"));
+ }
+
+ it("puts the unpublish confirmation behind Private and the publish confirmation behind Public", () => {
+ asOwner();
+ workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private"));
+ setupComponent({ type: "workflow" });
+
+ const [privateButton, publicButton] = fixture.debugElement.queryAll(By.css("button.access-button"));
Review Comment:
Applied in `9154c653` — and it cost something I had to put back in `3a7d25f4`, which is worth recording here.
The destructure is replaced by an `accessButton("Private" | "Public")` helper that filters `button.access-button` by its visible `.button-text-header` text and asserts the match is unique. No `data-testid` was added; that label already exists in the shipped template.
**But the positional destructure had been carrying a DOM-order constraint implicitly**, and dropping it let a mutant that swaps the two buttons in the template survive. Rather than revert the selector, I added a test that reads the two labels in document order and asserts `["Private", "Public"]`. Order and identity now fail independently, so the fragility you flagged is gone *and* the ordering contract is explicit instead of accidental.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -66,6 +67,10 @@ describe("ShareAccessComponent", () => {
let workflowActionSpy: { setWorkflowIsPublished: ReturnType<typeof vi.fn> };
let userServiceCurrentEmail: string | undefined;
let capturedModalConfigs: any[];
+ /** The NzModalRef stubs handed back by modalService.create, in creation order. */
+ let capturedModalRefs: { close: ReturnType<typeof vi.fn> }[];
+ /** The fixture built by the most recent setupComponent() call, for the template-level tests. */
+ let fixture: ComponentFixture<ShareAccessComponent>;
function setupComponent(opts: SetupOptions = {}): ShareAccessComponent {
Review Comment:
Applied in the narrow sense, in `9154c653`: the shared `fixture` is now cleared in the existing `beforeEach`, alongside the `TestBed.resetTestingModule()` already there, with a note on the declaration.
The `setupComponent` → `{ fixture, component }` refactor is refused. Its stated benefit is future parallelisation, and that is not available here: the Angular unit-test builder forks one Vitest worker per spec **file**, and specs within a file run sequentially against a single module registry. Returning the fixture would change nothing about concurrency, leaving a style change that touches every call site in a 780-line spec.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -92,14 +97,15 @@ describe("ShareAccessComponent", () => {
{ provide: WorkflowActionService, useValue: workflowActionSpy },
],
});
- const fixture = TestBed.createComponent(ShareAccessComponent);
+ fixture = TestBed.createComponent(ShareAccessComponent);
Review Comment:
Same thread as the comment at line 79 — see the reply there. Short version: the shared `fixture` is now cleared in `beforeEach` (`9154c653`), and the `{ fixture, component }` return is refused because one worker runs one spec file's tests sequentially, so there is no concurrency for it to unlock.
##########
frontend/src/app/dashboard/component/user/user-workflow/user-workflow-list-item/user-workflow-list-item.component.spec.ts:
##########
@@ -434,6 +493,37 @@ describe("UserWorkflowListItemComponent rendering", () => {
expect(deleted).toHaveBeenCalledTimes(1);
expect(duplicated).toHaveBeenCalledTimes(1);
});
+
+ it("opens the executions modal from the history action", async () => {
+ await setup({ executionsTracking: true });
+ render(makeWorkflowEntry({ wid: 11, name: "wf" }));
+ const modal = TestBed.inject(NzModalService);
+ const create = vi.spyOn(modal, "create").mockReturnValue({} as any);
+
+ byTooltip(t => t.startsWith("Executions of the workflow"))[0].click();
+
+ expect(create).toHaveBeenCalledWith(
+ expect.objectContaining({ nzContent: WorkflowExecutionHistoryComponent, nzData: { wid: 11 } })
+ );
Review Comment:
Applied in part, in `9154c653`. Added `onlyByTooltip(pred)` — it wraps the pre-existing `byTooltip()` and asserts exactly one match — and used it at the three sites this PR introduces, so a copy change that makes the predicate ambiguous now fails loudly instead of silently picking the first hit.
The `data-testid` / accessible-label suggestion is refused: both are production template edits, and this PR is test-only with an empty production diff, which is the basis it was approved on. The one non-production alternative is `nzType="history"`, and it is not more stable — it is a template attribute exactly like the tooltip, and it pins the glyph rather than the text a user actually reads.
--
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): cover the share-access, card-item and list-item components [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8133:
URL: https://github.com/apache/texera/pull/8133#discussion_r3894098925
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -66,6 +67,10 @@ describe("ShareAccessComponent", () => {
let workflowActionSpy: { setWorkflowIsPublished: ReturnType<typeof vi.fn> };
let userServiceCurrentEmail: string | undefined;
let capturedModalConfigs: any[];
+ /** The NzModalRef stubs handed back by modalService.create, in creation order. */
+ let capturedModalRefs: { close: ReturnType<typeof vi.fn> }[];
+ /** The fixture built by the most recent setupComponent() call, for the template-level tests. */
+ let fixture: ComponentFixture<ShareAccessComponent>;
function setupComponent(opts: SetupOptions = {}): ShareAccessComponent {
Review Comment:
Storing `fixture` in shared mutable state increases coupling between tests and makes future parallelization/concurrency harder. A more maintainable pattern is for `setupComponent` to return both `{ fixture, component }` (or return the fixture and read `fixture.componentInstance`), so each test keeps its own local references without relying on cross-test globals.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -711,4 +719,266 @@ describe("ShareAccessComponent", () => {
expect(datasetServiceSpy.updateDatasetPublicity).not.toHaveBeenCalled();
});
});
+
+ /**
+ * Everything above drives the component's methods directly. The template decides which control
+ * reaches which of those methods, and with what argument — the publish pair and the per-row
+ * access controls are near-symmetric, so a crossed binding would look right on screen and do the
+ * opposite thing.
+ */
+ describe("template wiring", () => {
+ /** Makes the current user the owner, which is what enables the write-gated controls. */
+ function asOwner(): void {
+ accessServiceSpy.getOwner.mockReturnValue(of("me@example.com"));
+ }
+
+ it("puts the unpublish confirmation behind Private and the publish confirmation behind Public", () => {
+ asOwner();
+ workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private"));
+ setupComponent({ type: "workflow" });
+
+ const [privateButton, publicButton] = fixture.debugElement.queryAll(By.css("button.access-button"));
Review Comment:
This assertion depends on DOM order (`[privateButton, publicButton]`), which is brittle and can break with harmless template refactors or styling/layout changes. Make the selection resilient by locating the buttons by their visible text/label, aria-label, or adding a dedicated test id (e.g., data-testid) so the test asserts the intended mapping (Private vs Public) rather than their position.
##########
frontend/src/app/workspace/component/workspace.component.spec.ts:
##########
@@ -380,6 +380,48 @@ describe("WorkspaceComponent", () => {
vi.useRealTimers();
}
});
+
+ it("does not persist an edit made by a signed-out visitor", async () => {
+ // A guest can still edit the canvas; persisting on their behalf would write to whatever
+ // workflow id the URL happens to carry.
+ vi.useFakeTimers();
+ try {
+ const workflowChanged$ = new Subject<void>();
+ await createFixture();
Review Comment:
Using fake timers before `await createFixture()` can make Angular’s async compilation/test setup brittle (fake timers can interfere with async scheduling during `compileComponents()` and other setup work). Prefer moving `vi.useFakeTimers()` to after `await createFixture()` (and after any async setup), so only the debounce/timeout behavior under test is timer-controlled.
##########
frontend/src/app/dashboard/component/user/user-workflow/user-workflow-list-item/user-workflow-list-item.component.spec.ts:
##########
@@ -434,6 +493,37 @@ describe("UserWorkflowListItemComponent rendering", () => {
expect(deleted).toHaveBeenCalledTimes(1);
expect(duplicated).toHaveBeenCalledTimes(1);
});
+
+ it("opens the executions modal from the history action", async () => {
+ await setup({ executionsTracking: true });
+ render(makeWorkflowEntry({ wid: 11, name: "wf" }));
+ const modal = TestBed.inject(NzModalService);
+ const create = vi.spyOn(modal, "create").mockReturnValue({} as any);
+
+ byTooltip(t => t.startsWith("Executions of the workflow"))[0].click();
+
+ expect(create).toHaveBeenCalledWith(
+ expect.objectContaining({ nzContent: WorkflowExecutionHistoryComponent, nzData: { wid: 11 } })
+ );
Review Comment:
Selecting UI elements by tooltip text is fragile (copy changes, i18n, punctuation changes). Prefer selecting the history action by a stable hook (data-testid), an accessible label, or a component/directive selector so the test validates behavior without being tightly coupled to user-facing wording.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -92,14 +97,15 @@ describe("ShareAccessComponent", () => {
{ provide: WorkflowActionService, useValue: workflowActionSpy },
],
});
- const fixture = TestBed.createComponent(ShareAccessComponent);
+ fixture = TestBed.createComponent(ShareAccessComponent);
Review Comment:
Storing `fixture` in shared mutable state increases coupling between tests and makes future parallelization/concurrency harder. A more maintainable pattern is for `setupComponent` to return both `{ fixture, component }` (or return the fixture and read `fixture.componentInstance`), so each test keeps its own local references without relying on cross-test globals.
--
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): cover the share-access, card-item and list-item components [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #8133:
URL: https://github.com/apache/texera/pull/8133
--
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