Add unit tests for occurrences utility function #507 #534
Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new comprehensive test suite for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
Summary of ChangesHello @Abhaytomar2004, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the reliability and correctness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of unit tests for the occurrences utility function. The tests are well-structured and cover many edge cases, including the use of property-based testing, which is excellent. My main feedback is to improve the testing strategy by removing the mock of the escapeRegExp function. Mocking it by re-implementing it makes the tests brittle. Instead, I suggest testing against the real implementation and adding a test to verify the caching behavior using spies. This will make the test suite more robust and reliable.
| beforeEach(() => { | ||
| vi.mocked(escapeRegExp).mockClear(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
With the mock removed, these hooks are no longer correct. We only need to ensure spies are restored after each test.
| beforeEach(() => { | |
| vi.mocked(escapeRegExp).mockClear(); | |
| }); | |
| afterEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| }); |
src/utils/occurrences.spec.ts
Outdated
| it('should call escapeRegExp for patterns', () => { | ||
| occurrences('test', 'test'); | ||
| expect(escapeRegExp).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test was for the mock. Now that we're using the real implementation, we can write a more valuable test to verify the caching behavior, which is a key performance feature of the occurrences function.
it('should use cache for repeated patterns', () => {
const escapeSpy = vi.spyOn(escapeRegExpModule, 'escapeRegExp');
occurrences('test string', 't');
expect(escapeSpy).toHaveBeenCalledTimes(1);
occurrences('another test', 't');
expect(escapeSpy).toHaveBeenCalledTimes(1); // Should not be called again for the same substring
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/utils/occurrences.spec.ts (4)
6-10: Consider removing the mock or using a spy instead.The mock duplicates the real
escapeRegExpimplementation exactly, which defeats the purpose of mocking. Since the test at lines 74-77 verifies thatescapeRegExpis called, consider usingvi.spyOnto spy on the real implementation instead:-vi.mock('./escapeRegExp', () => ({ - escapeRegExp: vi.fn((str: string) => { - return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - }) -})); +vi.spyOn(await import('./escapeRegExp'), 'escapeRegExp');Alternatively, if you're testing the integration between
occurrencesandescapeRegExp, remove the mock entirely.
13-19: Simplify mock lifecycle management.Both
beforeEachandafterEachare clearing mocks, which is redundant. Choose one approach:
- Use only
beforeEachwithvi.clearAllMocks()to ensure clean state before each test, or- Use only
afterEachwithvi.clearAllMocks()to clean up after tests.The current setup with both is unnecessarily verbose.
74-77: Reconsider testing implementation detail.This test verifies that
escapeRegExpis called (implementation detail) rather than testing observable behavior. The tests at lines 65-72 already validate that special characters are handled correctly, which is the important behavior.Consider removing this test or replacing it with a behavior-focused assertion. Implementation-detail tests are fragile and make refactoring harder.
1-165: Consider adding tests for pattern cache behavior.The
occurrencesfunction usesPATTERN_CACHEfor performance optimization (seesrc/utils/occurrences.tslines 11-13), but there are no tests verifying cache behavior. Consider adding tests to validate:
- Patterns are cached on first use
- Subsequent calls with the same substring reuse cached patterns
- Cache behavior doesn't cause issues with different strings but same substring
Example test:
it('should cache patterns for reuse', () => { vi.mocked(escapeRegExp).mockClear(); occurrences('test1', 'test'); expect(escapeRegExp).toHaveBeenCalledTimes(1); occurrences('test2', 'test'); expect(escapeRegExp).toHaveBeenCalledTimes(1); // Should not call again });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonweb-csv-toolbox-wasm/pkg/web_csv_toolbox_wasm_bg.wasmis excluded by!**/*.wasm
📒 Files selected for processing (1)
src/utils/occurrences.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/occurrences.spec.ts (2)
src/utils/escapeRegExp.ts (1)
escapeRegExp(8-10)src/utils/occurrences.ts (1)
occurrences(11-20)
🔇 Additional comments (4)
src/utils/occurrences.spec.ts (4)
21-49: LGTM! Comprehensive basic functionality coverage.The tests correctly validate:
- Single and multiple character counting
- Empty string edge cases (including the subtle
occurrences('', '')= 1)- Empty substring behavior (matching at every position)
- Case sensitivity
All assertions are accurate based on the function's regex lookahead implementation.
51-72: Excellent coverage of overlapping patterns and special character escaping.These tests effectively validate the function's core features:
- Overlapping occurrences using regex lookahead (
'aaa'/'aa'= 2,'abababa'/'aba'= 3)- Proper escaping of regex metacharacters (
.,*,+,?,^,$)The test cases are well-chosen and comprehensive.
93-139: Excellent property-based testing with fast-check.These property tests validate important invariants:
- Non-negativity of counts
- Correct handling when substring exceeds string length (proper use of
fc.pre)- Upper bound validation for non-empty substrings
- Empty substring edge case
- Self-search identity property
The properties are mathematically sound and provide broad validation across random inputs. Great use of property-based testing!
141-164: Comprehensive edge case coverage.The edge cases validate:
- Performance with long strings (1000+ chars)
- Unicode handling including emojis and multi-byte characters
- Whitespace patterns (single, double, multiple spaces)
- Not-found scenarios including length mismatches
These tests ensure the function handles real-world edge cases correctly.
kamiazya
left a comment
kamiazya
left a comment
There was a problem hiding this comment.
Please remove Package-lock.json and web-csv-toolbox-wasm/pkg/web_csv_toolbox_wasm_bg.wasm as they are unrelated to this pull request.
|
@Abhaytomar2004 |
Thanks for reminding , will be reverting the changes you mention earlier , in the latest commit . |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/utils/occurrences.spec.ts (2)
4-4: Good switch to module import for spying.Importing the module namespace enables reliable vi.spyOn without brittle mocks. Nice.
1-1: Remove redundant beforeEach restore; keep only afterEach.Restoring twice is unnecessary. Trim the hook and its import.
-import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, afterEach, vi } from 'vitest'; @@ -describe('occurrences', () => { - beforeEach(() => { - vi.restoreAllMocks(); - }); +describe('occurrences', () => { @@ afterEach(() => { vi.restoreAllMocks(); });Also applies to: 7-13
🧹 Nitpick comments (3)
src/utils/occurrences.spec.ts (3)
68-75: Harden cache-behavior test against accidental pre-population.Use a unique key and assert the called argument; avoids flakiness if someone adds a prior test using 't'.
it('should use cache for repeated patterns', () => { const escapeSpy = vi.spyOn(escapeRegExpModule, 'escapeRegExp'); - occurrences('test string', 't'); - expect(escapeSpy).toHaveBeenCalledTimes(1); + const key = '__CACHE_KEY__'; + occurrences('test string', key); + expect(escapeSpy).toHaveBeenCalledTimes(1); + expect(escapeSpy).toHaveBeenLastCalledWith(key); - occurrences('another test', 't'); + occurrences('another test', key); expect(escapeSpy).toHaveBeenCalledTimes(1); });Optional: for full isolation, reset modules and re-import occurrences within this test before spying.
// vi.resetModules(); const { occurrences } = await import('./occurrences');
58-66: Add a couple more regex metacharacters for completeness.Cover pipe and closing brackets to fully pin escaping behavior.
it('should handle regex special characters by escaping them', () => { expect(occurrences('a.b.c', '.')).toBe(2); expect(occurrences('a*b*c', '*')).toBe(2); expect(occurrences('a+b+c', '+')).toBe(2); expect(occurrences('a?b?c', '?')).toBe(2); expect(occurrences('a^b$c', '^')).toBe(1); expect(occurrences('a$b$c', '$')).toBe(2); + expect(occurrences('a|b|c', '|')).toBe(2); }); @@ it('should handle parentheses and brackets', () => { expect(occurrences('a(b)c', '(')).toBe(1); expect(occurrences('a)b)c', ')')).toBe(2); expect(occurrences('a[b]c', '[')).toBe(1); expect(occurrences('a{b}c', '{')).toBe(1); + expect(occurrences('a[b]c', ']')).toBe(1); + expect(occurrences('a{b}c', '}')).toBe(1); });Also applies to: 77-82
101-109: Prefer generator construction over fc.pre to avoid discarded runs.Build a strictly-longer substring directly; reduces shrink/discard noise and speeds up CI.
- it('property: if substring is longer than string, result should be 0', () => { - fc.assert( - fc.property(fc.string(), fc.string(), (str, substr) => { - fc.pre(substr.length > str.length && substr.length > 0); - const count = occurrences(str, substr); - expect(count).toBe(0); - }) - ); - }); + it('property: if substring is longer than string, result should be 0', () => { + fc.assert( + fc.property( + fc.string(), // str + fc.string({ minLength: 1 }), // suffix to make it longer + (str, suffix) => { + const substr = str + suffix; + expect(occurrences(str, substr)).toBe(0); + } + ) + ); + });Optional: add
{ numRuns: 200 }(and a fixedseed) to fc.assert for reproducibility in CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web-csv-toolbox-wasm/pkg/web_csv_toolbox_wasm_bg.wasmis excluded by!**/*.wasm
📒 Files selected for processing (1)
src/utils/occurrences.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/occurrences.spec.ts (1)
src/utils/occurrences.ts (1)
occurrences(11-20)
🔇 Additional comments (1)
src/utils/occurrences.spec.ts (1)
6-163: Overall: strong, thorough test suite.Good coverage of overlaps, empty substrings, escaping, unicode, and PBT. Aside from minor nits above, this is ready.
|
@kamiazya I've removed the unrelated files as requested. All tests are passing and the PR is ready for your review. |
There was a problem hiding this comment.
@Abhaytomar2004
It's also unrelated, please remove it too.
There was a problem hiding this comment.
@Abhaytomar2004 Hi, just a reminder, since it's last day of the Hactoberfest.
2 participants
This PR adds comprehensive unit tests for the occurrences utility function in src/utils/occurrences.ts. The function counts the number of times a substring appears in a string, including overlapping occurrences using regex lookahead with pattern caching for performance.
Changes Made
Created src/utils/occurrences.spec.ts with 20 comprehensive test cases
Covered basic counting functionality, edge cases, and overlapping patterns
Added property-based tests using fast-check for robust validation
Verified handling of special characters and regex escaping
Ensured all tests pass successfully
Summary by CodeRabbit