Enhance Android PR CI with Concurrent JDK Matrix #4301
Conversation
… 25. - Updated `.github/workflows/scripts-android.yml` to use a matrix strategy for JDK versions. - Modified `scripts/build-android-app.sh` and `scripts/run-android-instrumentation-tests.sh` to respect `JDK_HOME` env var. - Updated `scripts/lib/cn1ss.sh` to support failing on screenshot mismatch and skipping PR comments via env vars `CN1SS_FAIL_ON_MISMATCH` and `CN1SS_SKIP_COMMENT`. - Configured matrix jobs to fail on mismatch and skip comments for non-default JDKs.
|
👋 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 For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
- Renamed 'current' matrix entry to 'Default: 8'. - Removed JDK 11 and 25 from the matrix as requested. - Updated conditional logic to skip artifact uploads (screenshots, coverage) for non-default runs to prevent conflicts. - Updated test runner script block to skip coverage generation for non-default runs. - Preserved existing logic for JDK setup and env vars for non-default runs.
- Refactored matrix to use `include` with `id`, `name`, and `java_version` to avoid string quoting issues and provide robust logic.
- Removed JDK 11 and 25 from the matrix as requested.
- Updated conditional logic to use `matrix.id` for checking default vs. custom jobs.
- Fixed artifact conflict by strictly limiting uploads to the `default` job using the robust `matrix.id == 'default'` check.
- Added explicit job name `Build Android ${{ matrix.name }}` for better visibility in PR checks.
|
Compared 30 screenshots: 30 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
|
- Removed malformed shell script block in `scripts-android.yml` that caused syntax errors in the emulator runner. - Moved logic to skip coverage report generation into `run-android-instrumentation-tests.sh` using the `CN1SS_SKIP_COVERAGE` env var. - Updated workflow to set `CN1SS_SKIP_COVERAGE=1` for non-default matrix jobs. - Simplified `android-emulator-runner` script input to a single line command.
- Added "Free Disk Space" step for non-default matrix jobs to remove unused large packages (dotnet, ndk, ghc, swift) and prevent `Not enough space to create userdata partition` errors during emulator creation. - Verified that `mvnw` Java version mismatch is non-blocking for the build process.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ "${CN1SS_FAIL_ON_MISMATCH:-0}" = "1" ]; then | ||
| if [ -f "$summary_out" ] && (grep -q "^different|" "$summary_out" || grep -q "^error|" "$summary_out"); then | ||
| cn1ss_log "FATAL: Screenshot mismatches or errors detected (CN1SS_FAIL_ON_MISMATCH=1)" | ||
| return 15 |
There was a problem hiding this comment.
The new CN1SS_FAIL_ON_MISMATCH guard only trips when the summary file contains different| or error|. RenderScreenshotReport emits other non‑success statuses such as missing_expected and missing_actual (e.g., when a reference PNG is absent or the test never produced a screenshot), but those are ignored here, so matrix jobs with CN1SS_FAIL_ON_MISMATCH=1 still exit 0 even though the comparison detected missing screenshots. That defeats the goal of failing non‑default JDK runs on visual regressions; a JDK‑specific omission would be silently green.
Useful? React with 👍 / 👎.
This change enhances the Android PR CI workflow to run tests against multiple JDK versions (Current/8, 11, 17, 21, 25) concurrently. It introduces
JDK_HOMEsupport in build scripts and adds logic tocn1ss.shto enforce failure on screenshot mismatches and suppress redundant PR comments for the new matrix jobs, while preserving the existing behavior for the default run.PR created automatically by Jules for task 3144219580189981125 started by @shai-almog