This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7992-57a4230b6f744ba009aa0099893da825999f3077 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 97e3585a6ac7aadbea629e7350a96d1a84f7b51c Author: Xinyuan Lin <[email protected]> AuthorDate: Thu Aug 27 04:44:21 2026 +0000 test(frontend): cover the time-travel poller and the feedback component (#7992) ### What changes were proposed in this PR? Three small frontend components, measured from lcov with the same spec filter on both sides. | File | Codecov | lcov detail | |---|---|---| | `time-travel.component.ts` | 92.3% → **100%** | branches 20/22 → 22/22; functions 17/18 → 18/18 | | `feedback.component.ts` | 85.2% → **100%** | branches 14/17 → 17/17; functions 8/11 → 11/11 | | `feedback.component.html` | 83.3% → **100%** | branches 2/4 → 4/4; **functions 0/2 → 2/2** | | `repeat-dnd.component.html` | 86.4% → **95.5%** | one arm left deliberately — see below | | `repeat-dnd.component.ts` (incidental) | 90.9% → **100%** | branches 17/18 → 18/18 | **+14 fully-covered lines, +9 branch arms, +7 functions.** The template's function counter is the one to notice: `feedback.component.html` was at **zero of two functions covered** behind an 83% line figure. That is the fifth time in this campaign the function counter found what the line counter hid. `time-travel.component.ts`'s zero-hit lines 84-88 were the entire `ngOnInit` timer lambda body. **`port-property-edit-frame.component.ts` was in scope and is absent.** It is worth exactly zero: two of its three residual lines are dead Quill config and the third is unreachable in practice. No tests were added there and no mutations run. ### A production defect found while assessing it Chasing those two Quill lines turned up a real bug rather than coverage: the port-name editor's keyboard bindings use `key: 13`, which is **Quill 1 syntax that Quill 2 never dispatches**. So pressing Enter in the port-name editor inserts a newline into the shared display name instead of closing the editor. Reported, not pinned — pinning current behaviour would cement it. ### One arm left uncovered on purpose `repeat-dnd.component.html` line 48 is the **remove** button's `[disabled]="field.templateOptions?.disabled"`, and issue **#7431** records that the enclosing `*ngFor` shadows the outer `field`, so it never disables. That arm is deliberately not pinned. The assertion added in that spec is on the **add** button, and it carries a comment explaining why an absent `templateOptions` object must not read as disabled. ### Verification 28 mutations, **24 killed, 4 recorded.** The first draft's headline was false: it claimed "survivors: NONE, every one of the 14 new tests proven by a mutation actually applied and run". **Eight mutants survived it** — seven found by review plus one found here (deleting an `[nzLoading]` binding). All eight now die. Its survivor-to-mutation mapping was also wrong in one place: two separate mutations had been given the same number, so a kill was credited to the wrong row. The four recorded non-kills are: the #7431 arm above (refused), an equivalent mutant (`[nzData]="[...feedbackList]"` → `[nzData]="feedbackList"` — the spread produces an equal array), and two entries covering `port-property-edit-frame`, where no tests were added. One further correction worth making: `feedback.component.html` reaching 24/24 is **not** a quality claim on its own. At 24/24 the file is fully *executed*; what makes it constrained is the mutations, not the percentage. No production file is touched. The worktree used a real yarn install rather than a `node_modules` junction. ### Any related issues, documentation, discussions? Closes #7990 ### How was this PR tested? ``` npx ng test --watch=false --include="**/time-travel.component.spec.ts" --include="**/feedback.component.spec.ts" --include="**/repeat-dnd.component.spec.ts" ``` ``` Test Files 3 passed (3) ``` `yarn format:ci` passes. `frontend/junit.xml` and `frontend/coverage/` are regenerated by every run and are not committed. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> --- .../formly/repeat-dnd/repeat-dnd.component.spec.ts | 89 ++++++++++- .../user/feedback/feedback.component.spec.ts | 162 ++++++++++++++++++++- .../time-travel/time-travel.component.spec.ts | 65 ++++++++- 3 files changed, 309 insertions(+), 7 deletions(-) diff --git a/frontend/src/app/common/formly/repeat-dnd/repeat-dnd.component.spec.ts b/frontend/src/app/common/formly/repeat-dnd/repeat-dnd.component.spec.ts index 4589fa76ef..6a088a3dc5 100644 --- a/frontend/src/app/common/formly/repeat-dnd/repeat-dnd.component.spec.ts +++ b/frontend/src/app/common/formly/repeat-dnd/repeat-dnd.component.spec.ts @@ -17,7 +17,7 @@ * under the License. */ -import { CdkDragDrop, CdkDragHandle } from "@angular/cdk/drag-drop"; +import { CdkDrag, CdkDragDrop, CdkDragHandle, CdkDropList } from "@angular/cdk/drag-drop"; import { By } from "@angular/platform-browser"; import { FormArray, FormControl } from "@angular/forms"; import { ComponentFixture, TestBed } from "@angular/core/testing"; @@ -84,7 +84,21 @@ describe("FormlyRepeatDndComponent", () => { }); it("should reorder model, fieldGroup, formControl, and call reorder callback", () => { - const reorder = setComponentState(); + // The callback is the parent's cue to persist, so it has to run AFTER all three reorder + // steps: a parent that reads the form when notified would otherwise save the pre-drag + // order and silently discard the drag. Capturing the state from inside the callback is + // what makes that ordering observable — the final assertions below are order-insensitive, + // and moveItemInArray mutates in place, so the captures must be copies. + let seenModel: string[] | undefined; + let seenFieldKeys: unknown[] | undefined; + let seenControls: unknown[] | undefined; + const reorder = setComponentState( + vi.fn(() => { + seenModel = [...(component.model as string[])]; + seenFieldKeys = component.field.fieldGroup?.map(field => field.key); + seenControls = (component.formControl as FormArray).controls.map(control => control.value); + }) + ); component.onDrop(createDropEvent(0, 2)); @@ -92,6 +106,24 @@ describe("FormlyRepeatDndComponent", () => { expect(component.field.fieldGroup?.map(field => field.key)).toEqual(["b", "c", "a"]); expect((component.formControl as FormArray).controls.map(control => control.value)).toEqual(["b", "c", "a"]); expect(reorder).toHaveBeenCalledOnce(); + expect(seenModel).toEqual(["b", "c", "a"]); + expect(seenFieldKeys).toEqual(["b", "c", "a"]); + expect(seenControls).toEqual(["b", "c", "a"]); + }); + + it("still reorders a section that declares no reorder callback", () => { + // The reorder callback is how the parent persists the new order, and it is optional: + // a section rendered without one must still reorder in place rather than throw. + setComponentState(); + component.field = { + ...component.field, + props: {}, + } as any; + + expect(() => component.onDrop(createDropEvent(0, 2))).not.toThrow(); + expect(component.model).toEqual(["b", "c", "a"]); + expect(component.field.fieldGroup?.map(field => field.key)).toEqual(["b", "c", "a"]); + expect((component.formControl as FormArray).controls.map(control => control.value)).toEqual(["b", "c", "a"]); }); /** * The class-level tests above drive onDrop directly and never render. The template owns the rest @@ -130,11 +162,15 @@ describe("FormlyRepeatDndComponent", () => { expect(el.querySelectorAll(".dnd-row").length).toBe(3); }); - it("gives each row a drag handle", () => { - // Asserted on the cdkDragHandle directive, not the .drag-handle class: the class is styling - // and survives the directive being dropped, which would leave the row undraggable. + it("makes each row draggable, with its own drag handle", () => { + // Asserted on the directives, not on the .dnd-row / .drag-handle classes: the classes are + // styling and survive either directive being dropped. Both are needed — cdkDragHandle + // constructs happily with no CdkDrag parent (its CDK_DRAG_PARENT injection is optional), + // so the handle assertion alone passes for a row that cannot be picked up at all, and a + // row that cannot be picked up never fires cdkDropListDropped. render(); + expect(fixture.debugElement.queryAll(By.directive(CdkDrag)).length).toBe(3); expect(fixture.debugElement.queryAll(By.directive(CdkDragHandle)).length).toBe(3); }); @@ -183,5 +219,48 @@ describe("FormlyRepeatDndComponent", () => { expect(addButton().getAttribute("disabled")).toBeNull(); }); + + it("leaves the add button available for a section that declares no template options at all", () => { + // Not `{ disabled: false }`: a schema that says nothing about the repeat section + // produces no templateOptions object, and an absent object must not read as disabled. + setComponentState(); + fixture.detectChanges(); + + expect(component.field.templateOptions).toBeUndefined(); + expect(addButton().getAttribute("disabled")).toBeNull(); + }); + + it("renders each row's own sub-fields", () => { + setComponentState(); + component.field = { + ...component.field, + fieldGroup: [ + { key: "row-0", fieldGroup: [{ key: "row-0-name" }] }, + { key: "row-1", fieldGroup: [{ key: "row-1-name" }] }, + ], + } as any; + fixture.detectChanges(); + + // Asserted on the config each rendered field was actually handed, not on how many + // rendered: binding the row itself instead of its sub-field renders the same count + // of elements and would show up as a pass. + const rendered = fixture.debugElement.queryAll(By.css("formly-field.dnd-field")); + expect(rendered.map(f => (f.componentInstance as { field: { key?: unknown } }).field.key)).toEqual([ + "row-0-name", + "row-1-name", + ]); + }); + + it("forwards a drop on the row list to onDrop", () => { + // The drag-and-drop wiring is the whole point of this variant of the repeat section; + // without the template hookup the rows are draggable but nothing reorders. + const spy = vi.spyOn(component, "onDrop").mockImplementation(() => {}); + render(); + const event = createDropEvent(0, 2); + + fixture.debugElement.query(By.directive(CdkDropList)).triggerEventHandler("cdkDropListDropped", event); + + expect(spy).toHaveBeenCalledWith(event); + }); }); }); diff --git a/frontend/src/app/dashboard/component/user/feedback/feedback.component.spec.ts b/frontend/src/app/dashboard/component/user/feedback/feedback.component.spec.ts index 915ad9dcba..3422a024f1 100644 --- a/frontend/src/app/dashboard/component/user/feedback/feedback.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/feedback/feedback.component.spec.ts @@ -19,7 +19,8 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { HttpClientTestingModule } from "@angular/common/http/testing"; -import { of } from "rxjs"; +import { formatDate } from "@angular/common"; +import { NEVER, of, throwError } from "rxjs"; import { NZ_MODAL_DATA } from "ng-zorro-antd/modal"; import { NzMessageService } from "ng-zorro-antd/message"; @@ -41,6 +42,15 @@ function makeMessageSpy() { return { success: vi.fn(), error: vi.fn(), warning: vi.fn() }; } +/** Feedback fixture. `creationTime` is 2023-11-14T22:13:20Z; `fid`/`uid` stay small so that + * feeding either of them to the date pipe instead would render a 1970 date, which the + * table assertion below can tell apart from the real submission time. */ +const FIXTURE_CREATION_TIME = 1700000000000; + +function makeFeedback(fid: number, message: string): Feedback { + return { fid, uid: 1, message, creationTime: FIXTURE_CREATION_TIME }; +} + describe("FeedbackComponent", () => { describe("own-feedback (page) mode", () => { let component: FeedbackComponent; @@ -89,6 +99,148 @@ describe("FeedbackComponent", () => { expect(component.newFeedback).toBe(""); expect(messageSpy.success).toHaveBeenCalled(); expect(feedbackSpy.getMyFeedback).toHaveBeenCalled(); + // The success arm has to release the box too, not only the failure arm: `submitting` + // drives [nzLoading] on the button and [disabled] on the textarea, so leaving it set + // would lock the user out of sending a second piece of feedback. + expect(component.submitting).toBe(false); + }); + + /** + * Failure paths. Both requests report through the same `extractError` helper, whose job is + * to prefer the server's own explanation over the transport's generic one — so the fixtures + * below deliberately carry BOTH, otherwise the assertion passes either way. + */ + describe("failures", () => { + it("shows the server's own message when the feedback list fails to load", () => { + feedbackSpy.getMyFeedback.mockReturnValue( + throwError(() => ({ + error: { message: "feedback is unavailable" }, + message: "Http failure response for /api/feedback/me: 500 Internal Server Error", + })) + ); + + component.loadFeedback(); + + expect(messageSpy.error).toHaveBeenCalledWith("feedback is unavailable"); + }); + + it("re-enables the submit box and shows the server's own message when submitting fails", () => { + feedbackSpy.submitFeedback.mockReturnValue( + throwError(() => ({ + error: { message: "feedback quota exceeded" }, + message: "Http failure response for /api/feedback: 429 Too Many Requests", + })) + ); + feedbackSpy.getMyFeedback.mockClear(); + component.newFeedback = "one more thing"; + + component.submitFeedback(); + + expect(messageSpy.error).toHaveBeenCalledWith("feedback quota exceeded"); + // The box must not stay locked, and a failed submit must not clear what was typed + // nor reload the list as if it had been accepted. + expect(component.submitting).toBe(false); + expect(component.newFeedback).toBe("one more thing"); + expect(messageSpy.success).not.toHaveBeenCalled(); + expect(feedbackSpy.getMyFeedback).not.toHaveBeenCalled(); + }); + + it("falls back to the transport message when the server sent no body", () => { + feedbackSpy.getMyFeedback.mockReturnValue(throwError(() => new Error("connection refused"))); + + component.loadFeedback(); + + expect(messageSpy.error).toHaveBeenCalledWith("connection refused"); + }); + + it("falls back to a generic message for an error that carries no message at all", () => { + feedbackSpy.getMyFeedback.mockReturnValue(throwError(() => ({}))); + + component.loadFeedback(); + + expect(messageSpy.error).toHaveBeenCalledWith("An unexpected error occurred."); + }); + }); + + /** + * The template owns the rest of the submit flow: the box has to write what was typed back + * into the component, and the button has to be the thing that sends it. + */ + describe("rendered page", () => { + const textarea = () => fixture.nativeElement.querySelector("textarea") as HTMLTextAreaElement; + const submitButton = () => fixture.nativeElement.querySelector(".feedback-submit-button") as HTMLButtonElement; + const rows = () => Array.from(fixture.nativeElement.querySelectorAll("tbody tr") as NodeListOf<HTMLElement>); + + it("submits exactly what was typed into the box", () => { + const box = textarea(); + box.value = "please add dark mode"; + box.dispatchEvent(new Event("input")); + fixture.detectChanges(); + + // The two-way binding has to have written the typed text back to the component, + // which is also what un-disables the button. + expect(component.newFeedback).toBe("please add dark mode"); + expect(submitButton().disabled).toBe(false); + + submitButton().click(); + + expect(feedbackSpy.submitFeedback).toHaveBeenCalledWith("please add dark mode"); + }); + + it("shows the button as loading while the submit is in flight, and idle before it", () => { + // [nzLoading]="submitting" is the only signal the user gets that the submit is under + // way; a button that still looks idle invites a second click on the same feedback. + // nz-button reflects it as a host class, so it is read that way. + expect(submitButton().classList.contains("ant-btn-loading")).toBe(false); + + feedbackSpy.submitFeedback.mockReturnValue(NEVER); + component.newFeedback = "one more thing"; + component.submitFeedback(); + fixture.detectChanges(); + + expect(component.submitting).toBe(true); + expect(submitButton().classList.contains("ant-btn-loading")).toBe(true); + }); + + it("offers the submit box in own-feedback mode", () => { + // Positive control for the admin-mode assertion that the box is absent: without + // this pair, `*ngIf="!isAdminView"` could be widened to a constant and go unnoticed. + expect(fixture.nativeElement.querySelector(".feedback-submit-card")).not.toBeNull(); + expect(textarea()).not.toBeNull(); + }); + + it("keeps the submit button locked while the box is empty", () => { + expect(submitButton().disabled).toBe(true); + }); + + it("keeps the submit button locked for whitespace-only text", () => { + // The guard is `newFeedback.trim().length === 0`; without the trim the button + // un-disables here and the click is then rejected by submitFeedback instead. + const box = textarea(); + box.value = " "; + box.dispatchEvent(new Event("input")); + fixture.detectChanges(); + + // The typed text must have reached the model, or the assertion below is vacuous. + expect(component.newFeedback).toBe(" "); + expect(submitButton().disabled).toBe(true); + }); + + it("renders a row per feedback entry with its submitted time and its message", () => { + feedbackSpy.getMyFeedback.mockReturnValue(of([makeFeedback(1, "please add dark mode")])); + component.loadFeedback(); + fixture.detectChanges(); + + expect(rows().length).toBe(1); + const cells = Array.from(rows()[0].querySelectorAll("td")); + expect(cells.length).toBe(2); + // Compared against the same format independently applied to creationTime rather than + // against a date-shaped regex: a regex passes for any number in that cell, and both + // other numeric fields of the fixture (fid, uid) render as a 1970 date that matches it. + expect(cells[0].textContent?.trim()).toBe(formatDate(FIXTURE_CREATION_TIME, "MM/dd/y, h:mm a", "en-US")); + expect(cells[1].classList.contains("feedback-message")).toBe(true); + expect(cells[1].textContent?.trim()).toBe("please add dark mode"); + }); }); }); @@ -119,5 +271,13 @@ describe("FeedbackComponent", () => { expect(feedbackSpy.getUserFeedback).toHaveBeenCalledWith(42); expect(feedbackSpy.getMyFeedback).not.toHaveBeenCalled(); }); + + it("renders the target user's feedback read-only, with no submit box", () => { + // The read-only half of the two-mode contract. A submit box shown here is wired to + // submitFeedback(), which posts as the admin rather than as the user being inspected. + expect(fixture.nativeElement.querySelector(".feedback-submit-card")).toBeNull(); + expect(fixture.nativeElement.querySelector("textarea")).toBeNull(); + expect(fixture.nativeElement.querySelector(".feedback-submit-button")).toBeNull(); + }); }); }); diff --git a/frontend/src/app/workspace/component/left-panel/time-travel/time-travel.component.spec.ts b/frontend/src/app/workspace/component/left-panel/time-travel/time-travel.component.spec.ts index 90f989b482..ba44232002 100644 --- a/frontend/src/app/workspace/component/left-panel/time-travel/time-travel.component.spec.ts +++ b/frontend/src/app/workspace/component/left-panel/time-travel/time-travel.component.spec.ts @@ -17,7 +17,7 @@ * under the License. */ -import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { ComponentFixture, TestBed, fakeAsync, tick } from "@angular/core/testing"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; import { BrowserAnimationsModule } from "@angular/platform-browser/animations"; import { By } from "@angular/platform-browser"; @@ -231,6 +231,69 @@ describe("TimeTravelComponent", () => { }); }); + /** + * The panel keeps itself up to date from a `timer(0, 5000)` poller started in ngOnInit. + * The fixture built by the outer beforeEach cannot be used to drive it: `src/test-zone-setup.ts` + * installs the ProxyZone that fakeAsync patches around the `it` body only, so anything + * scheduled from a beforeEach — including the poller that beforeEach's detectChanges starts + * (inert there, since the metadata stub reports no wid) — lives in the real zone, out of + * tick()'s reach. These tests therefore re-run ngOnInit from inside the fakeAsync body, + * which is where timer(0, 5000) has to be created for tick() to drive it, and destroy the + * fixture at the end so @UntilDestroy unsubscribes the periodic timer. + */ + describe("ngOnInit polling", () => { + it("skips the refresh while the workflow has no id", fakeAsync(() => { + metadataSpy.mockReturnValue(undefined as any); + const widSpy = vi.spyOn(component, "getWid"); + const displaySpy = vi.spyOn(component, "displayExecutionWithLogs").mockImplementation(() => {}); + + component.ngOnInit(); + tick(0); // the first emission of timer(0, 5000) is asynchronous + + // getWid pins that the poller actually ran: without it the negative assertion + // below would also pass with the poller never firing at all. + expect(widSpy).toHaveBeenCalledTimes(1); + expect(displaySpy).not.toHaveBeenCalled(); + + component.ngOnDestroy(); + })); + + it("refreshes the execution list immediately and then every five seconds", fakeAsync(() => { + metadataSpy.mockReturnValue({ wid: 7 } as any); + const displaySpy = vi.spyOn(component, "displayExecutionWithLogs").mockImplementation(() => {}); + + component.ngOnInit(); + + tick(0); + expect(displaySpy).toHaveBeenCalledTimes(1); + expect(displaySpy).toHaveBeenCalledWith(7); + + // The second advance brackets the period instead of merely clearing it: asserting only + // "two calls by t=5000" holds for every period <= 5000, so a shortened interval — which + // multiplies the panel's load on /api/executions — would pass unnoticed. + tick(4999); + expect(displaySpy).toHaveBeenCalledTimes(1); + tick(1); + expect(displaySpy).toHaveBeenCalledTimes(2); + + fixture.destroy(); + })); + + it("stops polling once the panel is destroyed", fakeAsync(() => { + metadataSpy.mockReturnValue({ wid: 7 } as any); + const displaySpy = vi.spyOn(component, "displayExecutionWithLogs").mockImplementation(() => {}); + + component.ngOnInit(); + tick(0); + expect(displaySpy).toHaveBeenCalledTimes(1); + + fixture.destroy(); + tick(5000); + + expect(displaySpy).toHaveBeenCalledTimes(1); + })); + }); + describe("template rendering", () => { // Query, assert the element is present, then dispatch — a missing selector fails // with a clear message instead of a null dereference.
