test(LogUtils): add missing tests for undefined format#6739
test(LogUtils): add missing tests for undefined format#6739ckifer merged 1 commit intorecharts:mainfrom
Conversation
WalkthroughA new test case added to LogUtils.spec.ts that verifies console.warn is called with the correct error message when the format parameter is undefined in both mocked and unmocked scenarios. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/util/LogUtils.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run type checking on the codebase using
npm run check-types
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas const
Files:
test/util/LogUtils.spec.ts
{test,www/test}/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Write unit tests in the
testorwww/testdirectories with.spec.tsxfile extension
Files:
test/util/LogUtils.spec.ts
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/util/LogUtils.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
test/util/LogUtils.spec.ts
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (test/README.md)
test/**/*.{test,spec}.{ts,tsx}: Aim for 100% unit test code coverage when writing new code
Prefer to use thecreateSelectorTestCasehelper function when writing or modifying tests
Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion
Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance
MockgetBoundingClientRectin tests using the helper function provided intest/helper/MockGetBoundingClientRect.ts
Usevi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame
Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper, and avoidvi.runAllTimers()to prevent infinite loops
UseuserEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })or theuserEventSetuphelper function fromtest/helper/userEventSetup.tswhen creating userEvent instances
When testing tooltips on hover, usevi.runOnlyPendingTimers()after eachuserEvent.hover()call or use theshowTooltiphelper function fromtooltipTestHelpers.tsto account for requestAnimationFrame delays
Files:
test/util/LogUtils.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When running unit tests, prefer to run a single test file using
npm run test -- path/to/TestFile.spec.tsxrather than running all tests withnpm test
Files:
test/util/LogUtils.spec.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
Applied to files:
test/util/LogUtils.spec.ts
| test('warns when format is undefined', () => { | ||
| const logSpy = vi.spyOn(console, 'warn').mockImplementation((): void => undefined); | ||
|
|
||
| // condition is true, format is undefined | ||
| // @ts-expect-error - testing undefined format | ||
| warn(true, undefined); | ||
| expect(logSpy).toHaveBeenCalledWith('LogUtils requires an error message argument'); | ||
|
|
||
| logSpy.mockClear(); | ||
|
|
||
| // condition is false, format is undefined | ||
| // @ts-expect-error - testing undefined format | ||
| warn(false, undefined); | ||
| expect(logSpy).toHaveBeenCalledWith('LogUtils requires an error message argument'); | ||
| expect(logSpy).toHaveBeenCalledWith( | ||
| 'Minified exception occurred; use the non-minified dev environment for the full error message and additional helpful warnings.', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for expectLastCalledWith helper definition
rg -n "export.*expectLastCalledWith" test/Repository: recharts/recharts
Length of output: 298
🏁 Script executed:
# Check the full LogUtils.spec.ts file to see current imports and test setup
cat -n test/util/LogUtils.spec.tsRepository: recharts/recharts
Length of output: 1067
🏁 Script executed:
# Check if expectLastCalledWith is imported in this file
rg -n "expectLastCalledWith\|import" test/util/LogUtils.spec.tsRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Verify the test file exists and check its size
wc -l test/util/LogUtils.spec.ts
stat test/util/LogUtils.spec.ts 2>/dev/null || echo "File check"Repository: recharts/recharts
Length of output: 101
🏁 Script executed:
# Check git status to see if the file has uncommitted changes
cd test/util && git diff LogUtils.spec.ts 2>/dev/null || echo "No git diff available"Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Run type checking on the entire test directory
npm run check-typesRepository: recharts/recharts
Length of output: 239
🏁 Script executed:
# Run linting on LogUtils.spec.ts
npm run lint -- test/util/LogUtils.spec.tsRepository: recharts/recharts
Length of output: 166
🏁 Script executed:
# Try to find the helper import location to understand the proper import
cat -n test/helper/expectLastCalledWith.ts | head -20Repository: recharts/recharts
Length of output: 990
🏁 Script executed:
# Check other test files to see how they import and use expectLastCalledWith
rg -B2 "expectLastCalledWith" test/ --type ts --type tsx -A2 | head -50Repository: recharts/recharts
Length of output: 88
🏁 Script executed:
# Check the warn function implementation to understand the expected behavior
cat -n src/util/LogUtils.tsRepository: recharts/recharts
Length of output: 928
🏁 Script executed:
# Check existing test patterns in the repository to understand vi.useFakeTimers usage
rg -l "vi.useFakeTimers" test/ --type ts --type tsx | head -10Repository: recharts/recharts
Length of output: 88
🏁 Script executed:
# See if LogUtils tests need timers
rg "vi.useFakeTimers\|requestAnimationFrame\|setTimeout" test/util/LogUtils.spec.tsRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check if there's test setup that mocks NODE_ENV
rg -B5 "NODE_ENV\|LogUtils" test/util/LogUtils.spec.tsRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check if existing tests have any setup that ensures isDev is true
rg -B10 "describe.*LogUtils" test/util/LogUtils.spec.tsRepository: recharts/recharts
Length of output: 167
🏁 Script executed:
# Look for vitest setup files
fd -e "setup\|config" test/ | grep -E "(setup|config|vitest)" | head -10Repository: recharts/recharts
Length of output: 386
🏁 Script executed:
# Check how other test files handle similar scenarios
rg "vi.spyOn.*console" test/ --type ts -B2 -A5 | head -40Repository: recharts/recharts
Length of output: 2924
🏁 Script executed:
# Let me check the vitest config to understand the test environment
cat vitest.config.ts 2>/dev/null || cat vite.config.ts 2>/dev/null || echo "Config not found"Repository: recharts/recharts
Length of output: 77
🏁 Script executed:
# Check package.json for test environment configuration
jq '.scripts, .vitest' package.json 2>/dev/null | head -30Repository: recharts/recharts
Length of output: 1957
🏁 Script executed:
# Verify once more what NODE_ENV the tests run under
grep -r "NODE_ENV\|process.env" test/util/ 2>/dev/null | head -5Repository: recharts/recharts
Length of output: 43
Fix test logic and use expectLastCalledWith for better typing.
The test at line 31-33 has a logical error: warn(true, undefined) should NOT call console.warn based on the implementation, but the test expects it to be called. This contradicts the warn() function logic (lines 10-21 of src/util/LogUtils.ts) where console.warn is only called when !condition.
For the second scenario (lines 38-43), use expectLastCalledWith helper instead of expect(logSpy).toHaveBeenCalledWith() for better TypeScript typing. Also add explicit call count and nth call assertions to verify the expected behavior:
test('warns when format is undefined', () => {
const logSpy = vi.spyOn(console, 'warn').mockImplementation((): void => undefined);
// condition is true, format is undefined
// @ts-expect-error - testing undefined format
warn(true, undefined);
- expect(logSpy).toHaveBeenCalledWith('LogUtils requires an error message argument');
+ expect(logSpy).not.toHaveBeenCalled();
logSpy.mockClear();
// condition is false, format is undefined
// @ts-expect-error - testing undefined format
warn(false, undefined);
+ expect(logSpy).toHaveBeenCalledTimes(2);
- expect(logSpy).toHaveBeenCalledWith('LogUtils requires an error message argument');
- expect(logSpy).toHaveBeenCalledWith(
- 'Minified exception occurred; use the non-minified dev environment for the full error message and additional helpful warnings.',
- );
+ expect(logSpy).toHaveBeenNthCalledWith(1, 'LogUtils requires an error message argument');
+ expectLastCalledWith(
+ logSpy,
+ 'Minified exception occurred; use the non-minified dev environment for the full error message and additional helpful warnings.',
+ );
});Import expectLastCalledWith from test/helper/expectLastCalledWith.ts at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
| // condition is true, format is undefined | ||
| // @ts-expect-error - testing undefined format | ||
| warn(true, undefined); | ||
| expect(logSpy).toHaveBeenCalledWith('LogUtils requires an error message argument'); |
There was a problem hiding this comment.
since this is internal and we use typescript I don't really see any value in this test but doesn't hurt anything I suppose
There was a problem hiding this comment.
Thank you for the feedback!
I understand that the test may not add major value given TypeScript’s checks, but I added it for completeness since the undefined-format branch wasn’t covered earlier.
If everything looks fine to you, please feel free to merge it.
I’m happy to update or adjust anything you’d prefer — just let me know! 🙏
There was a problem hiding this comment.
Sir very Thank you for merging my pr 🙏
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6739 +/- ##
==========================================
+ Coverage 94.02% 94.04% +0.01%
==========================================
Files 500 500
Lines 42685 42685
Branches 4917 4918 +1
==========================================
+ Hits 40136 40141 +5
+ Misses 2544 2539 -5
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR adds missing test cases for LogUtils to cover scenarios where the format argument is undefined.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.