This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 9f2102e9b0 test(frontend): close the result-table frame's
stat-formatting gaps (#7972)
9f2102e9b0 is described below
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}