feat: snapshot tests during CI #137
Conversation
WalkthroughThis pull request introduces a new job to the CI workflow and a comprehensive Makefile. The CI configuration now includes a Changes
Sequence Diagram(s)sequenceDiagram
participant PR as "Pull Request"
participant CI as "CI Runner"
participant Checkout as "actions/checkout"
participant Cache as "actions/cache & rust-cache"
participant Bootstrap as "Bootstrap Step"
participant Test as "Cargo Test (snapshot tests)"
PR->>CI: Trigger CI Job (if not draft)
CI->>Checkout: Checkout code
CI->>Cache: Execute caching steps for ledger.db & chain.db
Cache-->>CI: Cache hit/miss signal
alt Cache Miss
CI->>Bootstrap: Run amaru bootstrap step
end
CI->>Test: Run node process and execute cargo test
sequenceDiagram
participant User as "User (Make Command)"
participant Enforce as "enforce-peer-address"
participant Download as "download-snapshots"
participant ImportSnap as "import-snapshots"
participant ImportHead as "import-headers"
participant ImportNonce as "import-nonces"
participant Node as "Node Process"
User->>Enforce: Run peer address enforcement
Enforce-->>User: Validate AMARU_PEER_ADDRESS
User->>Download: Execute download-snapshots target
Download-->>User: Create snapshot directory and fetch data
User->>ImportSnap: Trigger import-snapshots
User->>ImportHead: Trigger import-headers
User->>ImportNonce: Trigger import-nonces
User->>Node: Run node with snapshots, headers, and nonces imported
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.github/workflows/CI.yml (3)
108-116: Caching Ledger DB: Update the Action VersionThe caching step for
ledger.dbusesactions/cache@v3, which static analysis flags as a tad outdated. Consider updating it (if a newer version is available) to benefit from bug fixes and improvements.🧰 Tools
🪛 actionlint (1.7.4)
110-110: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
117-122: Caching Chain DB: A Similar Upgrade is WarrantedAs with the ledger cache, the chain cache also employs
actions/cache@v3. It might be a good idea to update this action too, ensuring the CI workflow remains robust and up-to-date.🧰 Tools
🪛 actionlint (1.7.4)
117-117: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
128-139: Run Node Block: Piped Process ID CautionThe approach of running
makeand piping its output into a whilst loop to monitor for a shutdown signal is inventive. However, using$!inside a piped loop can sometimes be a bit dodgy due to subshell behaviour. It might be worth verifying that the correct process ID is being captured so that the stop signal is effectively delivered.Makefile (5)
4-7: Help Target: Friendly Guidance at Your FingertipsThe
helptarget is a delightful addition, offering users a quick reference for available commands. The awk magic is clever, though a brief comment explaining its mechanics might save future maintainers from scratching their heads.
8-17: Download Snapshots: Consider Robust Error HandlingThis target efficiently creates a snapshots directory and fetches snapshot data. A small suggestion: adding error checking (perhaps via curl’s
-foption or verifying exit statuses) could make the process even more resilient if the network gets a bit cheeky.
18-23: Import Snapshots: Hard-Coded Paths—Might Need a SofteningThe logic here is sound, but the snapshot file paths are hard-coded. If these filenames are subject to change, consider parameterising them. That way, future tweaks won’t turn into a right faff.
24-33: Import Headers: Duplicate Invocations Require a NoteTwo separate invocations for importing headers with different starting points are intriguing. If both are necessary, a brief comment explaining the rationale would be grand, helping others understand the need for the dual calls.
34-41: Import Nonces: Keep an Eye on Hard-Coded ParametersThe nonces import command is precise but heavily hard-coded. Should those values ever need tweaking, adding a comment or refactoring to make them configurable could save a lot of future bother.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/CI.yml(1 hunks)Makefile(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/CI.yml
110-110: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
117-117: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Sanity
- GitHub Check: Snapshots
🔇 Additional comments (6)
.github/workflows/CI.yml (3)
99-105: Snapshots Job Addition: Neat and Conditional!The new
snapshotsjob is a smart addition—it ensures that snapshot tests run only when the pull request isn’t still in draft mode. Jolly good move to prevent any premature resource usage.
123-127: Bootstrap Amaru Step: Clever Conditional ExecutionThe conditional execution of
make bootstrapwhen the cache isn’t hit is a smart strategy. Just have a quick gander to ensure thebootstraptarget in your Makefile gracefully handles any edge cases.
140-141: Run Tests Step: Straightforward and Spot OnThe step to execute tests is succinct and clear. No worries here—everything appears to be in order.
Makefile (3)
42-42: Bootstrap Target: Neat Orchestration of StepsCombining the
import-headers,import-snapshots, andimport-noncesinto a singlebootstraptarget is a smart touch. Just ensure that if any sub-target fails, it’s handled as intended.
44-48: Enforce Peer Address: A Solid Safety CheckThis target neatly guards against running the node without a defined
AMARU_PEER_ADDRESS. It’s a simple yet effective barrier against potential misconfigurations.
50-51: Run Target: All Systems Go!The
runtarget, by building on the enforce-peer-address check, is crisp and functional. It sets the node rolling with clarity.
KtorZ
force-pushed
the
jeluard/ci-snapshots
branch
from
c250b71 to
4ab0004
Compare
This is the default behaviour when running 'cargo test', and it's been slightly annoying to have to run them one by one every time. Now, they happily all run at once by sharing and reusing connections to the on-disk snapshots. Signed-off-by: KtorZ <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
KtorZ
force-pushed
the
jeluard/ci-snapshots
branch
2 times, most recently
from
b8a8cc9 to
9fb2111
Compare
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Interstingly enough, a 'cache-hit' value of 'false' does not indicate a miss. It indicates a hit on a restore key (and not a _direct_ hit). A miss is denoted by an empty string. :| Signed-off-by: KtorZ <[email protected]>
KtorZ
force-pushed
the
jeluard/ci-snapshots
branch
2 times, most recently
from
d4c731b to
82efb1e
Compare
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
KtorZ
force-pushed
the
jeluard/ci-snapshots
branch
from
b63f086 to
4ff9688
Compare
Signed-off-by: jeluard <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
…ushed. Signed-off-by: KtorZ <[email protected]>
…che conflict Signed-off-by: KtorZ <[email protected]>
KtorZ
force-pushed
the
jeluard/ci-snapshots
branch
from
ff7f78a to
1cbf29c
Compare
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes