chore: fix test suite configuration script #2402
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (1)
scripts/configure_test_suite_network.sh (1)
67-68: Consider caching mechanism for resilienceThe script relies heavily on external API calls which could cause issues if the insight API is temporarily unavailable.
Consider implementing a caching mechanism or fallback values to improve reliability:
- Cache the last known height in a local file
- Use cached value if API call fails
- Implement exponential backoff for retries
Would you like me to provide an implementation example?
| CERT_FLAG="" | ||
| ST_EXECUTION_INTERVAL=15000 | ||
| fi | ||
| SKIP_SYNC_BEFORE_HEIGHT=4800 # $(curl -s $INSIGHT_URL | jq '.height - 200') | ||
| INSIGHT_URL="http://insight.${NETWORK_STRING#devnet-}.networks.dash.org:3001/insight-api/sync" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for INSIGHT_URL construction
The dynamic URL construction using string manipulation could fail if the network string format changes. Consider adding validation:
+ if [[ ! "$NETWORK_STRING" =~ ^(devnet|devnet-.*|testnet)$ ]]; then
+ echo "Error: Invalid network string format: $NETWORK_STRING"
+ exit 1
+ fi
INSIGHT_URL="http://insight.${NETWORK_STRING#devnet-}.networks.dash.org:3001/insight-api/sync"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSIGHT_URL="http://insight.${NETWORK_STRING#devnet-}.networks.dash.org:3001/insight-api/sync" | |
| if [[ ! "$NETWORK_STRING" =~ ^(devnet|devnet-.*|testnet)$ ]]; then | |
| echo "Error: Invalid network string format: $NETWORK_STRING" | |
| exit 1 | |
| fi | |
| INSIGHT_URL="http://insight.${NETWORK_STRING#devnet-}.networks.dash.org:3001/insight-api/sync" |
| CERT_FLAG="" | ||
| ST_EXECUTION_INTERVAL=15000 | ||
| fi | ||
| SKIP_SYNC_BEFORE_HEIGHT=4800 # $(curl -s $INSIGHT_URL | jq '.height - 200') | ||
| INSIGHT_URL="http://insight.${NETWORK_STRING#devnet-}.networks.dash.org:3001/insight-api/sync" | ||
| SKIP_SYNC_BEFORE_HEIGHT=$(curl -s $INSIGHT_URL | jq '.height - 200') |
There was a problem hiding this comment.
Add error handling for height calculation
The curl command and height calculation lack error handling for network issues or invalid responses. Consider adding:
- SKIP_SYNC_BEFORE_HEIGHT=$(curl -s $INSIGHT_URL | jq '.height - 200')
+ # Add timeout and retry logic for curl
+ SYNC_DATA=$(curl -s -f --max-time 10 --retry 3 --retry-delay 2 $INSIGHT_URL)
+ if [ $? -ne 0 ]; then
+ echo "Error: Failed to fetch sync data from $INSIGHT_URL"
+ exit 1
+ fi
+
+ # Validate height exists and is a number
+ HEIGHT=$(echo "$SYNC_DATA" | jq -e '.height')
+ if [ $? -ne 0 ] || ! [[ "$HEIGHT" =~ ^[0-9]+$ ]]; then
+ echo "Error: Invalid height received from insight API"
+ exit 1
+ fi
+
+ SKIP_SYNC_BEFORE_HEIGHT=$((HEIGHT - 200))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SKIP_SYNC_BEFORE_HEIGHT=$(curl -s $INSIGHT_URL | jq '.height - 200') | |
| # Add timeout and retry logic for curl | |
| SYNC_DATA=$(curl -s -f --max-time 10 --retry 3 --retry-delay 2 $INSIGHT_URL) | |
| if [ $? -ne 0 ]; then | |
| echo "Error: Failed to fetch sync data from $INSIGHT_URL" | |
| exit 1 | |
| fi | |
| # Validate height exists and is a number | |
| HEIGHT=$(echo "$SYNC_DATA" | jq -e '.height') | |
| if [ $? -ne 0 ] || ! [[ "$HEIGHT" =~ ^[0-9]+$ ]]; then | |
| echo "Error: Invalid height received from insight API" | |
| exit 1 | |
| fi | |
| SKIP_SYNC_BEFORE_HEIGHT=$((HEIGHT - 200)) |
Issue being fixed or feature implemented
The test suite network configuration script doesn't work
What was done?
How Has This Been Tested?
Running locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
INSIGHT_URLfordevnetandtestnetnetworks.SKIP_SYNC_BEFORE_HEIGHTto adapt to network state.Chores