Copilot commented on code in PR #8288:
URL: https://github.com/apache/texera/pull/8288#discussion_r3894007842
##########
frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-execution-history.component.spec.ts:
##########
@@ -206,30 +215,34 @@ describe("WorkflowExecutionHistoryComponent", () => {
it("draws a username pie, a status pie, and a process-time bar chart",
async () => {
await setup();
- // ngAfterViewInit renders the charts via real Plotly, which attaches
`data`/`layout`
- // to each graph div (looked up by the id the component passes, incl.
the leading '#').
- const gd = (id: string) => document.getElementById(id) as unknown as {
data: any[]; layout: any };
-
- const usernamePie = gd("#execution-userName-pie-chart").data[0];
- expect(usernamePie.type).toBe("pie");
- expect(usernamePie.labels).toEqual(["alice", "bob"]);
- expect(usernamePie.values).toEqual([2, 1]);
- expect(gd("#execution-userName-pie-chart").layout).toMatchObject({
+ // ngAfterViewInit plots each chart by id; assert on what the component
+ // handed Plotly rather than on the DOM Plotly would build from it.
+ const plot = (id: string) => {
+ const call = vi.mocked(Plotly.newPlot).mock.calls.find(c => c[0] ===
id);
+ if (!call) throw new Error(`no Plotly.newPlot call for ${id}`);
Review Comment:
Using `.find(...)` means the test can still pass if the component
accidentally calls `Plotly.newPlot` multiple times for the same id (it will
silently take the first match). Consider additionally asserting the expected
total call count and/or that each expected chart id appears exactly once (e.g.,
`calls.filter(...).length === 1`) to make the test fail on duplicate renders as
well as missing renders.
##########
frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-execution-history.component.spec.ts:
##########
@@ -38,9 +38,17 @@ import { StubOperatorMetadataService } from
"../../../../../workspace/service/op
import { commonTestProviders } from "../../../../../common/testing/test-utils";
import { DebugElement } from "@angular/core";
import { By } from "@angular/platform-browser";
+import * as Plotly from "plotly.js-basic-dist-min";
import { NoopAnimationsModule } from "@angular/platform-browser/animations";
import { NzPopoverDirective } from "ng-zorro-antd/popover";
+// The component's job at this boundary is choosing a chart id and computing
the
+// data and layout for it; turning those into DOM is Plotly's. Rendering for
real
+// in jsdom asserts nothing extra -- the chart test below reads back exactly
the
+// arguments the component passed -- while it dominates the file's runtime,
since
+// 36 of these tests build the component and each build plots twice. See #8287.
+vi.mock("plotly.js-basic-dist-min", () => ({ newPlot: vi.fn() }));
Review Comment:
Plotly's `newPlot` typically returns a Promise; the mock currently returns
`undefined`. To keep the mock API-compatible and reduce future brittleness if
production code starts awaiting/then-ing `newPlot`, consider making the mock
return a resolved Promise (e.g., via `mockResolvedValue(...)`).
--
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]
