feat: add heph-based integration tests to CI#86
Conversation
Add a new CI job that builds heph from source and runs SDK integration tests against it. This replaces the previously-ignored storage tests that required a remote CCN with self-contained tests that upload data first, then verify downloads, file sizes, and hash verification. Changes: - Add integration-test job to CI workflow (builds and runs heph locally) - Add heph_integration.rs with 6 tests covering upload, download, size, and verified download flows - Fix AlephClient uploads: use a plain reqwest client for multipart requests (retry middleware can't clone streaming bodies) - Fix heph HEAD /api/v0/storage/raw: let actix derive Content-Length from the response body instead of manually setting it (actix was overriding to 0) - Add GET /api/v0/version endpoint to heph for health checks - Remove redundant ignored tests now covered by integration tests; keep IPFS/websocket/corechannel tests that heph doesn't support yet
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR adds valuable heph-based integration tests to CI, replacing previously-ignored remote CCN tests with self-contained tests. The core changes are well-implemented: (1) The upload_client fix correctly addresses the retry middleware incompatibility with multipart bodies, (2) the HEAD request fix properly delegates Content-Length handling to actix, (3) the new version endpoint enables reliable health checks, and (4) the 6 integration tests cover upload, download, size verification, and hash verification flows. The CI workflow is well-structured with proper caching and health check logic.
crates/heph/src/api/storage.rs (line 2): The HttpRequest import is no longer used after removing the manual HEAD request handling. Consider removing it to avoid a dead code warning.
.github/workflows/ci.yml (line 96): Consider capturing heph's stdout/stderr (e.g., ./target/release/heph > /tmp/heph.log 2>&1 &) to aid debugging if the startup fails.
crates/aleph-sdk/src/client.rs (line 1089): The upload_client uses default reqwest settings. If the main client ever gets custom timeout configuration, consider whether uploads should inherit those settings.
Redirect heph stdout/stderr to /tmp/heph.log and dump it on startup failure or test failure.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR adds valuable integration tests for heph-based storage functionality, replacing previously-ignored tests that required a remote CCN. The changes are well-structured: a new CI job builds and runs heph locally, the SDK's AlephClient correctly uses a plain reqwest client for multipart uploads (since retry middleware can't handle non-cloneable streaming bodies), and heph fixes a bug where HEAD requests had incorrect Content-Length headers. The new version endpoint enables proper health checks. All code is clear, well-commented, and follows existing patterns.
crates/heph/src/api/mod.rs (line 16): The get_version() function is marked async but doesn't perform any async operations. Consider making it synchronous for clarity.
crates/aleph-sdk/tests/heph_integration.rs (line 95): Temp file names like aleph-heph-test-download could collide if tests run in parallel. Consider including a unique identifier (e.g., test name or timestamp) in the filename, though this is unlikely to be an issue since tests run sequentially by default.
crates/aleph-sdk/src/client.rs (line 1089): The upload_client uses default reqwest settings. If the http_client has custom timeouts or other configurations, uploads won't benefit from them. This is likely intentional (uploads shouldn't retry), but worth documenting if timeouts differ significantly.
Add a new CI job that builds heph from source and runs SDK integration tests against it. This replaces the previously-ignored storage tests that required a remote CCN with self-contained tests that upload data first, then verify downloads, file sizes, and hash verification.
Changes: