chore: improve logging in RPC compare tests #6210

LesnyRumcajs · 2025-10-30T13:53:57Z

Summary of changes

Changes introduced in this pull request:

the current output, e.g., here isn't really helpful in identifying potential issues - it just says that a certain method fails. Let's print the report
also, I think there is a bug in shutdown after the api compare is done; it won't be executed due to bash's set -e in case of a failure of the compare. This would make Forest run in background during the offline check, increasing memory usage of the entire setup.

Reference issue to close (if applicable)

Closes

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

Chores
- Reduced test-run verbosity and improved CI scripts to generate and expose API comparison reports (including separate offline reports) for easier diagnosis.
Tests
- Test runs now produce failure-only API comparison reports, print report output when available, consistently propagate exit statuses, and always print a final summary before saving and asserting failures.

coderabbitai · 2025-10-30T13:54:17Z

Walkthrough

Updated Docker Compose entrypoint scripts for api-compare and api-compare-offline to change shell options, enable failure-only reporting with explicit report directories, capture and print JSON reports, and propagate the tool exit status. Adjusted Rust test finalization to print the summary unconditionally before optionally saving a report and to check failures after any save.

Changes

Cohort / File(s)	Summary
API Comparison Services Report Handling `scripts/tests/api_compare/docker-compose.yml`	Modified service entrypoint shells: replaced `set -euxo pipefail` with `set -uxo pipefail`; added `--report-mode=failure-only` and `--report-dir` flags (`/data/api-compare-report`, `/data/api-compare-report-offline`); capture tool exit status, print a labeled header, attempt to `cat` the generated `full_report.json` (with fallback message), and `exit` with the recorded status.
Test Finalization Flow `src/tool/subcommands/api_cmd/api_compare_tests.rs`	Adjusted `run_tests` finalization: unconditionally call `print_summary()` before handling `report_dir` saving (removed alternate branch), then perform the `has_failures` check after any possible save and preserve final assertion/exit behavior.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Entrypoint as Docker service entrypoint
    participant Wrapper as shell wrapper script
    participant Tool as api-compare
    participant ReportFS as /data/... (mounted)
    rect rgb(245,250,240)
      Entrypoint->>Wrapper: start
      Wrapper->>Wrapper: set -uxo pipefail
      Wrapper->>Tool: run with --report-mode=failure-only --report-dir=/data/...
      Tool-->>ReportFS: write full_report.json (if failures)
      Tool-->>Wrapper: exit (status)
      Wrapper->>Wrapper: capture status ($?)
      Wrapper->>Entrypoint: print "==== API Compare Report ===="
      Wrapper->>ReportFS: cat full_report.json (or print fallback)
      Wrapper-->>Entrypoint: exit with recorded status
    end

sequenceDiagram
    autonumber
    participant Runner as test harness
    participant run_tests as function
    rect rgb(250,245,255)
      Runner->>run_tests: invoke
      run_tests->>run_tests: execute tests, collect results
      run_tests->>run_tests: print_summary() (now unconditional)
      alt report_dir provided
        run_tests->>Filesystem: save full report to report_dir
      end
      run_tests->>run_tests: evaluate has_failures and return/exit accordingly
      run_tests-->>Runner: return status
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Verify removal of -e from set doesn't change desired immediate-exit semantics given explicit status capture and final exit.
Confirm report directories (/data/api-compare-report*) are mounted/writable in the Docker Compose configuration.
Ensure cat fallback messaging and printing do not mask or alter the original exit status.
Review run_tests ordering to ensure saving the report cannot affect failure detection or exit semantics.

Suggested reviewers

hanabi1224
elmattic
akaladarshi

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'chore: improve logging in RPC compare tests' is clearly related to the main changes in the changeset. The modifications focus on improving logging and output reporting in the RPC/API compare test infrastructure, which aligns well with the title. While the title doesn't capture every detail (like the bash script improvements or shutdown fixes), it appropriately summarizes the primary change from a logging and reporting perspective.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch improve-logging-rpc-tests

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 688cf3e and a8e0dbc.

📒 Files selected for processing (1)

