chore: de-duplicate shared helpers between the two testnet load tests - #110
Open
olathedev wants to merge 3 commits into
Open
chore: de-duplicate shared helpers between the two testnet load tests#110olathedev wants to merge 3 commits into
olathedev wants to merge 3 commits into
Conversation
The two testnet load tests each carry their own copy of the same logging, timing and invocation helpers. Add the shared home for them first, before either script is migrated, so CI is checking the new file from the commit that introduces it. `shellcheck scripts/*.sh` does not glob into scripts/lib/, so the lint step now lists that directory explicitly. It also gains -x: without it shellcheck refuses to follow a `source` line and reports SC1091, which is an error under the default severity; with it, each script is checked against the helper definitions it actually gets at runtime rather than the two halves being analysed in isolation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drops this script's copies of log/log_success/log_error, get_time/ elapsed_time/avg_time, gen_key, balance and invoke_contract in favour of scripts/lib/load-test-common.sh. Every one of them is byte-identical to the shared version once parsed, so behaviour and output are unchanged. balance() was the single exception: it hardcoded `--source load_deployer`, which the v2 script spells `v2load_deployer`. The identity name now lives in DEPLOYER_IDENTITY, set once in the configuration block and used at the script's other deployer call sites too, so the shared helper reads it instead of a literal and the name still has exactly one definition. Phase logic, the contract-specific parameters and the resolver-committee setup all stay here; only the genuinely duplicated helpers moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same removal as the previous commit, from the v2 script's own copies of the identical helpers, with DEPLOYER_IDENTITY set to v2load_deployer so balance() keeps sourcing its read-only query from this script's deployer. network_id() and json_field() stay put: both are specific to v2's commitment scheme and have no counterpart in the v1 script, so there is nothing to share. With this the duplication the issue describes is gone, one copy of each helper remains, and `shellcheck -x scripts/*.sh scripts/lib/*.sh` is clean. Closes drydocs#97 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
collinsezedike
requested changes
Aug 23, 2026
collinsezedike
left a comment
Collaborator
There was a problem hiding this comment.
The refactor itself is solid, verified byte-identical helper extraction, careful set -euo pipefail scoping, and the -x fix for shellcheck's SC1091 on the new source line all check out. But ci.yml's shellcheck command changed without updating three other places that document the equivalent local command:
- Makefile:15 still runs
shellcheck scripts/*.sh - CONTRIBUTING.md:194 still lists
shellcheck scripts/*.shas the raw local command - README.md:116 still shows
shellcheck scripts/*.shlabeled as "same checks CI runs"
None of these were touched by this PR, so running make check or copying the README/CONTRIBUTING command locally now fails with SC1091 on the new source line, even though CI itself passes with the -x flag. Please update all three to shellcheck -x scripts/*.sh scripts/lib/*.sh to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #97
What changed
scripts/testnet-load.shandscripts/testnet-load-v2.sheach carried their own copy of nine helpers. Those now live inscripts/lib/load-test-common.sh, which both scripts source:log/log_success/log_errorget_time/elapsed_time/avg_timegen_key,balance,invoke_contractSTELLAR="stellar", which both set identicallynetwork_id(),json_field(), the phase logic, and every contract-specific parameter stay where they were — the v2 script's helpers have no v1 counterpart, so there's nothing to share.Split into three commits: the lib and CI change first (so the new file is linted from the commit that adds it), then one script each.
On the shellcheck glob
Answering the question from the issue thread: I kept the file at
scripts/lib/and updated the workflow rather than flattening it intoscripts/, because the glob wasn't the only problem.shellcheck scripts/*.shrefuses to follow asourceline at all without-x, and reports SC1091, which is fatal under the default severity — that would have failed CI even with the lib sitting directly inscripts/. So the step is now:-xmakes shellcheck follow the source and check each script against the helper definitions it actually gets at runtime, instead of analysing the two halves in isolation; the explicitscripts/lib/*.shkeeps the lib checked standalone too. The# shellcheck source=SCRIPTDIR/lib/...directive on each source line resolves against the script's own directory, so the lint result doesn't depend on the workflow's working directory. Verified green at all three commits with shellcheck 0.10.0.On
set -euo pipefailand scopingThe lib deliberately has no shebang and no
setline of its own. Sourcing runs in the caller's shell, which already sets-euo pipefail; re-setting them in the lib would silently re-enable an option for any future caller that had turned one off. It carries# shellcheck shell=bashinstead so shellcheck still knows the dialect.Every helper keeps its
localdeclarations, so nothing new leaks into the callers' scope. The one shared name that isn't a function isSTELLAR, which both scripts already defined to the same value.The one helper that wasn't identical
balance()differed by exactly one token: v1 hardcoded--source load_deployer, v2--source v2load_deployer. The identity name now lives inDEPLOYER_IDENTITY, set once per script in its configuration block, and the shared helper reads that. Each script's other deployer call sites use the variable too, so the name has one definition rather than being spelled out in five or six places.NETWORKandDEPLOYER_IDENTITYmust be set before the source line; both scripts set them in the config block immediately above it, and there's a comment on the source line saying so.Verification
No behaviour change intended, and the helpers were checked rather than eyeballed: sourcing the pre-change definitions and the new shared ones and diffing
declare -ffor all nine gives byte-identical post-parse output in both directions, withbalance's source argument as the sole expected difference. Both scripts passbash -n, andshellcheck -x scripts/*.sh scripts/lib/*.shis clean at each commit.The scripts themselves deploy to live testnet, so I haven't run them end to end.
🤖 Generated with Claude Code