Copilot commented on code in PR #7972:
URL: https://github.com/apache/texera/pull/7972#discussion_r3856068127
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts:
##########
@@ -402,6 +428,77 @@ 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");
+ component.isOperatorFinished = false;
+ component.tableStats = { col: { count: 1234567 } };
+ component.prevTableStats = { col: {} };
+
+ component.compare("col", "count");
+
+ const grouped = (1234567).toLocaleString();
+ expect(grouped).not.toBe("1234567");
+ expect(bypassSpy).toHaveBeenLastCalledWith(
+ grouped
+ .split("")
+ .map(char => black(char))
+ .join("")
+ );
+ });
Review Comment:
The assertion `expect(grouped).not.toBe("1234567")` makes this test
dependent on the runtime’s default locale/ICU behavior and can fail in
environments where `toLocaleString()` doesn’t apply grouping by default. To
keep the test deterministic while still ensuring `compare()` uses
`toLocaleString()` (vs `String()`), spy on `Number.prototype.toLocaleString`
and use the spy’s returned value for the expectation.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts:
##########
@@ -149,7 +149,11 @@ 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);
Review Comment:
`setupResultTable` is a public method on the component (not a private
helper), so casting `component` to `any` here is unnecessary and weakens
type-checking in the spec.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
