Automate Update Site verification#1397
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:
📝 WalkthroughWalkthroughAdds a CI step that runs a new Eclipse upgrade test script which downloads/extracts Eclipse, installs a stable and an RC plugin via the Eclipse director, performs log/conflict checks, writes a structured report, and always uploads logs and the report as an artifact. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GitHubActions as "GitHub Actions (CI)"
participant Script as "releng/update-site-tests/test-update.sh"
participant Eclipse as "Eclipse (download / director / headless)"
participant ArtifactStore as "actions/upload-artifact"
Developer->>GitHubActions: push / open PR
GitHubActions->>Script: run test-update.sh (env: LOGDIR, REPORT_FILE)
Script->>Eclipse: download & extract Eclipse
Script->>Eclipse: install stable plugin (director) -> stable-install.log
Script->>Eclipse: install RC plugin (director) -> rc-install.log
Script->>Script: scan logs, list installed roots, create report.txt
Script->>Eclipse: headless startup check -> headless.log
Script->>GitHubActions: exit and place REPORT_FILE
GitHubActions->>ArtifactStore: upload logs + report (always)
ArtifactStore-->>GitHubActions: artifact stored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 135-148: The script launches Eclipse with -vmargs
-Djava.awt.headless=false which will fail on headless CI; change the invocation
to either run under Xvfb (wrap "$ECLIPSE_HOME/eclipse" with xvfb-run) or set
-Djava.awt.headless=true, and replace the fragile sleep+kill logic that uses PID
and sleep 20 with a timeout utility (e.g., use timeout to limit launch duration)
and add a robust startup check (verify $PID is running and/or scan
"$LOGDIR/eclipse-launch.log" for a successful startup marker) so the test
reliably times out or confirms a running process instead of blindly killing
after 20s.
- Around line 1-2: Add `set -o pipefail` after the existing `set -e` in
test-update.sh to ensure pipe failures aren't masked, then update the two piped
commands guarded by `PIPESTATUS` (the two places currently checking
PIPESTATUS[0], referenced in the comment as lines with PIPESTATUS checks) so
they won't be prematurely aborted: either temporarily disable `set -e` around
each pipeline or append `|| true` to the pipeline and continue to use the
existing `PIPESTATUS` checks to detect failures; ensure you apply the same
pattern to the RC install pipeline as well so both pipelines reliably surface
errors.
- Around line 111-115: Change the broad grep that searches for "conflict" in
$LOGDIR/*.log to first ensure there are log files (e.g., test if any
"$LOGDIR"/*.log exist and skip check if none), then grep only for p2-specific
error patterns (use grep -iqE -- 'Cannot satisfy|conflicting
requirements|Conflicts detected|Resolution failed' "$LOGDIR"/*.log) so you avoid
benign matches; if grep returns success, print the failure message and exit 1.
Ensure you reference LOGDIR and replace the existing grep -iq "conflict"
"$LOGDIR"/*.log block with this guarded, pattern-specific check.
🧹 Nitpick comments (4)
releng/update-site-tests/test-update.sh (3)
8-18: Hardcoded URLs and versions limit the stated goal of testing against multiple IDE/Eclipse versions.The PR objective is to verify compatibility "against previous Espressif-IDE versions and recent Eclipse CDT releases," but the script pins a single Eclipse release (
2025-09) and a single stable plugin version (v4.0.0). Consider parameterizing these as script arguments or environment variables so the CI matrix (or future callers) can iterate over multiple version combinations.Example
-ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz" -STABLE_ZIP_URL="https://dl.espressif.com/dl/idf-eclipse-plugin/updates/com.espressif.idf.update-v4.0.0.zip" -RC_REPO="https://dl.espressif.com/dl/idf-eclipse-plugin/updates/latest/" +ECLIPSE_URL="${ECLIPSE_URL:?Please set ECLIPSE_URL}" +STABLE_ZIP_URL="${STABLE_ZIP_URL:?Please set STABLE_ZIP_URL}" +RC_REPO="${RC_REPO:?Please set RC_REPO}"
27-27: Dead assignment —ECLIPSE_HOMEis overwritten on line 36.Line 27 sets
ECLIPSE_HOME="$WORKDIR/eclipse", but line 36 immediately reassigns it viafind. Remove the line 27 assignment to avoid confusion.
33-36: No integrity verification of downloaded artifacts.
wgetdownloads over HTTPS but there's no checksum verification for the Eclipse tarball or the plugin zip. A corrupted or tampered download would produce confusing failures downstream. Consider adding SHA-256 checks..github/workflows/ci.yml (1)
49-55: Consider whether update site test failures should block the entire CI pipeline.The test downloads external resources (
ftp.osuosl.org,dl.espressif.com) that are outside your control. If either host is temporarily unavailable, the build will fail. Consider addingcontinue-on-error: trueif this test is intended as informational, or at minimum add retry logic in the shell script for thewgetcalls.
| echo "Checking logs for conflicts..." | ||
| if grep -iq "conflict" "$LOGDIR"/*.log; then | ||
| echo "❌ Conflict detected in installation logs!" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
grep "conflict" is overly broad and may produce false positives or false negatives.
The word "conflict" could appear in benign log messages (e.g., "No conflicts found", class names containing "conflict"). Consider grepping for p2-specific conflict markers or error patterns instead. Also, if no .log files exist the glob will fail under set -e.
Suggestion
-if grep -iq "conflict" "$LOGDIR"/*.log; then
+shopt -s nullglob
+if grep -iqP '(?<!no )conflicts?\b' "$LOGDIR"/*.log; then
echo "❌ Conflict detected in installation logs!"
exit 1
fiOr better, grep for the specific p2 error messages that indicate real conflicts.
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` around lines 111 - 115, Change the
broad grep that searches for "conflict" in $LOGDIR/*.log to first ensure there
are log files (e.g., test if any "$LOGDIR"/*.log exist and skip check if none),
then grep only for p2-specific error patterns (use grep -iqE -- 'Cannot
satisfy|conflicting requirements|Conflicts detected|Resolution failed'
"$LOGDIR"/*.log) so you avoid benign matches; if grep returns success, print the
failure message and exit 1. Ensure you reference LOGDIR and replace the existing
grep -iq "conflict" "$LOGDIR"/*.log block with this guarded, pattern-specific
check.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 117-124: The -listInstalledRoots invocation in test-update.sh is
currently restricted to -profile SDKProfile so it only reports the RC install;
update the script to list the default profile (where the stable plugin was
installed) as well — either run the org.eclipse.equinox.p2.director
-listInstalledRoots command once without -profile to capture the default profile
or run it twice (once with -profile SDKProfile and once without) and write both
outputs (e.g., to installed-roots-default.txt and installed-roots-sdk.txt) so
the report reflects both stable and RC plugins.
- Line 50: STABLE_REPO is hardcoded to
"file://$WORKDIR/stable-repo/artifacts/update" which breaks if the zip layout
changes; modify test-update.sh to discover the extracted subdirectory
dynamically by searching under "$WORKDIR/stable-repo" for a unique marker file
(e.g., content.jar or artifacts.jar) and set STABLE_REPO to the file:// URL of
the directory that contains that marker. In other words, replace the fixed
STABLE_REPO assignment with logic that uses WORKDIR and a find/lookup of
content.jar or artifacts.jar, assigns the containing directory to STABLE_REPO,
and emits a clear error if no marker is found.
- Around line 56-100: The stable and RC installs are using different p2 profiles
so the RC run is not upgrading the stable install; make both invocations target
the same p2 profile. Update the stable-install block (the
"$ECLIPSE_HOME/eclipse" call that uses "$STABLE_REPO") to include the same flags
used in the RC block: -profile SDKProfile -destination "$ECLIPSE_HOME"
-bundlepool "$WORKDIR/p2" (so FEATURE_ID is installed into the same
profile/bundlepool), or alternatively remove -profile
SDKProfile/-bundlepool/-destination from the RC block if you prefer default
profile behavior — but ensure both invocations use identical
profile/destination/bundlepool settings and remove any stray/duplicate mentions
of -profile SDKProfile elsewhere so the test actually performs an in-place
upgrade.
- Around line 36-37: The current assignment to ECLIPSE_HOME via the find command
can yield an empty string; guard against that by checking ECLIPSE_HOME after the
find (the variable set by ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d
-name "eclipse*" | head -n1")) and if empty, print a clear error message
(including WORKDIR context) and exit non‑zero; otherwise continue and echo the
path as now. Ensure subsequent references use the validated $ECLIPSE_HOME.
🧹 Nitpick comments (2)
releng/update-site-tests/test-update.sh (2)
8-15: Hardcoded URLs will silently break when versions rotate.
ECLIPSE_URLandSTABLE_ZIP_URLpoint to specific release versions. When these URLs are retired or updated, the script fails with no clear indication of why. Consider parameterizing these (via environment variables with defaults) so CI can override them, and add a check after eachwgetthat verifies the downloaded file is valid (e.g., non-zero size or correct content type).Proposed approach
-ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz" -STABLE_ZIP_URL="https://dl.espressif.com/dl/idf-eclipse-plugin/updates/com.espressif.idf.update-v4.0.0.zip" +ECLIPSE_URL="${ECLIPSE_URL:-https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz}" +STABLE_ZIP_URL="${STABLE_ZIP_URL:-https://dl.espressif.com/dl/idf-eclipse-plugin/updates/com.espressif.idf.update-v4.0.0.zip}"
149-163: Report always says "Conflict check: OK" — consider making it dynamic.Line 160 unconditionally writes
"Conflict check: OK". While this is technically correct today (the script exits on conflict), it's brittle if the flow is ever changed to be non-fatal. A small improvement:- echo "Conflict check: OK" + echo "Conflict check: ${CONFLICT_STATUS:-OK}"This is a minor maintainability nit.
| mkdir -p "$WORKDIR/stable-repo" | ||
| unzip -q "$WORKDIR/stable.zip" -d "$WORKDIR/stable-repo" | ||
|
|
||
| STABLE_REPO="file://$WORKDIR/stable-repo/artifacts/update" |
There was a problem hiding this comment.
Hardcoded zip internal path artifacts/update is fragile.
STABLE_REPO assumes the zip extracts into artifacts/update/. If the zip structure changes across plugin versions, this silently breaks. Consider discovering the path dynamically (e.g., by locating content.jar or artifacts.jar inside the extracted directory).
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` at line 50, STABLE_REPO is hardcoded
to "file://$WORKDIR/stable-repo/artifacts/update" which breaks if the zip layout
changes; modify test-update.sh to discover the extracted subdirectory
dynamically by searching under "$WORKDIR/stable-repo" for a unique marker file
(e.g., content.jar or artifacts.jar) and set STABLE_REPO to the file:// URL of
the directory that contains that marker. In other words, replace the fixed
STABLE_REPO assignment with logic that uses WORKDIR and a find/lookup of
content.jar or artifacts.jar, assigns the containing directory to STABLE_REPO,
and emits a clear error if no marker is found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 137-150: The unconditional "✅ Eclipse launched successfully"
should be gated by an actual check: after launching "$ECLIPSE_HOME/eclipse" to
background and capturing PID, wait and then inspect the process exit status
and/or the "$LOGDIR/eclipse-launch.log" contents for a known startup marker
before printing success; treat kill returning 137/143 as expected and consider
them not-a-success for immediate crash, e.g., check `kill -0 $PID` or `ps` to
see if the PID is still running and/or grep the log for a startup line, and only
echo the success message when those checks confirm Eclipse started (otherwise
print an error and exit non‑zero).
🧹 Nitpick comments (2)
releng/update-site-tests/test-update.sh (2)
28-28: Dead assignment —ECLIPSE_HOMEis overwritten on line 37.This initial assignment is never used because
findon line 37 replaces it unconditionally. Remove it to avoid confusion about which value is authoritative.-ECLIPSE_HOME="$WORKDIR/eclipse"
53-58: Capture the pre-install roots to a file for the report.The output of
listInstalledRootshere goes to stdout only (lost once CI scrollback is trimmed). Redirect to a log file so it can be compared with the post-install listing in step 6.echo "Listing installed roots before stable:" "$ECLIPSE_HOME/eclipse" \ -nosplash \ -application org.eclipse.equinox.p2.director \ -listInstalledRoots \ - -consoleLog + -consoleLog \ + | tee "$LOGDIR/installed-roots-before.txt"
| echo "Launching Eclipse once..." | ||
| "$ECLIPSE_HOME/eclipse" \ | ||
| -nosplash \ | ||
| -clean \ | ||
| -data "$WORKDIR/workspace" \ | ||
| -configuration "$WORKDIR/configuration" \ | ||
| -consoleLog \ | ||
| -vmargs -Djava.awt.headless=false \ | ||
| > "$LOGDIR/eclipse-launch.log" 2>&1 & | ||
|
|
||
| PID=$! | ||
| sleep 20 | ||
| kill $PID || true | ||
| echo "✅ Eclipse launched successfully" |
There was a problem hiding this comment.
Headless launch issues already flagged; additionally, the success message on line 150 is unconditional.
The prior review covered headless=false and the fragile sleep/kill pattern. On top of that: "✅ Eclipse launched successfully" is printed regardless of whether Eclipse actually started or crashed instantly. Check the process exit status (or at least verify the log isn't empty / contains a startup marker) before declaring success.
PID=$!
sleep 20
kill $PID || true
-echo "✅ Eclipse launched successfully"
+wait $PID 2>/dev/null
+LAUNCH_EXIT=$?
+if [ $LAUNCH_EXIT -ne 0 ] && [ $LAUNCH_EXIT -ne 137 ] && [ $LAUNCH_EXIT -ne 143 ]; then
+ echo "❌ Eclipse launch failed with exit code $LAUNCH_EXIT"
+ exit 1
+fi
+echo "✅ Eclipse launched successfully"Exit codes 137 (SIGKILL) and 143 (SIGTERM) are expected from the kill.
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` around lines 137 - 150, The
unconditional "✅ Eclipse launched successfully" should be gated by an actual
check: after launching "$ECLIPSE_HOME/eclipse" to background and capturing PID,
wait and then inspect the process exit status and/or the
"$LOGDIR/eclipse-launch.log" contents for a known startup marker before printing
success; treat kill returning 137/143 as expected and consider them
not-a-success for immediate crash, e.g., check `kill -0 $PID` or `ps` to see if
the PID is still running and/or grep the log for a startup line, and only echo
the success message when those checks confirm Eclipse started (otherwise print
an error and exit non‑zero).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 5-8: The cleanup block uses $WORKDIR and $LOGDIR before their
defaults are set, causing rm/mkdir to run with empty paths; move the default
assignments (the parameter expansions ${WORKDIR:-...} and ${LOGDIR:-...}) so
they occur before the cleanup block or alternatively move the entire cleanup
block after the configuration section where WORKDIR and LOGDIR are set; ensure
only one mkdir -p is kept (remove the redundant second mkdir -p) and update
references to WORKDIR and LOGDIR accordingly.
🧹 Nitpick comments (4)
releng/update-site-tests/test-update.sh (4)
162-179: Report generation looks correct but always says "Conflict check: OK".Because the script exits on line 121 if a conflict is detected, line 172 will only ever print "OK". This is technically accurate but means the report is never generated in the failure case. Consider trapping on exit or deferring the conflict-check exit so the report is always produced (it could be useful for debugging failed runs, especially since CI uploads it as an artifact).
Sketch: generate report even on failure using a trap
# Near the top of the script, after REPORT is defined: generate_report() { echo "Generating report..." { echo "Espressif IDE Upgrade Test Report" echo "================================" echo "" echo "Stable install exit code: ${STABLE_EXIT:-N/A}" echo "RC install exit code: ${RC_EXIT:-N/A}" echo "" echo "Installed Roots:" cat "$LOGDIR/installed-roots.txt" 2>/dev/null || echo "(not available)" echo "" echo "Conflict check: ${CONFLICT_STATUS:-UNKNOWN}" echo "" echo "Logs directory: $LOGDIR" } > "$REPORT" } trap generate_report EXITThen set
CONFLICT_STATUS="OK"orCONFLICT_STATUS="FAILED"accordingly before exiting.
14-15: Hardcoded Eclipse version2025-09will become stale.The PR objective is to test compatibility with "recent Eclipse CDT releases." Consider parameterizing
ECLIPSE_URL(or at least the release identifier) so the CI workflow can matrix-test against multiple Eclipse versions without editing the script.Sketch
-ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz" +ECLIPSE_VERSION="${ECLIPSE_VERSION:-2025-09}" +ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/${ECLIPSE_VERSION}/R/eclipse-cpp-${ECLIPSE_VERSION}-R-linux-gtk-x86_64.tar.gz"
17-18: Hardcoded stable plugin versionv4.0.0will become stale.Same concern — consider parameterizing
STABLE_ZIP_URLor at least the version component so it can be overridden from CI or updated easily.
58-63: Pre-installlistInstalledRootsoutput is not captured.The output of this invocation goes to stdout (interleaved with
set -xtrace output) and is not written to a file. If the intent is to compare before/after, capture it to a log file for the report.Proposed fix
echo "Listing installed roots before stable:" "$ECLIPSE_HOME/eclipse" \ -nosplash \ -application org.eclipse.equinox.p2.director \ -listInstalledRoots \ - -consoleLog + -consoleLog \ + > "$LOGDIR/installed-roots-before.txt" +cat "$LOGDIR/installed-roots-before.txt"
|
@kolipakakondal , @sigmaaa hi ! We must have 2 tests. Test 1: installs the release candidate on the latest stable Eclipse release. Test 2: takes Eclipse-1, installs the latest stable IDE release on it, and then updates it to the release candidate. At this moment I have 1 hybrid test to test both cases. Test logic:
Since this is an RC test case, what is the best approach to trigger this test? I don’t see that we use
But this approach works only if we follow the pattern of 1 IDE release per 1 Eclipse release. However, to make this script fully independent of manual intervention, we should consider adding an “Eclipse version support” tag to our
This mismatch can cause incorrect or inconsistent test behavior. |
|
Hi @AndriiFilippov, Thanks for the PR.
Ideally, we should provide dependencies that allow using 4.3.0 on Eclipse 25-12 even if it is written for 26-03. This is important because we need to support updating Espressif-IDE, which does not have an Eclipse platform update site. We also agreed not to include such a site, since users could accidentally break the IDE by installing incompatible dependencies. If a problem occurs in this scenario, it will indicate that we need to add additional dependencies, and the test will help us understand that something is missing. |
|
Hi @AndriiFilippov @sigmaaa I have created a detailed specification and goals we need to achieve with this task in the Athena project, please have a look. Adding here for your reference as well. Update Site Test — Specification1. Goals
2. Test Cases
3. What We Achieve With This
Overall: automated coverage of stable + dev update paths on current and next Eclipse, plus Espressif-IDE (latest stable → nightly update site), without manual testing. 4. VariablesAll URLs and version-like values used by the update-site tests are kept in one file: Purpose: A single place to update whenever an external dependency changes, so we don't have to hunt through scripts or workflows. What you need to do:
Scripts and workflows that run the update-site tests should source this file so they use the same values. |
|
@AndriiFilippov As I could PR address some of these changes already (case 2 and case 3), please take a look at the spec and goals again and make the PR changes accordingly. |
5820fec to
30ee99c
Compare
|
@kolipakakondal , @sigmaaa hi ! |
|
Hi @AndriiFilippov Can you update the PR description and publish test results what all cases addressed with this PR? |
sigmaaa
left a comment
There was a problem hiding this comment.
Hi @AndriiFilippov,
Once again, thanks for the PR. LGTM! Before merging, do not forget to revert ci.yml and ci_windows.yml to run swtbot tests
5f47040 to
25be21d
Compare
- move all URLs into variables.sh - run workflow daily at 00:00 UTC - add workflow summary
50c8892 to
70b9c77
Compare
Description
This PR introduces automated update-path testing for Espressif-IDE and Eclipse-based setups using the p2 director on Linux runner.
It validates installation and update flows across multiple scenarios, including stable and nightly update paths.
Fixes # (IEP-1685)
│ └── update-site-tests/
│ │
│ ├── variables.sh
│ │ # root config:
│ │ # - URLs (Eclipse, Espressif)
│ │ # - update sites
│ │
│ ├── detect-latest-eclipse-steams.sh
│ │ # - parses release.xml
│ │ # - detects latest R / M / RC
│ │ # - forms URLs
│ │ # - exports variables
│ │
│ ├── run-all-cases.sh
│ │ # orchestrator
│ │ # - runs Case 1–4
│ │ # - prepares env
│ │ # - download Espressif-IDE
│ │
│ ├── run-single-case.sh
│ │ # case executor
│ │ # - install Eclipse / IDE
│ │ # - executes installIU / update
│ │ # - writes run.log
│ │
│ ├── out/
│ │ └── case*/
│ │ └── logs/
│ │ ├── run.log
│ │ ├── installed-ius.txt
│ │ └── installed-roots.txt
Scheduled daily execution at 00:00 UTC.
Case 1: Eclipse (latest R) → install stable plugin⚠️ optional test case (based on Milestone / Release Candidate availability) ⚠️
Case 2: Eclipse (latest R) → install stable → update to nightly
Case 3: Eclipse (latest pre-release: M/RC) → install stable → update to nightly.
Case 4: Espressif-IDE (bundled) → update to nightly
Results:
Each test case produces:
run.log – full execution log
installed-ius.txt
installed-roots.txt
https://github.com/espressif/idf-eclipse-plugin/actions/runs/23256402453?pr=1397
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Tests
Chores