Add unit tests for <AnimatedCounter /> component#1829
Add unit tests for <AnimatedCounter /> component#1829gaurigupta0604 wants to merge 9 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughA new test suite for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@gaurigupta0604 Could you run |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@gaurigupta0604 could you fix this test?
it('calls onEnd when animation finishes', () => {
const onEndMock = jest.fn();
render(<AnimatedCounter end={500} duration={500} onEnd={onEndMock} />);
act(() => {
jest.advanceTimersByTime(500);
});
expect(onEndMock).toHaveBeenCalledTimes(1);
});
There's no onEnd prop in the AnimatedCounter component.
Hi @kasya, You're right, the onEnd prop isn't part of the AnimatedCounter component. I'll remove that test and push the necessary updates shortly. Appreciate your review! |
|
@gaurigupta0604 any updates on this? |
|
@arkid15r, yes — I’m working on it right now. I’ll remove the invalid test and push the updated changes shortly. Thanks for the reminder! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/sentry.server.config.ts (1)
2-2: Consider removing unused ENVIRONMENT import.The
ENVIRONMENTvariable is imported but no longer used after hardcoding the environment value.-import { SENTRY_DSN, ENVIRONMENT, RELEASE_VERSION } from 'utils/credentials' +import { SENTRY_DSN, RELEASE_VERSION } from 'utils/credentials'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
frontend/package.json(3 hunks)frontend/src/components/AnimatedCounter.test.tsx(1 hunks)frontend/src/components/AnimatedCounter.tsx(2 hunks)frontend/src/sentry.server.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/AnimatedCounter.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: the react-apexcharts chart component does not support aria attributes like aria-label and role as di...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/AnimatedCounter.tsx
🔇 Additional comments (7)
frontend/src/components/AnimatedCounter.tsx (3)
8-9: LGTM! Good addition of accessibility and callback props.The addition of optional
aria-labelandonEndprops enhances both accessibility and functionality of the component.
35-37: LGTM! Proper onEnd callback implementation.The
onEndcallback is correctly invoked only when the animation completes (progress reaches 1) and is properly included in the dependency array to avoid stale closures.Also applies to: 41-41
46-46: LGTM! Good accessibility enhancement.Adding
role="status"is appropriate for a counter that updates dynamically, as it indicates to screen readers that this content may change.frontend/package.json (4)
14-16: LGTM! Good test script standardization.The unified use of
cross-env NODE_OPTIONS=--experimental-vm-modulesacross test scripts improves consistency and cross-platform compatibility.
87-87: LGTM! Appropriate dependency updates for test infrastructure.The addition of
cross-envandwhichpackages supports the updated test scripts and tooling needs.Also applies to: 113-114
88-89: ESLint patch downgrade validatedWe reviewed
frontend/eslint.config.mjsand confirmed there are no hardcoded ESLint version checks or usage of features introduced after 9.31.0. A patch-level downgrade within the 9.x line will not break existing rules or plugin integrations.No further action required.
100-100: ts-jest fully removed; safe to proceedNo references to
ts-jestremain infrontend/jest.config.tsor elsewhere in the repo. All existing TypeScript tests will run under the new runner without issue.
|
Hi @kasya, I’ve removed the invalid |
|
@gaurigupta0604 hi! Please clean up this PR - your proposed changes should only include the test file. |
|
@gaurigupta0604 Hi! The deadline for this issue was Jul 30th. I appreciate you working on this, but since this PR is still not ready for review and it's been 4 days past deadline - I'm going to close it and reassign the issue to someone else. |
3 participants
Proposed change
Added unit tests for the
<AnimatedCounter />component to improve test coverage and ensure correct behavior across different animation types, roles, and accessibility attributes.Resolves #1798
Checklist
yarn testlocally; all checks and tests passed