Skip to content

test: add CI cache for rpc snapshot tests to mitigate flackyness issue #5978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LesnyRumcajs merged 16 commits into from
Aug 26, 2025
Merged

test: add CI cache for rpc snapshot tests to mitigate flackyness issue #5978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1b343e8
test: run rpc snapshots with `cargo test` instead of `nextest`
hanabi1224 Aug 25, 2025
a52c947
update test prefix to avoid false positive in filter
hanabi1224 Aug 25, 2025
c48a67d
apply AI suggestion
hanabi1224 Aug 25, 2025
671f5c2
CI cache for rpc snapshots
hanabi1224 Aug 25, 2025
3712b63
Merge branch 'main' into hm/run-rpc-snapshot-tests-with-cargo-test
hanabi1224 Aug 25, 2025
0e902af
switch back to nextest
hanabi1224 Aug 25, 2025
8464707
Merge remote-tracking branch 'origin/main' into hm/run-rpc-snapshot-t…
hanabi1224 Aug 25, 2025
d1c270f
fix nextest config
hanabi1224 Aug 25, 2025
d1bf0fc
fix cache path
hanabi1224 Aug 25, 2025
cf76ac2
yml fmt
hanabi1224 Aug 25, 2025
6cc77af
Merge branch 'main' into hm/run-rpc-snapshot-tests-with-cargo-test
hanabi1224 Aug 25, 2025
0f351a1
fix cache key
hanabi1224 Aug 25, 2025
d472ae8
enable for draft for testing
hanabi1224 Aug 25, 2025
38bd144
revert
hanabi1224 Aug 25, 2025
70dfb99
Merge branch 'main' into hm/run-rpc-snapshot-tests-with-cargo-test
hanabi1224 Aug 25, 2025
fa84780
apply AI comments
hanabi1224 Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .config/nextest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ slow-timeout = { period = "120s", terminate-after = 3 }
# There might be some network issues, so we allow some retries with backoff.
# Jitter is enabled to avoid [thundering herd issues](https://en.wikipedia.org/wiki/Thundering_herd_problem).
[[profile.default.overrides]]
filter = 'test(rpc_test_)'
filter = 'test(rpc_snapshot_test_)'
slow-timeout = { period = "120s", terminate-after = 3 }
retries = { backoff = "exponential", count = 4, delay = "5s", jitter = true }
retries = { backoff = "exponential", count = 3, delay = "5s", jitter = true }
18 changes: 18 additions & 0 deletions .github/workflows/unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ jobs:
continue-on-error: true
- name: Checkout Sources
uses: actions/checkout@v4
- uses: actions/cache/restore@v4
with:
path: |
/home/runner/.cache/forest/test/rpc-snapshots/rpc_test
# Broad key + prefix-based restore so the latest rpcsnap-<hash> is picked up.
key: rpcsnap-
enableCrossOsArchive: true
- name: Setup sccache
uses: mozilla-actions/sccache-action@v0.0.9
timeout-minutes: ${{ fromJSON(env.CACHE_TIMEOUT_MINUTES) }}
Expand All @@ -77,3 +84,14 @@ jobs:
env:
# To minimize compile times: https://nnethercote.github.io/perf-book/build-configuration.html#minimizing-compile-times
RUSTFLAGS: "-C linker=clang -C link-arg=-fuse-ld=lld"
- id: get-cache-hash
run: |
ls -lhR ~/.cache/forest/test/rpc-snapshots/rpc_test/*
echo "hash=$(openssl md5 ~/.cache/forest/test/rpc-snapshots/rpc_test/* | sort | openssl md5 | cut -d ' ' -f 2)" >> $GITHUB_OUTPUT
shell: bash
- uses: actions/cache/save@v4
with:
path: |
/home/runner/.cache/forest/test/rpc-snapshots/rpc_test
key: rpcsnap-${{ steps.get-cache-hash.outputs.hash }}
enableCrossOsArchive: true
Comment on lines +87 to +97

@coderabbitai coderabbitai Bot Aug 25, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Make hashing/saving robust; avoid step failures on empty/missing files and save even when tests fail.

  • ls -lhR ~/.cache/.../rpc_test/* will exit non‑zero if the directory is empty or missing, failing the job.
  • openssl md5 is brittle; prefer sha256sum which is present on runners.
  • cache/save won’t run if make test-release fails; you likely want to persist downloaded snapshots to help subsequent retries.

Apply this diff to harden the steps:

-      - id: get-cache-hash
-        run: |
-          ls -lhR ~/.cache/forest/test/rpc-snapshots/rpc_test/*
-          echo "hash=$(openssl md5 ~/.cache/forest/test/rpc-snapshots/rpc_test/* | sort | openssl md5 | cut -d ' ' -f 2)" >> $GITHUB_OUTPUT
-        shell: bash
-      - uses: actions/cache/save@v4
-        with:
-          path: |
-            /home/runner/.cache/forest/test/rpc-snapshots/rpc_test
-          key: rpcsnap-${{ steps.get-cache-hash.outputs.hash }}
-          enableCrossOsArchive: true
+      - id: get-cache-hash
+        if: always()
+        shell: bash
+        run: |
+          set -euo pipefail
+          dir="/home/runner/.cache/forest/test/rpc-snapshots/rpc_test"
+          # Don't fail if directory is empty/missing.
+          if [[ ! -d "$dir" ]]; then
+            echo "hash=" >> "$GITHUB_OUTPUT"
+            exit 0
+          fi
+          shopt -s nullglob
+          files=("$dir"/*)
+          if [[ ${#files[@]} -eq 0 ]]; then
+            echo "hash=" >> "$GITHUB_OUTPUT"
+            exit 0
+          fi
+          # Optional: list for diagnostics, but never fail.
+          ls -lhR "$dir" || true
+          # Stable content hash across OS/arch.
+          printf '%s\0' "${files[@]}" \
+            | xargs -0 sha256sum \
+            | sort -k1,1 \
+            | sha256sum \
+            | awk '{print "hash="$1}' >> "$GITHUB_OUTPUT"
+
+      - uses: actions/cache/save@v4
+        if: ${{ always() && steps.get-cache-hash.outputs.hash != '' }}
+        with:
+          path: |
+            /home/runner/.cache/forest/test/rpc-snapshots/rpc_test
+          key: rpcsnap-${{ steps.get-cache-hash.outputs.hash }}
+          enableCrossOsArchive: true

Optional guard (if you see flaky/corrupted snapshots): add a quick JSON validation before saving to avoid caching error pages/rate-limit bodies.

      - name: Validate snapshots are JSON
        if: ${{ always() }}
        shell: bash
        run: |
          set -euo pipefail
          dir="/home/runner/.cache/forest/test/rpc-snapshots/rpc_test"
          if [[ -d "$dir" ]]; then
            find "$dir" -type f -maxdepth 1 -print0 | xargs -0 -n1 sh -c 'jq -e . "$0" >/dev/null 2>&1 || echo "Non-JSON snapshot: $0"' || true
          fi

@hanabi1224 hanabi1224 Aug 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. The test will re-download proper snapshot if checksum does not match the http head header

@coderabbitai coderabbitai Bot Aug 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hanabi1224, understood! Thanks for the clarification about the checksum validation against HTTP HEAD headers - that's a solid fallback mechanism that handles the corruption scenarios I was concerned about. Your current implementation makes sense given that validation layer.

✏️ Learnings added 
Learnt from: hanabi1224
PR: ChainSafe/forest#5978
File: .github/workflows/unit-tests.yml:86-96
Timestamp: 2025-08-25T14:11:10.764Z
Learning: hanabi1224's RPC snapshot tests include a checksum validation mechanism that compares against HTTP HEAD headers, automatically re-downloading snapshots if the cached version doesn't match, providing protection against corrupted cached files.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: hanabi1224
PR: ChainSafe/forest#5978
File: .github/workflows/unit-tests.yml:72-76
Timestamp: 2025-08-25T09:53:37.424Z
Learning: hanabi1224 intentionally designs RPC snapshot test cache to be OS/arch-agnostic, sharing cached snapshot files across different platforms using enableCrossOsArchive: true and hash-based cache keys to improve cache efficiency and reduce redundant downloads.

Learnt from: hanabi1224
PR: ChainSafe/forest#5978
File: .github/workflows/unit-tests.yml:72-76
Timestamp: 2025-08-25T09:57:27.387Z
Learning: hanabi1224's strategy for RPC snapshot cache versioning: branch isolation is not needed currently, but if snapshot generation logic changes in the future, a version string can be added to the cache key to handle the evolution.

Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

2 changes: 1 addition & 1 deletion build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ fn rpc_regression_tests_gen() {
w,
r#"
#[tokio::test(flavor = "multi_thread")]
async fn rpc_test_{ident}() {{
async fn rpc_snapshot_test_{ident}() {{
rpc_regression_test_run("{test}").await
}}
"#,
Expand Down
Loading