src/tool/subcommands/api_cmd/api_compare_tests.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/tool/subcommands/api_cmd/api_compare_tests.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: Build Ubuntu
GitHub Check: cargo-publish-dry-run
GitHub Check: Build MacOS
GitHub Check: tests-release
GitHub Check: tests
GitHub Check: Build forest binaries on Linux AMD64
GitHub Check: All lint checks

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

scripts/tests/api_compare/docker-compose.yml (1)
242-243: Consider defensive handling for report file access (optional).

The code assumes /data/api-compare-report/full_report.json exists after forest-tool api compare completes. If the file doesn't exist or becomes unreadable, cat will error to stderr but the script continues (no -e flag), potentially masking issues. Since this is test infrastructure, a minor enhancement for robustness:
-        status=$$?
-        echo "==== API Compare Report ===="
-        cat /data/api-compare-report/full_report.json
+        status=$$?
+        echo "==== API Compare Report ===="
+        cat /data/api-compare-report/full_report.json || echo "(Report file not accessible)"
This ensures visible feedback if the report is unavailable, while still preserving the original exit code.

Also applies to: 245-249

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91f4ed1 and c67c576.

📒 Files selected for processing (1)

scripts/tests/api_compare/docker-compose.yml (3 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: tests
GitHub Check: tests-release
GitHub Check: Build MacOS
GitHub Check: Build Ubuntu
GitHub Check: cargo-publish-dry-run
GitHub Check: Build forest binaries on Linux AMD64
GitHub Check: All lint checks

🔇 Additional comments (2)

scripts/tests/api_compare/docker-compose.yml (2)

231-249: Shutdown bug fix and enhanced report logging look solid.

Removing the -e flag and explicitly capturing the exit status ensures the shutdown step runs even when forest-tool api compare fails. This addresses the stated issue of Forest lingering in the background during offline checks. The report logging before shutdown is a nice addition for debugging.

The strategy of capturing the original status and propagating it at the end is the right approach here.

275-289: Offline service follows the same solid pattern.

The api-compare-offline service mirrors the api-compare changes correctly, maintaining consistency across both test variants. The same considerations and optional improvements from the main service apply here.

LesnyRumcajs requested a review from a team as a code owner October 30, 2025 13:53

LesnyRumcajs requested review from elmattic and sudo-shashank and removed request for a team October 30, 2025 13:53

chore: improve logging in RPC compare tests

c67c576

LesnyRumcajs force-pushed the improve-logging-rpc-tests branch from 9e070be to c67c576 Compare October 30, 2025 13:54

coderabbitai bot reviewed Oct 30, 2025

View reviewed changes

improve report error logging

d6578f5

elmattic previously approved these changes Oct 30, 2025

View reviewed changes

sudo-shashank previously approved these changes Oct 30, 2025

View reviewed changes

LesnyRumcajs added the RPC requires calibnet RPC checks to run on CI label Oct 30, 2025

hanabi1224 previously approved these changes Oct 30, 2025

View reviewed changes

LesnyRumcajs added 2 commits October 30, 2025 16:17

always stdout summary

b5b66b5

fix report name on failures

3c137ed

LesnyRumcajs dismissed stale reviews from hanabi1224, sudo-shashank, and elmattic via 3c137ed October 30, 2025 15:17

LesnyRumcajs and others added 4 commits October 30, 2025 16:26

fix build

688cf3e

Merge branch 'main' into improve-logging-rpc-tests

60133e0

Merge branch 'main' into improve-logging-rpc-tests

a8e0dbc

Merge branch 'main' into improve-logging-rpc-tests

4458ff4

hanabi1224 approved these changes Nov 3, 2025

View reviewed changes

LesnyRumcajs added this pull request to the merge queue Nov 3, 2025

Merged via the queue into main with commit bcf80b3 Nov 3, 2025
40 checks passed

LesnyRumcajs deleted the improve-logging-rpc-tests branch November 3, 2025 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: improve logging in RPC compare tests #6210

chore: improve logging in RPC compare tests #6210

Uh oh!

LesnyRumcajs commented Oct 30, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 30, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chore: improve logging in RPC compare tests #6210

chore: improve logging in RPC compare tests #6210

Uh oh!

Conversation

LesnyRumcajs commented Oct 30, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LesnyRumcajs commented Oct 30, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 30, 2025 •

edited

Loading