test: enhance line animation tests for ComposedChart and responsive#7289
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new test suite that validates Line entrance-animation behavior across composed and responsive charts, ensuring SVG path Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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: 1
🧹 Nitpick comments (2)
test/cartesian/Line.animation.spec.tsx (2)
570-577: ⚡ Quick winRe-query line nodes after advancing animation progress.
Line 570 stores
.recharts-line-curvenodes beforesetAnimationProgress(0.5), then reuses them at Line 575-576. If re-render swaps path nodes, these references can go stale and make the test flaky.Suggested fix
- const lines = Array.from(container.querySelectorAll<SVGPathElement>('.recharts-line-curve')); + let lines = Array.from(container.querySelectorAll<SVGPathElement>('.recharts-line-curve')); expect(lines).toHaveLength(2); expect(lines[0]).toHaveAttribute('stroke-dasharray'); await animationManager.setAnimationProgress(0.5); + lines = Array.from(container.querySelectorAll<SVGPathElement>('.recharts-line-curve')); expect(lines[0]).toHaveAttribute('stroke-dasharray', '50px 100px'); expect(lines[1]).toHaveAttribute('stroke-dasharray', '50px 100px');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/Line.animation.spec.tsx` around lines 570 - 577, The test captures SVG path elements into the local variable lines before calling animationManager.setAnimationProgress(0.5), which can yield stale DOM references if the component re-renders; update the test in Line.animation.spec.tsx to re-query container.querySelectorAll<SVGPathElement>('.recharts-line-curve') after calling setAnimationProgress(0.5) (i.e., call the same query again into a new or overwritten lines variable before asserting stroke-dasharray on lines[0] and lines[1]) so assertions use fresh DOM nodes.
334-351: ⚡ Quick winAdd selector call-count assertions for the new selector-based test cases.
The new
createSelectorTestCasescenarios don’t validate selector invocation counts, so unnecessary re-renders in these new flows could slip through.As per coding guidelines, “Verify the number of selector calls using the spy object from
createSelectorTestCaseto spot unnecessary re-renders and improve performance.”Also applies to: 500-505, 604-605
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/Line.animation.spec.tsx` around lines 334 - 351, The new selector-based tests created with createSelectorTestCase (e.g., the renderTestCase defined around the Line animation tests) are missing assertions that verify selector invocation counts; update each selector-based test case (including the ones around lines cited: 334-351, 500-505, 604-605) to assert the spy object returned by createSelectorTestCase was called the expected number of times after each render/update to catch unnecessary re-renders—locate the spy from the createSelectorTestCase helper and add assertions like spy.callCount (or the helper’s exposed API) after initial render and after any prop/state changes in the test flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cartesian/Line.animation.spec.tsx`:
- Around line 606-614: The test currently only asserts the stroke-dasharray is
not "0px 0px" after rerender, which allows partial regressions; change the
assertion to capture the pre-rerender value and assert it is preserved exactly
after rerender. Concretely, in the test "should not reset the visible stroke
length when Legend is added mid-animation" use getLine(container) to read and
store the initial stroke-dasharray after await
animationManager.setAnimationProgress(0.5) (e.g., const initial =
getLine(container).getAttribute('stroke-dasharray')), then call
rerender(ChartWithLegend) and assert
getLine(container).getAttribute('stroke-dasharray') === initial; apply the same
change to the other similar test block noted (lines 616-624).
---
Nitpick comments:
In `@test/cartesian/Line.animation.spec.tsx`:
- Around line 570-577: The test captures SVG path elements into the local
variable lines before calling animationManager.setAnimationProgress(0.5), which
can yield stale DOM references if the component re-renders; update the test in
Line.animation.spec.tsx to re-query
container.querySelectorAll<SVGPathElement>('.recharts-line-curve') after calling
setAnimationProgress(0.5) (i.e., call the same query again into a new or
overwritten lines variable before asserting stroke-dasharray on lines[0] and
lines[1]) so assertions use fresh DOM nodes.
- Around line 334-351: The new selector-based tests created with
createSelectorTestCase (e.g., the renderTestCase defined around the Line
animation tests) are missing assertions that verify selector invocation counts;
update each selector-based test case (including the ones around lines cited:
334-351, 500-505, 604-605) to assert the spy object returned by
createSelectorTestCase was called the expected number of times after each
render/update to catch unnecessary re-renders—locate the spy from the
createSelectorTestCase helper and add assertions like spy.callCount (or the
helper’s exposed API) after initial render and after any prop/state changes in
the test flow.
🪄 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: 8f3d3c67-0db8-4c4a-91c6-2b586b0aeb33
📒 Files selected for processing (1)
test/cartesian/Line.animation.spec.tsx
Comment on lines
+606
to
+614
| it('should not reset the visible stroke length when Legend is added mid-animation', async () => { | ||
| const { container, animationManager, rerender } = renderTestCase(); | ||
|
|
||
| await animationManager.setAnimationProgress(0.5); | ||
| expect(getLine(container)).toHaveAttribute('stroke-dasharray', '50px 100px'); | ||
|
|
||
| rerender(ChartWithLegend); | ||
| expect(getLine(container)).not.toHaveAttribute('stroke-dasharray', '0px 0px'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert preserved progress value, not only “not reset to zero.”
Line 613 and Line 623 only assert the value is not 0px 0px, which still allows partial regressions (e.g. dropping from 50px 100px to 10px 100px). For this regression test, assert preservation of the pre-rerender value.
Suggested fix
await animationManager.setAnimationProgress(0.5);
- expect(getLine(container)).toHaveAttribute('stroke-dasharray', '50px 100px');
+ const dashBeforeLegend = getLine(container).getAttribute('stroke-dasharray');
+ expect(dashBeforeLegend).toBe('50px 100px');
rerender(ChartWithLegend);
- expect(getLine(container)).not.toHaveAttribute('stroke-dasharray', '0px 0px');
+ expect(getLine(container)).toHaveAttribute('stroke-dasharray', dashBeforeLegend);
await animationManager.setAnimationProgress(0.5);
- expect(getLine(container)).toHaveAttribute('stroke-dasharray', '50px 100px');
+ const dashBeforeAutoYAxis = getLine(container).getAttribute('stroke-dasharray');
+ expect(dashBeforeAutoYAxis).toBe('50px 100px');
rerender(ChartWithAutoYAxis);
- expect(getLine(container)).not.toHaveAttribute('stroke-dasharray', '0px 0px');
+ expect(getLine(container)).toHaveAttribute('stroke-dasharray', dashBeforeAutoYAxis);Also applies to: 616-624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cartesian/Line.animation.spec.tsx` around lines 606 - 614, The test
currently only asserts the stroke-dasharray is not "0px 0px" after rerender,
which allows partial regressions; change the assertion to capture the
pre-rerender value and assert it is preserved exactly after rerender.
Concretely, in the test "should not reset the visible stroke length when Legend
is added mid-animation" use getLine(container) to read and store the initial
stroke-dasharray after await animationManager.setAnimationProgress(0.5) (e.g.,
const initial = getLine(container).getAttribute('stroke-dasharray')), then call
rerender(ChartWithLegend) and assert
getLine(container).getAttribute('stroke-dasharray') === initial; apply the same
change to the other similar test block noted (lines 616-624).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7289 +/- ##
=======================================
Coverage 89.05% 89.05%
=======================================
Files 541 541
Lines 41088 41088
Branches 5564 5564
=======================================
Hits 36589 36589
Misses 4491 4491
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7289 +/- ##
=======================================
Coverage 89.05% 89.05%
=======================================
Files 541 541
Lines 41088 41088
Branches 5564 5564
=======================================
Hits 36589 36589
Misses 4491 4491
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Bundle ReportBundle size has no change ✅ |
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Merged
This was referenced May 31, 2026
Merged
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.
This pull request adds comprehensive new test cases to the
Line.animation.spec.tsxfile to improve the coverage of line animation behaviors, particularly in complex and responsive chart scenarios. The new tests focus on verifying the correct animation of lines in composed charts with sparse data, in responsive layouts, and when chart layout changes dynamically during animation.The most important changes include:
New animation tests for complex scenarios:
ComposedChartwith sparse best-fit line data, ensuring the line path remains present and animates correctly asstroke-dasharraygrows during entrance animation.ComposedChartcontinue animating correctly after a late resize event.Responsive and dynamic layout animation tests:
Legendor aYAxiswithwidth="auto") occur mid-animation, the visible stroke length of the animated line is preserved and not reset.Test utility and import improvements:
ComposedChart,Scatter,XAxis) and improved mocking utilities for bounding client rects.Summary by CodeRabbit