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-7957-0af18e0b013b64650bf34ffd5dcf3ae1cc2637d6 in repository https://gitbox.apache.org/repos/asf/texera.git
commit d7dc698c54ca18e99cb89c98d2e0d3ccc537a875 Author: Xinyuan Lin <[email protected]> AuthorDate: Tue Aug 25 21:22:08 2026 +0000 test(frontend): cover the dataset detail component's upload-progress paths (#7957) ### What changes were proposed in this PR? `dataset-detail.component.spec.ts` goes from 195 tests to 203, closing the residue left after an earlier pass — mostly the upload-progress paths. Measured from raw lcov counters, same spec filter on both sides, `rm -rf coverage` between runs: | Counter | Before | After | |---|---|---| | Lines hit | 412/414 | **414/414 = 100%** | | Branch arms | 193/203 = 95.1% | **200/203 = 98.5%** (10 missed → 3) | | Functions | 120/120 | 120/120 | | Codecov's metric | 411/422 = 97.4% | **420/422 = 99.5%** | Functions were already 120/120 with zero `FNDA:0` records both before and after — worth stating, because twice in this campaign a file sat at high line coverage with functions uncovered and a binding whose handler the spec called directly stayed `FNDA:0` while looking covered. Not the case here. ### Verification 16 mutations, **all 16 killed**, each named in the mutation table with its exact failure message. The first draft reported no survivors. **Seven mutants survived its 202-test suite with exit 0** — including `.pop()` → `.shift()`, replacing a computed time-zone name with `""`, and several progress-index substitutions. All now die, and the search space additionally includes one mutant added on a fresh axis (the `", "` separator). Stated with the scope the reviewer correctly insisted on: **this reports the search space, not a proof about the file.** Sixteen mutants died; that is not the same claim as "the file is mutation-complete". Three repairs are worth naming because the original tests looked fine: - **The time-zone test was degenerate**, asserting on whatever the runner's ambient `Intl` formatter produced. It now stubs the formatter, so the assertion is about the component's own `.split(", ").pop()` parsing rather than the platform's output — and a second test covers the separator. - **Two single-index tests passed trivially.** "Ignores a hide request for a row that is gone" and the basename test each now assert a valid-index half alongside the invalid one, so an index substitution cannot slip through. - One reported failure mode was simply wrong and is rewritten: a mutant was described as surfacing a `TypeError` through a `.not.toThrow()` assertion, when the emission actually carries a valid percentage. ### Deliberately not included Two lines with three branch arms remain, and both are refused for the same reason: `percentage: progress.percentage ?? this.uploadTasks[taskIndex].percentage ?? 0` (line 647) and its twin in the error handler (line 678) fall through a `??` whose right operand no caller can produce. Verified in the final lcov — line 647 is hit 28 times with both arms at zero. After this bundle those are the **only** two lines in the file still carrying a missed arm, and the zero-hit set is empty. Also recorded honestly: **three** of the tests pin defensive paths production cannot reach (the first draft said two). They are kept because they document the guards, not because they earn coverage. No production file is touched, and the `node_modules` junction used for the run was removed before committing. ### Any related issues, documentation, discussions? Closes #7955 ### How was this PR tested? ``` npx ng test --watch=false --include="**/dataset-detail.component.spec.ts" ``` ``` Test Files 1 passed (1) ``` `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) --- .../dataset-detail.component.spec.ts | 183 ++++++++++++++++++++- 1 file changed, 182 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts index af5c6aed18..20570f4e95 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts @@ -22,7 +22,7 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { By } from "@angular/platform-browser"; import { NoopAnimationsModule } from "@angular/platform-browser/animations"; import { ActivatedRoute, Router } from "@angular/router"; -import { of, Subject, throwError } from "rxjs"; +import { concat, of, Subject, throwError } from "rxjs"; import { NzModalService } from "ng-zorro-antd/modal"; import { NzResizableDirective } from "ng-zorro-antd/resizable"; import { NzTooltipDirective } from "ng-zorro-antd/tooltip"; @@ -199,6 +199,72 @@ describe("DatasetDetailComponent upload queue", () => { }); }); + /** + * A progress event and the five-second hide timer both address a row by its index in + * `uploadTasks`, and that row can already be gone — dismissed by the user — by the time + * either arrives, so both lookups have to survive the miss. The completion path also has + * to pick a key for `uploadTimeMap` out of a name that may carry directories. + */ + describe("progress bookkeeping", () => { + beforeEach(() => { + // The completion path arms a 5s row-hide timer; keep it off the real clock. + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("ignores progress for a row that is no longer listed", () => { + dropFiles("a.csv"); + component.uploadTasks = []; // dismissed while a chunk was still in flight + + expect(() => uploadSubjects[0].next({ filePath: "a.csv", percentage: 50, status: "uploading" })).not.toThrow(); + // The late event must not resurrect the row or write a phantom index into the list. + expect(component.uploadTasks).toEqual([]); + expect(Object.keys(component.uploadTasks)).toHaveLength(0); + }); + + it("keys the upload time by the last path segment, falling back to the whole name", () => { + // Taking the segment after the last "/" yields "" for a name that ends in one, and + // keying the map under "" would collide every such upload onto one entry; the + // fallback keeps the name the caller gave instead. + dropFiles("nested/dir/"); + + finishUpload(0, "nested/dir/", 7); + + expect(component.uploadTimeMap.get("nested/dir/")).toBe(7); + expect(component.uploadTimeMap.has("")).toBe(false); + + // The reader of this map (user-dataset-staged-objects-list) looks a row up by + // `filePath.split("/").pop() || filePath`, so an ordinary nested name has to be + // keyed by its last segment here or the per-file time silently stops rendering. + dropFiles("dir/sub/a.csv"); + + finishUpload(1, "dir/sub/a.csv", 9); + + expect(component.uploadTimeMap.get("a.csv")).toBe(9); + expect(component.uploadTimeMap.has("dir/sub/a.csv")).toBe(false); + }); + + it("ignores a hide request for a row that is gone", () => { + // Every one of scheduleHide's call sites already checks the index, so the -1 arm + // pins a defensive no-op rather than a reachable scenario: without the guard the + // lookup would read `filePath` off undefined and throw. The valid-index call that + // follows keeps a scheduleHide which does nothing at all from passing this test. + dropFiles("a.csv"); + const before = [...component.uploadTasks]; + + expect(() => (component as any).scheduleHide(-1)).not.toThrow(); + expect(component.uploadTasks).toEqual(before); + + (component as any).scheduleHide(0); + vi.advanceTimersByTime(5000); + + expect(component.uploadTasks).toEqual([]); + }); + }); + /** * Aborting an in-flight upload has to survive the backend still finalizing the previous attempt: * the abort call is retried on 409 up to ABORT_RETRY_MAX_ATTEMPTS, a 404 means it is already gone, @@ -235,6 +301,12 @@ describe("DatasetDetailComponent upload queue", () => { expect(finalize).toHaveBeenCalledWith("[email protected]", "test-dataset", "a.txt", true); expect(component.uploadTasks.find(t => t.filePath === "a.txt")!.status).toBe("aborted"); expect(onAborted).toHaveBeenCalledTimes(1); + + // The aborted row goes on the same five-second hide timer a finished one does, so + // it clears itself out of the list instead of sitting there for the rest of the session. + vi.advanceTimersByTime(5000); + + expect(component.uploadTasks.find(t => t.filePath === "a.txt")).toBeUndefined(); }); it("stops listening to the upload it aborted", () => { @@ -410,6 +482,38 @@ describe("DatasetDetailComponent upload queue", () => { expect(component.activeCount).toBe(0); expect(onCanceled).toHaveBeenCalledTimes(1); }); + + it("tells the caller once even when the abort call reports more than once", () => { + // The callback is latched so that it fires exactly once no matter how many of the + // subscription's handlers reach it. HttpClient itself delivers a single response, + // so this drives the latch directly: a response followed by a stream failure runs + // the next handler and then the error handler, and both of them report done. + finalize.mockReturnValueOnce( + concat( + of({}), + throwError(() => ({ status: 500 }) as any) + ) + ); + const task = inFlight(); + const onAborted = vi.fn(); + + component.onClickAbortUploadProgress(task as any, onAborted); + + expect(onAborted).toHaveBeenCalledTimes(1); + }); + + it("aborts a task whose row was already dropped without resurrecting it", () => { + const task = inFlight(); + component.uploadTasks = []; // the row was dismissed before the abort was clicked + + component.onClickAbortUploadProgress(task as any); + + expect(finalize).toHaveBeenCalledWith("[email protected]", "test-dataset", "a.txt", true); + // Writing "aborted" back at a missing index would leave a phantom "-1" property on + // the array, which neither a throw nor `.length` would reveal. + expect(component.uploadTasks).toEqual([]); + expect(Object.keys(component.uploadTasks)).toHaveLength(0); + }); }); /** @@ -1221,6 +1325,68 @@ describe("DatasetDetailComponent behavior", () => { expect(component.coverImageUrl).toBeNull(); expect(datasetServiceStub.getDatasetCoverUrl).not.toHaveBeenCalled(); }); + + /** + * Stands in for the platform time-zone formatter so the assertions do not depend on + * whichever zone the machine running the suite sits in. `formatted` maps the requested + * `timeZoneName` option to the whole string the formatter would return, so the stub + * answers "long" and "short" differently the way a real formatter does — asking for the + * wrong one stays observable. Any call that does not ask for a zone name is delegated to + * the real constructor, since other code formats the same date through Intl. + */ + const stubZoneFormatter = (formatted: Record<string, string>) => { + const realDateTimeFormat = Intl.DateTimeFormat; + return vi.spyOn(Intl, "DateTimeFormat").mockImplementation(function (locale?: any, options?: any) { + const requested = options?.timeZoneName as string | undefined; + return requested === undefined + ? new (realDateTimeFormat as any)(locale, options) + : ({ format: () => formatted[requested] ?? `<unstubbed:${requested}>` } as any); + } as any); + }; + + const renderTooltipWithCreationTime = () => { + datasetServiceStub.getDataset.mockReturnValue( + of(makeDashboardDataset({ dataset: makeDataset({ creationTime: CREATION_TS }) })) + ); + + createComponent(); + component.did = 5; + component.retrieveDatasetInfo(); + }; + + it("takes the tooltip's time zone from the spelled-out name the formatter appends", () => { + // The parenthetical is the segment after the last ", " of a long-form formatted + // date. Reading any other segment, or asking the formatter for the abbreviated + // zone, would put "11/14/2023" or "PST" in front of the user instead. + const zoned = stubZoneFormatter({ + long: "11/14/2023, Pacific Standard Time", + short: "11/14/2023, PST", + }); + + try { + renderTooltipWithCreationTime(); + + expect(component.datasetCreationTimeTooltip).toMatch(/ \(Pacific Standard Time\)$/); + } finally { + // Vitest runs these specs without isolation, so a leaked global spy would + // follow the worker into the next spec file. + zoned.mockRestore(); + } + }); + + it("leaves the tooltip's time zone empty when the runtime supplies no zone name", () => { + // A formatter that yields no zone name at all must render an empty parenthetical + // rather than leaking "undefined" into a user-visible tooltip. + const zoneless = stubZoneFormatter({ long: "", short: "" }); + + try { + renderTooltipWithCreationTime(); + + expect(component.datasetCreationTimeTooltip).toMatch(/ \(\)$/); + } finally { + zoneless.mockRestore(); + } + }); }); describe("retrieveDatasetVersionList", () => { @@ -1951,6 +2117,21 @@ describe("DatasetDetailComponent behavior", () => { expect(component.datasetDescription).toBe("old"); expect(notificationServiceStub.error).toHaveBeenCalledWith("Failed to update dataset description"); }); + + it("stores an empty description when the editor hands back nothing", () => { + // The editor round-trips whatever it was bound to, and a dataset whose stored + // description is null binds a nullish value straight back out. Persisting that + // verbatim would write `undefined` over a description instead of clearing it. + datasetServiceStub.updateDatasetDescription.mockReturnValue(of({})); + createComponent(); + component.did = 5; + component.datasetDescription = "old"; + + component.onDatasetDescriptionChange(undefined as unknown as string); + + expect(datasetServiceStub.updateDatasetDescription).toHaveBeenCalledWith(5, ""); + expect(component.datasetDescription).toBe(""); + }); }); describe("copyCurrentFilePath", () => {
