Add test for Pie and Legend animation#7488
Merged
Merged
Conversation
Contributor
WalkthroughTwo animation assertion helpers ( ChangesPie Animation Test Helpers and Legend Coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PavelVanecek
marked this pull request as ready for review
June 23, 2026 12:36
Bundle ReportBundle size has no change ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helper/expectAnimatedPieAngles.tsx`:
- Around line 9-32: The expectAnimatedPieAngles function accepts a steps
parameter that can be negative, which when used to calculate stepSize will cause
the animation progress loop to run indefinitely and hang tests. Add validation
at the beginning of the expectAnimatedPieAngles function to ensure the steps
parameter is positive (greater than 0), similar to validation patterns applied
elsewhere in the codebase, and throw an appropriate error if the validation
fails.
In `@test/helper/expectAnimatedPiePaths.ts`:
- Around line 10-35: The `steps` parameter in this function is unvalidated and
can accept negative values, which causes the loop to run indefinitely since
`stepSize` becomes negative (making `animationProgress` decrease forever while
remaining less than 1). Add validation at the beginning of the function to
ensure `steps` is greater than zero, either by throwing an error for invalid
values or by using an assertion. This validation should occur before the
`stepSize` calculation to prevent the infinite loop condition in the for loop
that iterates while `animationProgress < 1`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfacb4a7-61aa-494e-a336-b46c7f18d300
📒 Files selected for processing (4)
test/helper/expectAnimatedPieAngles.tsxtest/helper/expectAnimatedPiePaths.tstest/polar/Pie/Pie.animation.spec.tsxtest/polar/Pie/PieWithLegend.animation.spec.tsx
Comment on lines
+9
to
+32
| steps: number = 5, | ||
| ): Promise<ReadonlyArray<ReadonlyArray<{ startAngle: number; endAngle: number }>>> { | ||
| assertNotNull(container); | ||
| /* | ||
| * Pie sectors at 0% progress do not render at all, so we | ||
| * start from 0.1 so that the sectors have rendered a little bit. | ||
| */ | ||
| let animationProgress = 0.1; | ||
| await animationManager.setAnimationProgress(animationProgress); | ||
| const stepSize = (1 - animationProgress) / steps; | ||
| const initialPieSectors = selectPieSectors(container); | ||
| const initialAngles = getPieSectorAngles(initialPieSectors); | ||
| const initialAttributes = Array.from(initialPieSectors).map(sector => ({ | ||
| fill: sector.getAttribute('fill'), | ||
| stroke: sector.getAttribute('stroke'), | ||
| })); | ||
|
|
||
| const anglesDuringAnimation: { startAngle: number; endAngle: number }[][] = []; | ||
| for (animationProgress += stepSize; animationProgress < 1; animationProgress += stepSize) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await animationManager.setAnimationProgress(animationProgress); | ||
| const currentPieSectors = selectPieSectors(container); | ||
| const currentAngles = getPieSectorAngles(currentPieSectors); | ||
| anglesDuringAnimation.push(currentAngles); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Apply the same steps validation here to avoid hangs.
This helper has the same unbounded progress loop pattern; steps < 0 can keep the loop running forever and block test completion.
Suggested fix
export async function expectAnimatedPieAngles(
container: Element,
animationManager: MockAnimationManager,
steps: number = 5,
): Promise<ReadonlyArray<ReadonlyArray<{ startAngle: number; endAngle: number }>>> {
+ if (!Number.isInteger(steps) || steps <= 0) {
+ throw new Error(`steps must be a positive integer, received: ${steps}`);
+ }
assertNotNull(container);
@@
- for (animationProgress += stepSize; animationProgress < 1; animationProgress += stepSize) {
+ for (let i = 1; i < steps; i += 1) {
+ animationProgress = 0.1 + i * stepSize;
// eslint-disable-next-line no-await-in-loop
await animationManager.setAnimationProgress(animationProgress);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| steps: number = 5, | |
| ): Promise<ReadonlyArray<ReadonlyArray<{ startAngle: number; endAngle: number }>>> { | |
| assertNotNull(container); | |
| /* | |
| * Pie sectors at 0% progress do not render at all, so we | |
| * start from 0.1 so that the sectors have rendered a little bit. | |
| */ | |
| let animationProgress = 0.1; | |
| await animationManager.setAnimationProgress(animationProgress); | |
| const stepSize = (1 - animationProgress) / steps; | |
| const initialPieSectors = selectPieSectors(container); | |
| const initialAngles = getPieSectorAngles(initialPieSectors); | |
| const initialAttributes = Array.from(initialPieSectors).map(sector => ({ | |
| fill: sector.getAttribute('fill'), | |
| stroke: sector.getAttribute('stroke'), | |
| })); | |
| const anglesDuringAnimation: { startAngle: number; endAngle: number }[][] = []; | |
| for (animationProgress += stepSize; animationProgress < 1; animationProgress += stepSize) { | |
| // eslint-disable-next-line no-await-in-loop | |
| await animationManager.setAnimationProgress(animationProgress); | |
| const currentPieSectors = selectPieSectors(container); | |
| const currentAngles = getPieSectorAngles(currentPieSectors); | |
| anglesDuringAnimation.push(currentAngles); | |
| export async function expectAnimatedPieAngles( | |
| container: Element, | |
| animationManager: MockAnimationManager, | |
| steps: number = 5, | |
| ): Promise<ReadonlyArray<ReadonlyArray<{ startAngle: number; endAngle: number }>>> { | |
| if (!Number.isInteger(steps) || steps <= 0) { | |
| throw new Error(`steps must be a positive integer, received: ${steps}`); | |
| } | |
| assertNotNull(container); | |
| /* | |
| * Pie sectors at 0% progress do not render at all, so we | |
| * start from 0.1 so that the sectors have rendered a little bit. | |
| */ | |
| let animationProgress = 0.1; | |
| await animationManager.setAnimationProgress(animationProgress); | |
| const stepSize = (1 - animationProgress) / steps; | |
| const initialPieSectors = selectPieSectors(container); | |
| const initialAngles = getPieSectorAngles(initialPieSectors); | |
| const initialAttributes = Array.from(initialPieSectors).map(sector => ({ | |
| fill: sector.getAttribute('fill'), | |
| stroke: sector.getAttribute('stroke'), | |
| })); | |
| const anglesDuringAnimation: { startAngle: number; endAngle: number }[][] = []; | |
| for (let i = 1; i < steps; i += 1) { | |
| animationProgress = 0.1 + i * stepSize; | |
| // eslint-disable-next-line no-await-in-loop | |
| await animationManager.setAnimationProgress(animationProgress); | |
| const currentPieSectors = selectPieSectors(container); | |
| const currentAngles = getPieSectorAngles(currentPieSectors); | |
| anglesDuringAnimation.push(currentAngles); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/helper/expectAnimatedPieAngles.tsx` around lines 9 - 32, The
expectAnimatedPieAngles function accepts a steps parameter that can be negative,
which when used to calculate stepSize will cause the animation progress loop to
run indefinitely and hang tests. Add validation at the beginning of the
expectAnimatedPieAngles function to ensure the steps parameter is positive
(greater than 0), similar to validation patterns applied elsewhere in the
codebase, and throw an appropriate error if the validation fails.
Comment on lines
+10
to
+35
| steps: number = 5, | ||
| ): Promise<ReadonlyArray<ReadonlyArray<string>>> { | ||
| assertNotNull(container); | ||
| /* | ||
| * Pie sectors at 0% progress do not render at all, so we | ||
| * start from 0.1 so that the sectors have rendered a little bit. | ||
| */ | ||
| let animationProgress = 0.1; | ||
| await animationManager.setAnimationProgress(animationProgress); | ||
| const stepSize = (1 - animationProgress) / steps; | ||
| const initialPieSectors = selectPieSectors(container); | ||
| expect(initialPieSectors.length).toBeGreaterThan(0); | ||
| const getD = (sector: Element) => { | ||
| const trimmed = trim(sector.getAttribute('d')); | ||
| assertNotNull(trimmed); | ||
| return trimmed; | ||
| }; | ||
| const initialPathDs = Array.from(initialPieSectors).map(getD); | ||
| const initialAttributes = Array.from(initialPieSectors).map(sector => ({ | ||
| fill: sector.getAttribute('fill'), | ||
| stroke: sector.getAttribute('stroke'), | ||
| })); | ||
|
|
||
| const pathDsDuringAnimation: string[][] = []; | ||
| for (animationProgress += stepSize; animationProgress < 1; animationProgress += stepSize) { | ||
| // eslint-disable-next-line no-await-in-loop |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard steps to prevent non-terminating animation loops.
steps is unvalidated, so negative values make the loop run indefinitely (animationProgress decreases forever while < 1 stays true). This can hang the test process.
Suggested fix
export async function expectAnimatedPiePaths(
container: Element,
animationManager: MockAnimationManager,
steps: number = 5,
): Promise<ReadonlyArray<ReadonlyArray<string>>> {
+ if (!Number.isInteger(steps) || steps <= 0) {
+ throw new Error(`steps must be a positive integer, received: ${steps}`);
+ }
assertNotNull(container);
@@
- for (animationProgress += stepSize; animationProgress < 1; animationProgress += stepSize) {
+ for (let i = 1; i < steps; i += 1) {
+ animationProgress = 0.1 + i * stepSize;
// eslint-disable-next-line no-await-in-loop
await animationManager.setAnimationProgress(animationProgress);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/helper/expectAnimatedPiePaths.ts` around lines 10 - 35, The `steps`
parameter in this function is unvalidated and can accept negative values, which
causes the loop to run indefinitely since `stepSize` becomes negative (making
`animationProgress` decrease forever while remaining less than 1). Add
validation at the beginning of the function to ensure `steps` is greater than
zero, either by throwing an error for invalid values or by using an assertion.
This validation should occur before the `stepSize` calculation to prevent the
infinite loop condition in the for loop that iterates while `animationProgress <
1`.
|
Tip All tests passed and all changes approved!🟢 UI Tests: 198 tests unchanged |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7488 +/- ##
==========================================
+ Coverage 88.06% 88.13% +0.07%
==========================================
Files 604 606 +2
Lines 13994 14077 +83
Branches 3523 3531 +8
==========================================
+ Hits 12324 12407 +83
Misses 1483 1483
Partials 187 187 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. |
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
1 participant
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.
Description
So I tried to reproduce the Pie + Legend animation problem and turns out that I accidentally fixed it in #7457. So here is at least a test to prevent regressions. I confirmed I can reproduce in browser and in the unit test before that commit, and cannot reproduce after the commit.
Related Issue
Closes #7254
Summary by CodeRabbit