Conversation
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughA new test suite was added for rollout functionality, including a test file that validates hash determinism and rollout computation logic, a test setup file that mocks React Native version information, and the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/isInRollout.test.ts (1)
34-36: Consider asserting exact empty-string hash output.
typeof === 'number'is weak here; checking the exact deterministic output gives stronger regression protection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/isInRollout.test.ts` around lines 34 - 36, Replace the weak typeof assertion in the test for murmurhash3_32_gc('') with an exact equality check against the deterministic numeric hash (expect(murmurhash3_32_gc('')).toBe(0)); update the test case in src/__tests__/isInRollout.test.ts to assert the exact output of murmurhash3_32_gc for the empty string instead of using typeof.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/isInRollout.test.ts`:
- Line 50: The dynamic import cache-busting currently uses Date.now() which can
collide; replace it with a monotonic nonce so each import path is unique across
rapid consecutive tests. In the test where you do const { isInRollout } = await
import(`../isInRollout?id=${Date.now()}`); introduce a monotonic counter (e.g.,
a module-scoped let monotonicNonce = 0 and increment it for each import, or a
helper getMonotonicNonce that returns ++monotonicNonce or
process.hrtime.bigint().toString()) and use that value in the import template
string instead of Date.now(); ensure the helper is declared in the test module
so the import line for isInRollout uses the monotonic value.
---
Nitpick comments:
In `@src/__tests__/isInRollout.test.ts`:
- Around line 34-36: Replace the weak typeof assertion in the test for
murmurhash3_32_gc('') with an exact equality check against the deterministic
numeric hash (expect(murmurhash3_32_gc('')).toBe(0)); update the test case in
src/__tests__/isInRollout.test.ts to assert the exact output of
murmurhash3_32_gc for the empty string instead of using typeof.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 180a7bad-7c77-4e73-b03e-d9e7d95c5f85
📒 Files selected for processing (3)
src/__tests__/isInRollout.test.tssrc/__tests__/setup.tssrc/isInRollout.ts
| describe('isInRollout', () => { | ||
| it('should return true when the rollout is greater than the hash modulo', async () => { | ||
| mockUuid = 'test1'; | ||
| const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "isInRollout.test.ts" -type f 2>/dev/null | head -5Repository: reactnativecn/react-native-update
Length of output: 59
🏁 Script executed:
find . -name "isInRollout.test.ts" -type fRepository: reactnativecn/react-native-update
Length of output: 112
🏁 Script executed:
cat -n ./src/__tests__/isInRollout.test.tsRepository: reactnativecn/react-native-update
Length of output: 3699
🏁 Script executed:
ls -la ./src/__tests__/ | grep -i setupRepository: reactnativecn/react-native-update
Length of output: 134
🏁 Script executed:
cat -n ./src/__tests__/setup.tsRepository: reactnativecn/react-native-update
Length of output: 1443
🏁 Script executed:
cat -n ./src/isInRollout.ts | head -50Repository: reactnativecn/react-native-update
Length of output: 1773
🏁 Script executed:
cat -n ./src/isInRollout.ts | tail -30Repository: reactnativecn/react-native-update
Length of output: 928
Use a monotonic nonce instead of Date.now() for dynamic-import cache busting.
Date.now() can repeat within the same millisecond, which risks reusing the same module cache key across consecutive tests and causing test flakiness when mockUuid values differ.
Suggested fix
+let importNonce = 0;
+const importFreshIsInRollout = () => import(`../isInRollout?id=${++importNonce}`);
+
describe('isInRollout', () => {
it('should return true when the rollout is greater than the hash modulo', async () => {
mockUuid = 'test1';
- const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`);
+ const { isInRollout } = await importFreshIsInRollout();
expect(isInRollout(25)).toBe(true);
});
it('should return false when the rollout is equal to the hash modulo', async () => {
mockUuid = 'test1';
- const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`);
+ const { isInRollout } = await importFreshIsInRollout();
expect(isInRollout(24)).toBe(false);
});
it('should return false when the rollout is less than the hash modulo', async () => {
mockUuid = 'test1';
- const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`);
+ const { isInRollout } = await importFreshIsInRollout();
expect(isInRollout(23)).toBe(false);
});
it('should evaluate correctly for a different uuid', async () => {
mockUuid = 'test3';
- const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`);
+ const { isInRollout } = await importFreshIsInRollout();
expect(isInRollout(1)).toBe(true);
expect(isInRollout(0)).toBe(false);
expect(isInRollout(-1)).toBe(false);
});
it('should always return false for 0% rollout', async () => {
mockUuid = 'test1';
- const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`);
+ const { isInRollout } = await importFreshIsInRollout();
expect(isInRollout(0)).toBe(false);
});
it('should always return true for 100% rollout', async () => {
mockUuid = 'test1';
- const { isInRollout } = await import(`../isInRollout?id=${Date.now()}`);
+ const { isInRollout } = await importFreshIsInRollout();
expect(isInRollout(100)).toBe(true);
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/isInRollout.test.ts` at line 50, The dynamic import
cache-busting currently uses Date.now() which can collide; replace it with a
monotonic nonce so each import path is unique across rapid consecutive tests. In
the test where you do const { isInRollout } = await
import(`../isInRollout?id=${Date.now()}`); introduce a monotonic counter (e.g.,
a module-scoped let monotonicNonce = 0 and increment it for each import, or a
helper getMonotonicNonce that returns ++monotonicNonce or
process.hrtime.bigint().toString()) and use that value in the import template
string instead of Date.now(); ensure the helper is declared in the test module
so the import line for isInRollout uses the monotonic value.
1 participant
🎯 What: The testing gap in
isInRollout.tshas been addressed. Themurmurhash3_32_gcfunction was not tested for determinism and correct evaluation of UUIDs into rollout percentages.📊 Coverage: Covered both
murmurhash3_32_gchash outputs (determinism, correct output modulo mapping, empty string) and theisInRolloutfunction (correct comparisons against rollout thresholds, evaluating 0% and 100% rollout bounds). Tests utilize dynamic module imports and mocks to evaluate module-level calculated states cleanly.✨ Result: Test coverage significantly improved for algorithm-specific logic ensuring safe and predictable deterministic UUID-based percentage distributions. All 10 tests across the
isInRollout.test.tssuite are passing.PR created automatically by Jules for task 2512462468046287531 started by @sunnylqm
Summary by CodeRabbit
Tests
Refactor