fix(test, frontend): stub Plotly in the execution-history spec - #8288
Open
mengw15 wants to merge 2 commits into
Open
fix(test, frontend): stub Plotly in the execution-history spec#8288mengw15 wants to merge 2 commits into
mengw15 wants to merge 2 commits into
Conversation
`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: jsdom has no layout or canvas, so whether Plotly drew anything sensible is unobservable here — 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, and the file's test time drops from 4.12s to 1.78s locally. 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 20s 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 339ms/25 tests and 197ms/12 tests they are not close to 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 now `--include`. Closes apache#8287.
mengw15
requested review from
aglinxinyuan
and
a lite review from Copilot
and removed request for
Copilot
August 31, 2026 11:11
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR reduces flakiness and runtime in workflow-execution-history.component.spec.ts by stubbing Plotly renders and asserting against Plotly.newPlot call arguments, and fixes the documented Angular single-spec flag in TESTING.md.
Changes:
- Mock
plotly.js-basic-dist-minin the execution-history spec and validate chart inputs vianewPlotcall args instead of jsdom-rendered graph div state. - Clear the Plotly mock between setups to avoid cross-test pollution.
- Correct
TESTING.mdto useng test --includefor single-file runs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/app/dashboard/component/user/user-workflow/ngbd-modal-workflow-executions/workflow-execution-history.component.spec.ts | Stubs Plotly and rewrites chart assertions to check newPlot arguments, improving test speed/stability |
| frontend/TESTING.md | Updates single-file test docs to use the correct --include flag |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Automated Reviewer SuggestionsBased on the
|
github-actions
Bot
added
fix
frontend
Contributor
Backport auto-label reportThis
|
Addresses review feedback on PR apache#8288: the `plot()` helper used `.find()`, which would silently pick the first match if the component ever called `Plotly.newPlot` twice for the same id. Switch to `.filter()` + `expect(calls).toHaveLength(1)` so a duplicate render fails the test alongside a missing one.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
workflow-execution-history.component.spec.tsletngAfterViewInitcall the realPlotly.newPloton 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
dataandlayoutback 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
newPlotinstead. 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-quotaandworkflow-runtime-statisticsrender 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 documentedng 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
WIDTHfrom 450 to 451 turns the chart test red, and removing one of the twoPlotly.newPlotcalls does too. Restoring each returns the file to 61 passing.eslintis clean on the touched spec.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5)