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-7972-5fc901727c4582778c14035bfe6e4be6ccd02770 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 9f2102e9b0d83b7b518f37bab3ae497d3a0e9b20 Author: Xinyuan Lin <[email protected]> AuthorDate: Tue Aug 25 21:36:40 2026 +0000 test(frontend): close the result-table frame's stat-formatting gaps (#7972) ### What changes were proposed in this PR? `result-table-frame.component.spec.ts` goes from 41 tests to 46, closing the last reachable gap in the component. Measured from raw lcov (v8 via vitest), identical include glob on both sides, `rm -rf coverage junit.xml` between runs. The before figure came from restoring the spec with `git show HEAD:<spec>` — a single exact path, never a directory checkout. | Counter | Before | After | |---|---|---| | Lines hit | 184/185 = 99.46% | **185/185 = 100%** | | Codecov's metric (fully covered) | 180/185 = 97.30% | **184/185 = 99.46%** | | Branch arms | 92/96 | **95/96** | | Functions | 37/37 | 37/37 | **+4 fully-covered lines and +3 branch arms — and this is plainly a small PR.** Three short tests, ~40 spec lines. I would rather say that than dress it up: 184/185 is this file's permanent ceiling under a test-only change. Functions were already 37/37 with zero `FNDA:0` entries in both runs. Worth checking rather than assuming, because three times in this campaign a file sat at high line coverage with functions uncovered. ### What the new tests actually pin One of them closes a real hole rather than a counter: an existing test's *name* claimed it exercised the empty-result guard, while its body actually exercised the `operatorId` guard. The two are now separated. Another covers a genuinely reachable path that the type signature denies: `IcebergDocument.getTableStatistics` emits **ISO date strings** for `Timestamp` column min/max, contradicting the frontend's `Record<string, Record<string, number>>`. Reaching it needs two `as unknown as number` casts, which the test carries a comment explaining — otherwise a reviewer would reasonably read it as a coverage hack. ### Verification 16 mutations, **14 killed, 2 survivors**, both stated rather than papered over. - **`BRDA:250,20,1` is structurally dead.** The `: currentStr` arm of `previous !== undefined ? previous.toFixed(2) : currentStr` sits inside `typeof current === "number" && typeof previous === "number"`, so `previous` is provably not undefined. Closing it would need a production edit (deleting the vacuous guard), which the test-only constraint forbids. This is why the ceiling is 184/185. - **A single-sided `previous.toLocaleString()` → `String(previous)` at line 253 survives, and is left unkilled on purpose.** It is observable only when the *same* column's stat is non-numeric in one snapshot and a number ≥ 1000 in the other — a mid-run stat type flip. Pinning that would cement a shape the backend does not produce. Five claims from the first draft were corrected, including a test title that claimed to observe `toLocaleString` for string payloads (it cannot — `String#toLocaleString` returns the same string), and a "there is no other gap for anyone to re-hunt" conclusion drawn from the absence of zero-count lcov entries, which is a category error: a fully-hit file can still be entirely unpinned. ### Reported, not pinned An empty fetched page returns at the line-424 guard **before** `isLoadingResult = false` at line 428, so the `nz-table`'s bound spinner never clears for an operator that produced zero rows. The test deliberately omits that assertion so this PR does not cement it; it deserves its own issue. No production file is touched, and the worktree's `node_modules` is a real install rather than a junction. ### Any related issues, documentation, discussions? Closes #7971 ### How was this PR tested? ``` npx ng test --watch=false --include="**/result-table-frame.component.spec.ts" ``` ``` Test Files 1 passed (1) Tests 46 passed (46) ``` `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]> --- .../result-table-frame.component.spec.ts | 132 ++++++++++++++++++++- 1 file changed, 129 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts index 0265490ac1..d4c9e85b08 100644 --- a/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts @@ -149,10 +149,13 @@ describe("ResultTableFrameComponent", () => { expect(component).toBeTruthy(); }); - it("currentResult should not be modified if setupResultTable is called with empty (zero-length) execution result", () => { + // NOTE: this exercises the *missing operator* guard, not the empty-result guard - the + // fixture built in beforeEach has no operatorId, so setupResultTable returns before it + // ever looks at the row count. The empty-result guard is covered by + // "keeps the existing table when the fetched page comes back empty" below. + it("currentResult should not be modified if setupResultTable is called without a selected operator", () => { component.currentResult = [{ test: "property" }]; - (component as any).setupResultTable([], 0); - + component.setupResultTable([], 0); expect(component.currentResult).toEqual([{ test: "property" }]); }); @@ -236,6 +239,28 @@ describe("ResultTableFrameComponent", () => { expect(component.totalNumTuples).toBe(0); }); + it("keeps the existing table when the fetched page comes back empty", () => { + component.operatorId = "op1"; + const row: IndexableObject = { media: "https://example.com/clip.mp4"; }; + // build the "existing table" through the real code path so that cellMediaTypes is + // populated for real, making a guard that wiped it visible in the assertions below + component.setupResultTable([row], 5); + + // An empty page whose reported total is NONZERO: the row count shrank while the user + // sat on a page index past the new end. The two comparands differ, so this pins that + // the guard tests the length of the fetched page and not the reported total. + component.setupResultTable([], 7); + + expect(component.currentResult).toEqual([row]); + expect(component.currentColumns?.map(c => c.columnDef)).toEqual(["media"]); + // still the total from the non-empty page: the guard returns before totalNumTuples is + // reassigned from totalRowCount + expect(component.totalNumTuples).toBe(5); + // the precomputed media-type map is part of the table state and must survive as well, + // otherwise every media cell of the still-displayed table degrades to plain text + expect(component.getCellMediaType(row, 0)).toBe("video"); + }); + it("builds columns from the first row and drops the internal _id column", () => { component.operatorId = "op1"; component.isLoadingResult = true; @@ -402,6 +427,78 @@ describe("ResultTableFrameComponent", () => { expect(bypassSpy).toHaveBeenCalledWith(black("3") + black(".") + black("0") + black("0")); }); + // `tableStats` is declared Record<string, Record<string, number>>, but the backend + // does put non-numeric values in it: IcebergDocument.getTableStatistics seeds a + // Timestamp column's min/max with an ISO date *string*, and the template feeds those + // straight into compare(). The casts below model that real payload, which is the only + // way to reach the non-numeric formatting branch. Note that String#toLocaleString is + // the identity, so a string payload cannot observe that call at all; what this test + // pins is that previousStr is derived from the previous snapshot instead of collapsing + // onto currentStr. The toLocaleString call itself is pinned by the numeric test below. + it("highlights only the character that changed when both stats are non-numeric strings", () => { + const bypassSpy = vi.spyOn(TestBed.inject(DomSanitizer), "bypassSecurityTrustHtml"); + component.isOperatorFinished = false; + component.tableStats = { ts: { min: "2024-01-02" as unknown as number } }; + component.prevTableStats = { ts: { min: "2024-01-09" as unknown as number } }; + + component.compare("ts", "min"); + + // the two dates agree up to the last character, which is the only one highlighted + expect(bypassSpy).toHaveBeenLastCalledWith( + "2024-01-0" + .split("") + .map(char => black(char)) + .join("") + blue("2") + ); + }); + + it("highlights the trailing characters that the shorter previous snapshot never reaches", () => { + const bypassSpy = vi.spyOn(TestBed.inject(DomSanitizer), "bypassSecurityTrustHtml"); + component.isOperatorFinished = false; + component.tableStats = { ts: { min: "2024-01-02" as unknown as number } }; + component.prevTableStats = { ts: { min: "2024" as unknown as number } }; + + component.compare("ts", "min"); + + // the first four characters match and stay black; for every index past the end of + // previousStr the lookup yields undefined, which counts as changed + expect(bypassSpy).toHaveBeenLastCalledWith( + "2024" + .split("") + .map(char => black(char)) + .join("") + + "-01-02" + .split("") + .map(char => blue(char)) + .join("") + ); + }); + + // The non-numeric branch is also taken for a *numeric* current stat whose previous + // snapshot is missing (a column that has only just appeared in the stats stream), and + // there the difference between toLocaleString and plain String conversion is + // user-visible: a row count in the millions is rendered with group separators. The + // expectation is computed rather than hard-coded so it holds under any locale, and the + // first assertion keeps it from going vacuous on a runtime without number grouping. + it("formats large numeric stats with locale group separators", () => { + const bypassSpy = vi.spyOn(TestBed.inject(DomSanitizer), "bypassSecurityTrustHtml"); + const toLocaleSpy = vi.spyOn(Number.prototype, "toLocaleString"); + component.isOperatorFinished = false; + component.tableStats = { col: { count: 1234567 } }; + component.prevTableStats = { col: {} }; + + component.compare("col", "count"); + + expect(toLocaleSpy).toHaveBeenCalled(); + const grouped = String(toLocaleSpy.mock.results[0]?.value); + expect(bypassSpy).toHaveBeenLastCalledWith( + grouped + .split("") + .map(char => black(char)) + .join("") + ); + }); + it("falls back to plain formatting when the previous stat is missing", () => { const bypassSpy = vi.spyOn(TestBed.inject(DomSanitizer), "bypassSecurityTrustHtml"); component.isOperatorFinished = false; @@ -670,6 +767,35 @@ describe("ResultTableFrameComponent", () => { expect(isImageUrl("text")).toBe(false); }); + describe("getCellMediaType", () => { + it("returns the media type precomputed for that exact row and column", () => { + component.cellMediaTypes = new Map(); + expect(component.getCellMediaType({ media: "https://example.com/clip.mp4"; }, 0)).toBe("text"); + + component.operatorId = "op1"; + // Two rows x two columns with the media cells on opposite diagonals. A 1x1 fixture + // would leave both of the correspondences this map exists to encode unpinned: the + // per-row entry (keyed by row identity) and the position of each column's type + // inside that entry, which the template indexes by the *ngFor index over + // currentColumns. + const rows: IndexableObject[] = [ + { pic: "https://example.com/a.png";, clip: "plain" }, + { pic: "plain", clip: "https://example.com/b.mp4"; }, + ]; + component.setupResultTable(rows, 2); + + expect(component.getCellMediaType(rows[0], 0)).toBe("image"); + expect(component.getCellMediaType(rows[0], 1)).toBe("text"); + expect(component.getCellMediaType(rows[1], 0)).toBe("text"); + expect(component.getCellMediaType(rows[1], 1)).toBe("video"); + // in the map, but past the end of its column list + expect(component.getCellMediaType(rows[1], 3)).toBe("text"); + // an equal-valued row object that never went through setupResultTable: the map is + // keyed by identity, so this misses and falls back + expect(component.getCellMediaType({ ...rows[1] }, 1)).toBe("text"); + }); + }); + describe("media cell rendering in table", () => { beforeEach(() => { component.operatorId = "test-op";
![https://example.com/a.png"](https://example.com/a.png"){kind=link}