You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "mengw15 (via GitHub)" <gi...@apache.org> on 2026/08/31 11:10:36 UTC
[PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
mengw15 opened a new pull request, #8288:
URL: https://github.com/apache/texera/pull/8288
### What changes were proposed in this PR?
`workflow-execution-history.component.spec.ts` let `ngAfterViewInit` call the real `Plotly.newPlot` on every setup. 36 of its 61 tests build the component, and each build plots twice, so one run drove roughly 72 real Plotly renders through jsdom — for the benefit of a single test.
That test then read `data` and `layout` back off the graph divs Plotly populates. Every value it asserted (`type`, `labels`, `values`, `x`, `y`, the widths, the titles) is one the component itself computed and handed over, so the round trip through the real library bought no coverage. It also cannot buy any here: jsdom has no layout or canvas, so whether Plotly drew anything sensible is unobservable — that belongs to the browser-mode suite, if anywhere.
Mock the module for this spec and assert on the arguments passed to `newPlot` instead. The chart test now checks the component's own decision rather than Plotly's handling of it.
This is what made the spec flaky rather than merely slow: the failing case on macOS was three synchronous assertions behind a `setup()`, timing out against the 20 s per-test limit under runner contention, while ubuntu and windows passed. Removing the dominant avoidable cost restores the margin; it is not a proof that Plotly was the only contributor.
`user-quota` and `workflow-runtime-statistics` render real Plotly too, but at 339 ms / 25 tests and 197 ms / 12 tests they are nowhere near the limit, so they are left alone.
Also corrects `TESTING.md`, which documented `ng test --test-file <path>` for single-file runs; the builder rejects that argument and the flag is `--include`.
### Any related issues, documentation, discussions?
Closes #8287.
### How was this PR tested?
`yarn ng test --include ".../workflow-execution-history.component.spec.ts"` — 61 passed, test time 4.12 s → 1.69 s.
Failure paths checked, since the rewritten assertions must still catch what the old ones did: changing the component's pie-chart `WIDTH` from 450 to 451 turns the chart test red, and removing one of the two `Plotly.newPlot` calls does too. Restoring each returns the file to 61 passing. `eslint` is clean on the touched spec.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 merged PR #8288:
URL: https://github.com/apache/texera/pull/8288
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8288:
URL: https://github.com/apache/texera/pull/8288#issuecomment-5477667526
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@aglinxinyuan`
You can notify them by mentioning `@aglinxinyuan` in a comment.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8288:
URL: https://github.com/apache/texera/pull/8288#issuecomment-5477670914
<!-- backport-auto-label-report -->
### Backport auto-label report
This `fix:` PR was checked against each actively-supported release branch. `release/*` labels drive the post-merge backport, so add or remove one to change where this fix lands.
| Release branch | Analysis |
| --- | --- |
| ⚠️ `release/v1.3` | Not labeled automatically — none of the files this PR modifies exist on this branch (`frontend/TESTING.md`, `frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-execution-history.component.spec.ts`). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. **Please check and add `release/v1.3` by hand if this fix should be backported here.** |
| ✅ `release/v1.2` | Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7. |
_[Auto-label run](https://github.com/apache/texera/actions/runs/33385746535)._
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8288:
URL: https://github.com/apache/texera/pull/8288#issuecomment-5478875492
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/8288?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 93.93%. Comparing base ([`50321e4`](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`73612c7`](https://app.codecov.io/gh/apache/texera/commit/73612c7b9317d107eed6011140b0a9b709af3cf1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8288 +/- ##
=========================================
Coverage 93.93% 93.93%
Complexity 4762 4762
=========================================
Files 1191 1191
Lines 48308 48308
Branches 5377 5377
=========================================
Hits 45380 45380
Misses 1494 1494
Partials 1434 1434
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.00% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `99.32% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.03% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.67% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `86.86% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.91% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `96.41% <ø> (ø)` | | |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.31% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.87% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/8288/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.19% <ø> (ø)` | | Carriedforward from [50321e4](https://app.codecov.io/gh/apache/texera/commit/50321e403c82df299a13deb50a7f9849dd93bdba?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/8288?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(test, frontend): stub Plotly in the execution-history spec [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #8288:
URL: https://github.com/apache/texera/pull/8288#discussion_r3894137049
##########
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:
Good point — switched the helper to `filter(...)` and asserted the id appears exactly once, so a duplicate render for the same id fails the test alongside a missing one.
##########
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:
Leaving the mock at `vi.fn()`. The component never awaits or thens `newPlot` today and neither does this spec, so a resolved-Promise return would only guard a hypothetical future caller. When one actually shows up, the PR that adds the `await` can bring the mock along.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org