Add test script #33
Add test script #33
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new Bash script Changes
Sequence DiagramsequenceDiagram
participant CI as Travis CI
participant TestScript as test_chkdm.sh
participant Chkdm as chkdm executable
CI->>TestScript: Execute test script
TestScript->>TestScript: Check chkdm executable
TestScript->>Chkdm: Test valid domains
Chkdm-->>TestScript: Validation results
TestScript->>Chkdm: Test invalid domains
Chkdm-->>TestScript: Validation results
TestScript-->>CI: Report test summary
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
Files not reviewed (1)
- test_chkdm.sh: Language not supported
![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 (3)
test_chkdm.sh (3)
50-53: Improve temporary directory cleanup.The cleanup trap could fail if the script is interrupted during directory creation. Move the trap before directory creation:
- local tmp_dir - tmp_dir=$(mktemp -d) - trap 'rm -rf "$tmp_dir"' EXIT + local tmp_dir + trap 'rm -rf "${tmp_dir:-}"' EXIT + tmp_dir=$(mktemp -d)
81-126: Consider parallelizing invalid domain tests.The invalid domain tests are run sequentially. Consider using the same parallel execution strategy as
test_valid_domainsfor consistent performance:test_invalid_domains() { # ... existing setup ... + local tmp_dir + trap 'rm -rf "${tmp_dir:-}"' EXIT + tmp_dir=$(mktemp -d) + for domain in "${invalid_domains[@]}"; do echo -e " Testing domain: ${BLUE}$domain${NC}" - local test_start_time test_end_time test_duration - test_start_time=$(date +%s%N) - - if $CHKDM "$domain" > /dev/null 2>&1; then - echo -e " ${RED}✗ Error: domain '$domain' was marked as valid${NC}" - else - echo -e " ${GREEN}✓ OK${NC}" - ((passed++)) - fi - - test_end_time=$(date +%s%N) - test_duration=$(( (test_end_time - test_start_time) / 1000000 )) - echo -e " ${BLUE}Time: ${YELLOW}${test_duration}ms${NC}\n" + ( + local test_start_time test_end_time test_duration + test_start_time=$(date +%s%N) + + if $CHKDM "$domain" > /dev/null 2>&1; then + echo -e " ${RED}✗ Error: domain '$domain' was marked as valid${NC}" + echo "0" > "$tmp_dir/$domain" + else + echo -e " ${GREEN}✓ OK${NC}" + echo "1" > "$tmp_dir/$domain" + fi + + test_end_time=$(date +%s%N) + test_duration=$(( (test_end_time - test_start_time) / 1000000 )) + echo -e " ${BLUE}Time: ${YELLOW}${test_duration}ms${NC}\n" + ) & done + wait + + for domain in "${invalid_domains[@]}"; do + if [ -f "$tmp_dir/$domain" ] && [ "$(cat "$tmp_dir/$domain")" = "1" ]; then + ((passed++)) + fi + done
128-143: Enhance executable check with version information.Add version information to the executable check for better debugging:
check_chkdm_executable() { echo -e " ${BLUE}Checking program: ${NC}" if [ ! -f "$CHKDM" ]; then echo -e " ${RED}✗ Error: program not found${NC}" return 1 fi echo -e " ${GREEN}✓ Found${NC}" echo -e " ${BLUE}Checking permissions${NC}" if [ ! -x "$CHKDM" ]; then echo -e " ${RED}✗ Error: program is not executable${NC}" return 1 fi echo -e " ${GREEN}✓ Executable${NC}\n" + + echo -e " ${BLUE}Version information:${NC}" + if ! $CHKDM --version; then + echo -e " ${YELLOW}! Warning: Could not get version information${NC}\n" + fi + return 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.travis.yml(2 hunks)test_chkdm.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Travis CI - Branch
🔇 Additional comments (2)
.travis.yml (1)
35-38: LGTM! Test scenarios are well-preserved.The changes effectively centralize test execution in
test_chkdm.shwhile maintaining coverage of all scenarios:
- Default configuration
- Custom DNS file
- Missing default DNS file
- Non-existent custom DNS file
Also applies to: 67-70
test_chkdm.sh (1)
145-187: LGTM! Well-structured main function.The main function effectively:
- Orchestrates test execution
- Handles errors appropriately
- Provides clear test summary and timing information
test_chkdm.sh
Outdated
| test_single_valid_domain() { | ||
| local domain="$1" | ||
| local start_time end_time duration | ||
| start_time=$(date +%s%N) | ||
| local result=0 | ||
|
|
||
| if ! $CHKDM "$domain" > /dev/null 2>&1; then | ||
| echo -e " ${RED}✗ Error: domain '$domain' was marked as invalid${NC}" | ||
| result=1 | ||
| else | ||
| echo -e " ${GREEN}✓ OK${NC}" | ||
| fi | ||
|
|
||
| end_time=$(date +%s%N) | ||
| duration=$(( (end_time - start_time) / 1000000 )) | ||
| echo -e " ${BLUE}Time: ${YELLOW}${duration}ms${NC}\n" | ||
|
|
||
| return $result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling to prevent hanging tests.
Add a timeout to test_single_valid_domain to prevent hanging on slow DNS responses:
test_single_valid_domain() {
local domain="$1"
local start_time end_time duration
start_time=$(date +%s%N)
local result=0
- if ! $CHKDM "$domain" > /dev/null 2>&1; then
+ if ! timeout 10s $CHKDM "$domain" > /dev/null 2>&1; then
echo -e " ${RED}✗ Error: domain '$domain' was marked as invalid${NC}"
result=1📝 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.
| test_single_valid_domain() { | |
| local domain="$1" | |
| local start_time end_time duration | |
| start_time=$(date +%s%N) | |
| local result=0 | |
| if ! $CHKDM "$domain" > /dev/null 2>&1; then | |
| echo -e " ${RED}✗ Error: domain '$domain' was marked as invalid${NC}" | |
| result=1 | |
| else | |
| echo -e " ${GREEN}✓ OK${NC}" | |
| fi | |
| end_time=$(date +%s%N) | |
| duration=$(( (end_time - start_time) / 1000000 )) | |
| echo -e " ${BLUE}Time: ${YELLOW}${duration}ms${NC}\n" | |
| return $result | |
| } | |
| test_single_valid_domain() { | |
| local domain="$1" | |
| local start_time end_time duration | |
| start_time=$(date +%s%N) | |
| local result=0 | |
| if ! timeout 10s $CHKDM "$domain" > /dev/null 2>&1; then | |
| echo -e " ${RED}✗ Error: domain '$domain' was marked as invalid${NC}" | |
| result=1 | |
| else | |
| echo -e " ${GREEN}✓ OK${NC}" | |
| fi | |
| end_time=$(date +%s%N) | |
| duration=$(( (end_time - start_time) / 1000000 )) | |
| echo -e " ${BLUE}Time: ${YELLOW}${duration}ms${NC}\n" | |
| return $result | |
| } |
There was a problem hiding this comment.
but a timeout result won't match the corresponding error message, does it?
There was a problem hiding this comment.
@PeterDaveHello You're absolutely right - a timeout would produce a different exit code (124) compared to a domain validation failure. We should handle it separately. Here's an improved version:
test_single_valid_domain() {
local domain="$1"
local start_time end_time duration
start_time=$(date +%s%N)
local result=0
if ! timeout 10s $CHKDM "$domain" > /dev/null 2>&1; then
if [ $? -eq 124 ]; then
echo -e " ${RED}✗ Error: domain '$domain' check timed out after 10s${NC}"
else
echo -e " ${RED}✗ Error: domain '$domain' was marked as invalid${NC}"
fi
result=1
else
echo -e " ${GREEN}✓ OK${NC}"
fi
end_time=$(date +%s%N)
duration=$(( (end_time - start_time) / 1000000 ))
echo -e " ${BLUE}Time: ${YELLOW}${duration}ms${NC}\n"
return $result
}This version distinguishes between timeout errors and validation failures. Would you prefer this approach, or would you rather not have timeout handling at all?
(_/)
(･ω･)
⊂ﾉ
| #!/usr/bin/env bash | ||
|
|
||
| SCRIPT_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" | ||
| CHKDM="${SCRIPT_DIR}/chkdm" | ||
|
|
||
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| BLUE='\033[0;34m' | ||
| YELLOW='\033[0;33m' | ||
| NC='\033[0m' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add script safety flags and improve portability.
- Add safety flags to catch errors early:
#!/usr/bin/env bash
+set -euo pipefail
+IFS=$'\n\t'- Make
readlinkusage portable to macOS:
-SCRIPT_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"📝 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.
| #!/usr/bin/env bash | |
| SCRIPT_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" | |
| CHKDM="${SCRIPT_DIR}/chkdm" | |
| RED='\033[0;31m' | |
| GREEN='\033[0;32m' | |
| BLUE='\033[0;34m' | |
| YELLOW='\033[0;33m' | |
| NC='\033[0m' | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| IFS=$'\n\t' | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| CHKDM="${SCRIPT_DIR}/chkdm" | |
| RED='\033[0;31m' | |
| GREEN='\033[0;32m' | |
| BLUE='\033[0;34m' | |
| YELLOW='\033[0;33m' | |
| NC='\033[0m' |
PeterDaveHello
force-pushed
the
add-test-script
branch
from
f10cd89 to
8f36c6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test_chkdm.sh (2)
1-4:⚠️ Potential issueAdd script safety flags and improve portability.
Previous feedback about script safety and portability remains unaddressed.
Apply these changes:
#!/usr/bin/env bash +set -euo pipefail +IFS=$'\n\t' -SCRIPT_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
31-49:⚠️ Potential issueAdd timeout handling and input validation.
- Previous feedback about timeout handling remains unaddressed.
- Missing input validation for the domain parameter.
test_single_valid_domain() { local domain="$1" + [[ -z "$domain" ]] && { echo "Domain parameter is required"; return 1; } + local start_time end_time duration start_time=$(get_time_ms) local result=0 - if ! $CHKDM "$domain" > /dev/null 2>&1; then + if ! timeout 10s $CHKDM "$domain" > /dev/null 2>&1; then echo -e " ${RED}✗ Error: domain '$domain' was marked as invalid${NC}" result=1
🧹 Nitpick comments (5)
test_chkdm.sh (5)
6-10: Declare color variables as readonly constants.Color codes should be declared as readonly to prevent accidental modification.
-RED='\033[0;31m' -GREEN='\033[0;32m' -BLUE='\033[0;34m' -YELLOW='\033[0;33m' -NC='\033[0m' +readonly RED='\033[0;31m' +readonly GREEN='\033[0;32m' +readonly BLUE='\033[0;34m' +readonly YELLOW='\033[0;33m' +readonly NC='\033[0m'
22-29: Improve duration formatting precision.The current implementation loses precision by only showing "<1" for durations less than 1ms.
format_duration() { local duration=$1 if (( duration < 1000000 )); then - echo "<1" + printf "%.2f" "$(echo "scale=2; $duration/1000000" | bc)" else - echo "$((duration / 1000000))" + printf "%.0f" "$(echo "scale=0; $duration/1000000" | bc)" fi }
101-112: Enhance invalid domain test cases.Consider adding more edge cases:
- Domains with leading/trailing hyphens
- Punycode domains
- Mixed case domains
local invalid_domains=( "example.123" "example.c-m" "example.a" "this-is-a-very-long-label-that-exceeds-the-63-character-limit123.com" "sub.this-is-another-very-long-label-that-exceeds-the-63-character-limit.net" "example--domain.com" "sub--domain.net" "" "test..domain" "invalid#domain.com" + "-leading-hyphen.com" + "trailing-hyphen-.com" + "xn--invalid-punycode" + "MIXED.case.DOMAIN" )
164-204: Add command-line options for better usability.Consider adding options for:
- Verbose mode
- Running specific test suites
- Custom timeout values
Example implementation:
parse_args() { while getopts "vt:h" opt; do case $opt in v) VERBOSE=1 ;; t) TIMEOUT="$OPTARG" ;; h) show_help; exit 0 ;; *) show_help; exit 1 ;; esac done }
1-206: Overall assessment: Good foundation with room for improvement.The script provides a solid testing framework with:
✓ Well-structured test organization
✓ Clear output formatting
✓ Good error handlingKey areas for improvement:
- Address previous review comments about script safety and timeout handling
- Enhance security with proper path validation
- Add more configuration options for better usability
- Improve robustness of temporary file operations
Consider splitting the script into modules for better maintainability:
- Move test cases to separate configuration files
- Create a library file for common functions
- Implement a plugin system for custom domain validators
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.travis.yml(2 hunks)test_chkdm.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .travis.yml
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Travis CI - Branch
| local tmp_dir | ||
| tmp_dir=$(mktemp -d) | ||
| trap 'rm -rf "$tmp_dir"' EXIT |
There was a problem hiding this comment.
Add error handling for temporary directory operations.
The script should handle failures in temporary directory creation and ensure atomic file operations.
- tmp_dir=$(mktemp -d)
+ tmp_dir=$(mktemp -d) || { echo "Failed to create temp directory"; return 1; }
+ chmod 700 "$tmp_dir"
trap 'rm -rf "$tmp_dir"' EXIT📝 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.
| local tmp_dir | |
| tmp_dir=$(mktemp -d) | |
| trap 'rm -rf "$tmp_dir"' EXIT | |
| local tmp_dir | |
| tmp_dir=$(mktemp -d) || { echo "Failed to create temp directory"; return 1; } | |
| chmod 700 "$tmp_dir" | |
| trap 'rm -rf "$tmp_dir"' EXIT |
There was a problem hiding this comment.
Which part ensures the atomic file operations, chmod 700?
There was a problem hiding this comment.
@PeterDaveHello You're right to question this. I apologize for the confusion in my previous comment. The chmod 700 command only sets directory permissions (read/write/execute for the owner only) and doesn't ensure atomic operations. The mktemp -d command does provide safety by atomically creating a unique temporary directory, but I was incorrect to mention "atomic file operations" in the context of these changes. Would you like me to revise my suggestion to focus solely on error handling for temporary directory creation?
✏️ Learnings added
Learnt from: PeterDaveHello
PR: PeterDaveHello/chkdomain#33
File: test_chkdm.sh:69-71
Timestamp: 2025-01-14T16:42:09.715Z
Learning: The suggestion to ensure "atomic file operations" was incorrect in the context of temporary directory creation. The `chmod 700` command only sets directory permissions (rwx for owner only) and doesn't provide atomicity. The `mktemp -d` command itself provides safety by atomically creating a unique temporary directory.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| check_chkdm_executable() { | ||
| echo -e " ${BLUE}Checking program: ${NC}" | ||
| if [ ! -f "$CHKDM" ]; then | ||
| echo -e " ${RED}✗ Error: program not found${NC}" | ||
| return 1 | ||
| fi | ||
| echo -e " ${GREEN}✓ Found${NC}" | ||
|
|
||
| echo -e " ${BLUE}Checking permissions${NC}" | ||
| if [ ! -x "$CHKDM" ]; then | ||
| echo -e " ${RED}✗ Error: program is not executable${NC}" | ||
| return 1 | ||
| fi | ||
| echo -e " ${GREEN}✓ Executable${NC}\n" | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add absolute path validation.
Ensure the CHKDM path is absolute to prevent potential security issues.
check_chkdm_executable() {
+ if [[ "$CHKDM" != /* ]]; then
+ echo -e " ${RED}✗ Error: program path must be absolute${NC}"
+ return 1
+ fi
+
echo -e " ${BLUE}Checking program: ${NC}"📝 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.
| check_chkdm_executable() { | |
| echo -e " ${BLUE}Checking program: ${NC}" | |
| if [ ! -f "$CHKDM" ]; then | |
| echo -e " ${RED}✗ Error: program not found${NC}" | |
| return 1 | |
| fi | |
| echo -e " ${GREEN}✓ Found${NC}" | |
| echo -e " ${BLUE}Checking permissions${NC}" | |
| if [ ! -x "$CHKDM" ]; then | |
| echo -e " ${RED}✗ Error: program is not executable${NC}" | |
| return 1 | |
| fi | |
| echo -e " ${GREEN}✓ Executable${NC}\n" | |
| return 0 | |
| } | |
| check_chkdm_executable() { | |
| if [[ "$CHKDM" != /* ]]; then | |
| echo -e " ${RED}✗ Error: program path must be absolute${NC}" | |
| return 1 | |
| fi | |
| echo -e " ${BLUE}Checking program: ${NC}" | |
| if [ ! -f "$CHKDM" ]; then | |
| echo -e " ${RED}✗ Error: program not found${NC}" | |
| return 1 | |
| fi | |
| echo -e " ${GREEN}✓ Found${NC}" | |
| echo -e " ${BLUE}Checking permissions${NC}" | |
| if [ ! -x "$CHKDM" ]; then | |
| echo -e " ${RED}✗ Error: program is not executable${NC}" | |
| return 1 | |
| fi | |
| echo -e " ${GREEN}✓ Executable${NC}\n" | |
| return 0 | |
| } |
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
💪
Summary by CodeRabbit
New Features
Chores