Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRe-enables the BrowserStack Android CI job; cleans and documents the BrowserStack E2E workflow; adds a post-test step to extract Detox session_id and query BrowserStack; updates Detox runner args; and applies Android packaging, settings, and npm script changes. ChangesBrowserStack ID-based app resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
tsdamas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
PingTestRunner/package.json (1)
24-24: 💤 Low valueConsider aligning Gradle task qualification across build scripts.
The
build:bs:androidscript now uses fully-qualified task names (:app:assembleRelease :app:assembleAndroidTest), which is more explicit and safer in multi-project builds. However, the.detoxrc.jsfile'sandroid.releasebuild command (line 40) still uses unqualified task names (assembleRelease assembleAndroidTest).While these serve different code paths (BrowserStack vs. local), the inconsistency could create maintenance confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PingTestRunner/package.json` at line 24, The android release build command is inconsistent: package.json's "build:bs:android" uses fully-qualified Gradle tasks while .detoxrc.js's android.release uses unqualified task names; update the android.release build command in .detoxrc.js to use the fully-qualified tasks (e.g., replace "assembleRelease assembleAndroidTest" with ":app:assembleRelease :app:assembleAndroidTest") so both build scripts use the same explicit task qualification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@PingTestRunner/package.json`:
- Line 24: The android release build command is inconsistent: package.json's
"build:bs:android" uses fully-qualified Gradle tasks while .detoxrc.js's
android.release uses unqualified task names; update the android.release build
command in .detoxrc.js to use the fully-qualified tasks (e.g., replace
"assembleRelease assembleAndroidTest" with ":app:assembleRelease
:app:assembleAndroidTest") so both build scripts use the same explicit task
qualification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a4487e1-002e-415b-9af7-d883813c10d9
⛔ Files ignored due to path filters (2)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.github/workflows/browserstack-e2e-android.ymlPingTestRunner/android/app/build.gradlePingTestRunner/android/settings.gradlePingTestRunner/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/browserstack-e2e-android.yml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/browserstack-e2e-android.yml:
- Around line 207-223: Change the BrowserStack session fetch in the View
BrowserStack test results step to call the correct GET endpoint and surface curl
failures: replace the POST to
"https://api-cloud.browserstack.com/app-automate/detox/v2/android/sessions/${session_id}"
with a GET to
"https://api-cloud.browserstack.com/app-automate/detox/v2/android/sessions/${session_id}.json",
run curl so it fails on non-2xx responses (check exit code / use --fail) and
print a clear warning including the session_id on error; optionally pipe the
response through jq to pretty-print successful JSON. Ensure you update the curl
invocation that references session_id and the step that echoes "Fetching
BrowserStack session details for session: $session_id".
In `@PingTestRunner/android/app/src/main/res/xml/network_security_config.xml`:
- Around line 15-27: The network_security_config.xml in src/main currently
enables cleartext globally via the <base-config cleartextTrafficPermitted="true"
/> referenced by android:networkSecurityConfig and combined with
android:usesCleartextTraffic="true", which makes cleartext available to all
builds; move that permissive base-config into a variant-scoped overlay (e.g.,
src/debug/res/xml/network_security_config.xml or
src/androidTest/res/xml/network_security_config.xml) and remove or replace the
permissive <base-config> in src/main/res/xml/network_security_config.xml with a
restrictive policy, then update the debug/androidTest manifest overlay (or
manifestPlaceholders) to reference the permissive resource (and/or set
android:usesCleartextTraffic="true" only in debug/androidTest) so only
Detox/Test runs (DetoxTest.kt targeting ws://10.0.2.2) allow cleartext.
🪄 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: bd8907c4-5f00-42e9-a015-f809e975a496
📒 Files selected for processing (3)
.github/workflows/browserstack-e2e-android.ymlPingTestRunner/.detoxrc.jsPingTestRunner/android/app/src/main/res/xml/network_security_config.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- PingTestRunner/.detoxrc.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PingTestRunner/.detoxrc.js`:
- Line 20: Remove the forced CI override of maxWorkers in the Detox config by
eliminating or conditionalizing the line setting maxWorkers: process.env.CI ? 2
: undefined; instead let Detox/Jest pick the appropriate worker count (or
explicitly set it to 1) so it does not conflict with testRunner.args.maxWorkers
and the e2e/jest.config.js maxWorkers: 1; update .detoxrc.js to either omit
maxWorkers entirely on CI or set it to 1 to ensure only one worker/device
session is used.
🪄 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: 9827f892-46e9-444d-9f4a-3137e5fb0899
📒 Files selected for processing (1)
PingTestRunner/.detoxrc.js
tsdamas
requested review from
pingidentity-gaurav,
rodrigoareis and
spetrov
and removed request for
rodrigoareis
pingidentity-gaurav
left a comment
pingidentity-gaurav
left a comment
There was a problem hiding this comment.
Code review findings
| browserstack-android-journey: | ||
| needs: browserstack-android-core | ||
| if: ${{ always() }} | ||
| permissions: |
There was a problem hiding this comment.
This causes the journey job to run even when core fails, burning BrowserStack credits on a broken build. Unless the two suites are genuinely independent (journey tests don't rely on anything core validates), consider dropping always() so journey is skipped when core fails.
There was a problem hiding this comment.
Thanks for calling out. This was added for testing purposes only as we are experiencing the issue on their detox fork and will be removed before merging.
…e workflow comment
…ey workflows Extract APK build into a dedicated prep workflow that uploads artifacts, then run core and journey E2E test suites concurrently on BrowserStack instead of serially in a single job.
…al vulnerabilities
…hApp failures testRunner.jest.retries only retries individual test bodies and cannot recover from a beforeAll failure. Adding testRunner.retries: 1 causes Detox to re-run the entire failed suite file (including beforeAll) when BrowserStack returns a pool-exhaustion launchApp error. Local runs are unaffected (retries: 0 when CI is unset).
2 participants
Summary