Conversation
Added missing unit tests for `isInRollout` function in `src/isInRollout.ts` using bun:test. Tested boundary values (0, 100) and specific hash modulo values based on a deterministic mock. 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. |
📝 WalkthroughWalkthroughAdds deterministic unit tests for isInRollout, a React Native version mock, small dynamic-import TS ignores, sed patches for tests, and renames Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 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)
24-27: Test description could be clearer.The description says "less than or equal to the hash modulo" but the actual condition is
intForUUID % 100 < rollout(strictly less than). When rollout equals 79,79 < 79is false. Consider rephrasing to "when rollout equals the hash modulo" for clarity.✏️ Suggested rewording
- test('returns false when rollout is less than or equal to the hash modulo (79)', () => { - // 79 < 79 is false + test('returns false when rollout equals the hash modulo (79)', () => { + // intForUUID % 100 = 79, so 79 < 79 is false expect(isInRollout(79)).toBe(false); });🤖 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 24 - 27, Update the test description to accurately reflect the condition being tested: change the wording from "returns false when rollout is less than or equal to the hash modulo" to something like "returns false when rollout equals the hash modulo (79)"; locate the test for isInRollout in src/__tests__/isInRollout.test.ts (the test that calls isInRollout(79)) and update its string description and optional inline comment to mention "equals" rather than "less than or equal to" so it matches the actual comparison performed by intForUUID % 100 < rollout in isInRollout.
🤖 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`:
- Around line 14-18: The dynamic import in the test uses a Bun query string
('../isInRollout?deterministic') which TypeScript's resolver flags; update the
import to silence the type error by importing as unknown and asserting the shape
or by adding a single-line TS suppression: e.g., change the import in the
beforeAll block so the result is assigned with a type assertion (cast to the
module type containing isInRollout) or add a // `@ts-ignore` comment just above
the dynamic import, then extract module.isInRollout into the isInRollout
variable; reference the beforeAll block and the isInRollout symbol when making
this change.
---
Nitpick comments:
In `@src/__tests__/isInRollout.test.ts`:
- Around line 24-27: Update the test description to accurately reflect the
condition being tested: change the wording from "returns false when rollout is
less than or equal to the hash modulo" to something like "returns false when
rollout equals the hash modulo (79)"; locate the test for isInRollout in
src/__tests__/isInRollout.test.ts (the test that calls isInRollout(79)) and
update its string description and optional inline comment to mention "equals"
rather than "less than or equal to" so it matches the actual comparison
performed by intForUUID % 100 < rollout in isInRollout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d754f053-020a-495d-9150-482e8719ef6d
📒 Files selected for processing (2)
src/__tests__/isInRollout.test.tssrc/__tests__/setup.ts
… variable
- Added `// @ts-ignore` to dynamic query imports (`import('../core?error')`, etc.) which fixes `TS2307: Cannot find module` in `tsc`.
- Renamed `info` variable to `updateInfo` in `downloadUpdate` arguments inside `src/client.ts` to prevent "already declared in upper scope" warnings.
Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
patch_tests.sh (1)
1-3: Add fail-fast safeguards to detect silent patch failures.The script lacks error handling and post-condition checks. Because
sedexits 0 even when a pattern fails to match, undetected formatting drift in the test files can leave imports only partially patched. Add a shebang, strict mode, and verification checks to catch mismatches immediately.Suggested hardening
+#!/usr/bin/env sh +set -eu + sed -i 's/await import('\''\.\.\/isInRollout?deterministic'\'')/await import('\''\.\.\/isInRollout?deterministic'\'' \/\* `@vite-ignore` \*\/) as any/' src/__tests__/isInRollout.test.ts sed -i 's/await import('\''\.\.\/core?error'\'')/await import('\''\.\.\/core?error'\'' \/\* `@vite-ignore` \*\/) as any/' src/__tests__/core.test.ts sed -i 's/await import('\''\.\.\/core?success'\'')/await import('\''\.\.\/core?success'\'' \/\* `@vite-ignore` \*\/) as any/' src/__tests__/core.test.ts + +grep -Fq "await import('../isInRollout?deterministic' /* `@vite-ignore` */) as any" src/__tests__/isInRollout.test.ts +grep -Fq "await import('../core?error' /* `@vite-ignore` */) as any" src/__tests__/core.test.ts +grep -Fq "await import('../core?success' /* `@vite-ignore` */) as any" src/__tests__/core.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch_tests.sh` around lines 1 - 3, Add a shebang and strict shell options (set -euo pipefail) at the top of the script, run each sed replacement as you already do, then verify each replacement succeeded by checking for absence of the original patterns (e.g. "await import('../isInRollout?deterministic')", "await import('../core?error')", "await import('../core?success')") and presence of the expected patched strings (with "/* `@vite-ignore` */) as any"); if any verification fails, print a clear error mentioning which pattern failed and exit non‑zero so the CI fails fast. Ensure the checks run after all sed invocations so partial/missed patches are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patch_client.sh`:
- Around line 1-4: The patch script uses fragile line-numbered sed edits and
fails silently when the expected snippets aren't present; update patch_client.sh
to perform content-based replacements for the tokens "info: CheckResult" ->
"updateInfo: CheckResult" and "info.updateUrl"/"info.hash" ->
"updateInfo.updateUrl"/"updateInfo.hash" by matching and replacing those exact
strings in src/client.ts (use global/content-aware sed or perl regex) and then
assert each replacement succeeded (e.g., check the file diff, count
replacements, or verify the new strings exist) and exit non‑zero if any expected
replacement did not occur so the script cannot succeed without applying the
intended edits.
---
Nitpick comments:
In `@patch_tests.sh`:
- Around line 1-3: Add a shebang and strict shell options (set -euo pipefail) at
the top of the script, run each sed replacement as you already do, then verify
each replacement succeeded by checking for absence of the original patterns
(e.g. "await import('../isInRollout?deterministic')", "await
import('../core?error')", "await import('../core?success')") and presence of the
expected patched strings (with "/* `@vite-ignore` */) as any"); if any
verification fails, print a clear error mentioning which pattern failed and exit
non‑zero so the CI fails fast. Ensure the checks run after all sed invocations
so partial/missed patches are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e02822a-ce02-4735-85c0-06f058df6459
📒 Files selected for processing (6)
patch_client.shpatch_client2.shpatch_tests.shsrc/__tests__/core.test.tssrc/__tests__/isInRollout.test.tssrc/client.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/core.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/isInRollout.test.ts
| sed -i '365s/info: CheckResult/updateInfo: CheckResult/' src/client.ts | ||
| sed -i '368s/info.updateUrl/updateInfo.updateUrl/' src/client.ts | ||
| sed -i '369s/info.hash/updateInfo.hash/' src/client.ts | ||
| sed -i '381s/info.hash/updateInfo.hash/' src/client.ts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo "== addressed lines in src/client.ts =="
sed -n '365p;368p;369p;381p' src/client.ts
echo
echo "== old identifiers still present? =="
rg -n "info: CheckResult|info\\.updateUrl|info\\.hash" src/client.ts || trueRepository: reactnativecn/react-native-update
Length of output: 214
These line-numbered rewrites are already out of sync with src/client.ts.
On the checked-in file, Lines 368, 369, and 381 no longer contain the info.updateUrl / info.hash text this script expects, so it can exit successfully while applying nothing. Use content-based replacements and assert that each rewrite happened.
Suggested direction
-sed -i '365s/info: CheckResult/updateInfo: CheckResult/' src/client.ts
-sed -i '368s/info.updateUrl/updateInfo.updateUrl/' src/client.ts
-sed -i '369s/info.hash/updateInfo.hash/' src/client.ts
-sed -i '381s/info.hash/updateInfo.hash/' src/client.ts
+sed -i 's/info: CheckResult/updateInfo: CheckResult/' src/client.ts
+sed -i 's/} = info;/} = updateInfo;/' src/client.ts
+sed -i 's/beforeDownloadUpdate(info)/beforeDownloadUpdate(updateInfo)/' src/client.ts
+sed -i 's/if (!info.update || !hash) {/if (!updateInfo.update || !hash) {/' src/client.ts
+grep -Fq 'updateInfo: CheckResult' src/client.ts
+grep -Fq '} = updateInfo;' src/client.ts
+grep -Fq 'beforeDownloadUpdate(updateInfo)' src/client.ts
+grep -Fq 'if (!updateInfo.update || !hash) {' src/client.ts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed -i '365s/info: CheckResult/updateInfo: CheckResult/' src/client.ts | |
| sed -i '368s/info.updateUrl/updateInfo.updateUrl/' src/client.ts | |
| sed -i '369s/info.hash/updateInfo.hash/' src/client.ts | |
| sed -i '381s/info.hash/updateInfo.hash/' src/client.ts | |
| sed -i 's/info: CheckResult/updateInfo: CheckResult/' src/client.ts | |
| sed -i 's/} = info;/} = updateInfo;/' src/client.ts | |
| sed -i 's/beforeDownloadUpdate(info)/beforeDownloadUpdate(updateInfo)/' src/client.ts | |
| sed -i 's/if (!info.update || !hash) {/if (!updateInfo.update || !hash) {/' src/client.ts | |
| grep -Fq 'updateInfo: CheckResult' src/client.ts | |
| grep -Fq '} = updateInfo;' src/client.ts | |
| grep -Fq 'beforeDownloadUpdate(updateInfo)' src/client.ts | |
| grep -Fq 'if (!updateInfo.update || !hash) {' src/client.ts |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patch_client.sh` around lines 1 - 4, The patch script uses fragile
line-numbered sed edits and fails silently when the expected snippets aren't
present; update patch_client.sh to perform content-based replacements for the
tokens "info: CheckResult" -> "updateInfo: CheckResult" and
"info.updateUrl"/"info.hash" -> "updateInfo.updateUrl"/"updateInfo.hash" by
matching and replacing those exact strings in src/client.ts (use
global/content-aware sed or perl regex) and then assert each replacement
succeeded (e.g., check the file diff, count replacements, or verify the new
strings exist) and exit non‑zero if any expected replacement did not occur so
the script cannot succeed without applying the intended edits.
1 participant
🎯 What: The testing gap addressed: Missing unit tests for the
isInRolloutfunction insrc/isInRollout.ts.📊 Coverage: Scenarios now tested include 0% rollout, 100% rollout, and boundary values based on the specific
murmurhash3_32_gcmodulo of a mocked UUID.✨ Result: Test coverage for
isInRollout.tsis improved, ensuring the feature logic evaluates deterministically and correctly.PR created automatically by Jules for task 10179981082815151273 started by @sunnylqm
Summary by CodeRabbit
Tests
Chores