🧪 [testing] Add unit tests for RCON hex functions and fix trailing input bug#98
Conversation
- Modified tools/rcon.sh to be sourceable by wrapping main execution in BASH_SOURCE check. - Fixed bug in reverse_hex_endian where short/trailing inputs were swallowed. - Added tools/tests/test_rcon.sh with comprehensive unit tests for hex utilities. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the reverse_hex_endian function in tools/rcon.sh to handle non-null-terminated or short inputs and wraps the main execution logic to enable sourcing. It also introduces a new unit test suite, tools/tests/test_rcon.sh. The review feedback suggests aligning the new test script with the repository's Bash standards by adding the required boilerplate (strict mode, shell options) and utilizing shared utility functions from common.sh for consistent output formatting.
There was a problem hiding this comment.
Pull request overview
Adds basic unit coverage for the pure-Bash RCON hex helpers and hardens reverse_hex_endian/tools/rcon.sh so it can be safely sourced without invoking the CLI entrypoint.
Changes:
- Add
tools/tests/test_rcon.shunit tests forreverse_hex_endian,decode_hex_int, andencode_int. - Fix
reverse_hex_endianto emit trailing input shorter than 8 hex chars instead of dropping it. - Wrap
rcon_commandinvocation behind aBASH_SOURCEmain-guard to allow sourcingtools/rcon.sh.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/tests/test_rcon.sh | New unit test script covering core hex utilities and the trailing-input fix. |
| tools/rcon.sh | Fix trailing-input handling in reverse_hex_endian and add a main guard for safe sourcing. |
|
|
||
| # Source the rcon.sh script | ||
| # Since we wrapped the main execution call, it should only load functions | ||
| source "$(dirname "$0")/../rcon.sh" |
| printf "\e[32m[PASS]\e[0m %s\n" "$description" | ||
| else | ||
| printf "\e[31m[FAIL]\e[0m %s\n" "$description" |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -0,0 +1,77 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
SUGGESTION: Missing project-required shellcheck directive. Use # shellcheck enable=all shell=bash source-path=SCRIPTDIR to match repository conventions (see tools/backup.sh:2).
| tests_failed=0 | ||
|
|
||
| # Assertion function | ||
| assert_equals() { |
There was a problem hiding this comment.
WARNING: Missing project's required Bash boilerplate. Add set -euo pipefail, shopt -s nullglob globstar, IFS=$'\n\t' LC_ALL=C (see tools/backup.sh:4-7).
| tests_failed=0 | ||
|
|
||
| # Assertion function | ||
| assert_equals() { |
There was a problem hiding this comment.
WARNING: Not sourcing common.sh for shared utilities (print_header, print_success, etc.). Should source common.sh like other tools (see tools/monitor.sh:5-11).
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All previously identified issues have been resolved in the incremental changes:
The changes correctly implement the intended fixes and follow project conventions. Files Reviewed (2 files)
Previous Review Summary (commit de41729)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit de41729)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Code Quality Notes
Files Reviewed (3 files)
Reviewed by laguna-m.1-20260312:free · Input: 243.6K · Output: 8.4K · Cached: 62.5K |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
🧪 [testing] Add unit tests for RCON hex functions and fix trailing input bug
🎯 What
Addressed the missing test coverage for the core hex manipulation functions in
tools/rcon.sh. During implementation, a bug was identified wherereverse_hex_endianwould silently swallow any input shorter than 8 characters (or trailing data at the end of a stream); this has been fixed.📊 Coverage
reverse_hex_endian: Now tested with standard 8-char hex, multi-block hex strings, and short inputs (ensuring they are preserved as-is).decode_hex_int: Verified correct decoding of little-endian hex to decimal.encode_int: Verified correct encoding of decimal integers to little-endian hex.✨ Result
tools/rcon.shis now safely sourceable for testing without executing the command-line interface.Tests can be run with:
bash tools/tests/test_rcon.shPR created automatically by Jules for task 15791535305284024798 started by @Ven0m0