fix: guard CI tests for missing tools - #45
Conversation
|
/gemini review |
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — all changes are defensive test guards with no impact on production code. All findings are P2 style/consistency observations. The changes correctly follow the established skip pattern used elsewhere in the file and do not touch any application logic. The one open question (Test 63 guard necessity) is worth a follow-up but does not block merging. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Run test] --> B{command -v tool?}
B -- "tool installed" --> C[Execute u7 command]
C --> D{Expected outcome?}
D -- "yes" --> E["✓ PASSED++"]
D -- "no" --> F["✗ FAILED++"]
B -- "tool NOT installed" --> G["✓ Skipped — PASSED++"]
style E fill:#2d6a2d,color:#fff
style G fill:#2d6a2d,color:#fff
style F fill:#8b0000,color:#fff
Prompt To Fix All With AIThis is a comment left during a code review.
Path: test.sh
Line: 648-653
Comment:
**Guard may hide regression in u7's own file-not-found handling**
Test 63 now skips when `qsv` is absent, but the directly analogous tests for the `cv csv` subcommand (tests 77 and 78, lines 733–738) run the missing-file and unsupported-format assertions *without* a `qsv` guard:
```bash
# Test 77 – no qsv guard
result=$(u7 cv csv convert.csv to xml 2>&1)
assert_contains "cv csv rejects unsupported format" "Unsupported" "$result"
# Test 78 – no qsv guard
result=$(u7 cv csv nonexistent.csv to json 2>&1)
assert_contains "cv csv reports missing file" "not found" "$result"
```
If `u7 sh csv` checks for file existence inside the tool itself (before calling `qsv`), then Test 63 does **not** need `qsv` to be installed and the guard is overly conservative — it will silently skip a test that could catch a regression in `u7`'s file-not-found logic every time CI runs without `qsv`.
Worth verifying whether `u7 sh csv` relies on `qsv` to produce the "File not found" message or whether `u7` itself performs the check. If the latter, the guard should be removed (or the test should at minimum document why it differs from tests 77/78).
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: guard tests requiring yq, qsv, ifco..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if command -v qsv &>/dev/null; then | ||
| assert_contains "sh csv reports missing file" "File not found" "$(u7 sh csv nonexistent.csv 2>&1)" | ||
| else | ||
| echo -e "${GREEN}✓${NC} sh csv reports missing file (skipped - qsv not installed)" | ||
| ((PASSED++)) | ||
| fi |
There was a problem hiding this comment.
Test 63 now skips when qsv is absent, but the directly analogous tests for the cv csv subcommand (tests 77 and 78, lines 733–738) run the missing-file and unsupported-format assertions without a qsv guard:
# Test 77 – no qsv guard
result=$(u7 cv csv convert.csv to xml 2>&1)
assert_contains "cv csv rejects unsupported format" "Unsupported" "$result"
# Test 78 – no qsv guard
result=$(u7 cv csv nonexistent.csv to json 2>&1)
assert_contains "cv csv reports missing file" "not found" "$result"If u7 sh csv checks for file existence inside the tool itself (before calling qsv), then Test 63 does not need qsv to be installed and the guard is overly conservative — it will silently skip a test that could catch a regression in u7's file-not-found logic every time CI runs without qsv.
Worth verifying whether u7 sh csv relies on qsv to produce the "File not found" message or whether u7 itself performs the check. If the latter, the guard should be removed (or the test should at minimum document why it differs from tests 77/78).
Prompt To Fix With AI
This is a comment left during a code review.
Path: test.sh
Line: 648-653
Comment:
**Guard may hide regression in u7's own file-not-found handling**
Test 63 now skips when `qsv` is absent, but the directly analogous tests for the `cv csv` subcommand (tests 77 and 78, lines 733–738) run the missing-file and unsupported-format assertions *without* a `qsv` guard:
```bash
# Test 77 – no qsv guard
result=$(u7 cv csv convert.csv to xml 2>&1)
assert_contains "cv csv rejects unsupported format" "Unsupported" "$result"
# Test 78 – no qsv guard
result=$(u7 cv csv nonexistent.csv to json 2>&1)
assert_contains "cv csv reports missing file" "not found" "$result"
```
If `u7 sh csv` checks for file existence inside the tool itself (before calling `qsv`), then Test 63 does **not** need `qsv` to be installed and the guard is overly conservative — it will silently skip a test that could catch a regression in `u7`'s file-not-found logic every time CI runs without `qsv`.
Worth verifying whether `u7 sh csv` relies on `qsv` to produce the "File not found" message or whether `u7` itself performs the check. If the latter, the guard should be removed (or the test should at minimum document why it differs from tests 77/78).
How can I resolve this? If you propose a fix, please make it concise.![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 introduces environment-aware testing in test.sh by checking for the existence of required binaries (yq, ifconfig, zip, qsv) before executing specific test blocks. If a binary is missing, the test is skipped and recorded as a pass. The review feedback suggests consolidating these repetitive checks into a single helper function to improve code maintainability and readability.
| if command -v qsv &>/dev/null; then | ||
| assert_contains "sh csv reports missing file" "File not found" "$(u7 sh csv nonexistent.csv 2>&1)" | ||
| else | ||
| echo -e "${GREEN}✓${NC} sh csv reports missing file (skipped - qsv not installed)" | ||
| ((PASSED++)) | ||
| fi |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, consider adding a helper function to handle checking for tool existence and skipping tests. This pattern is repeated for qsv, yq, ifconfig, and zip.
You could add a function like this at the top of test.sh:
run_test_if_present() {
local tool="$1"
local test_name="$2"
if ! command -v "$tool" &>/dev/null; then
echo -e "${GREEN}✓${NC} $test_name (skipped - $tool not installed)"
((PASSED++))
return 1
fi
return 0
}Then you can simplify this test and others. I've provided a suggestion for this block. You can apply a similar change to other tests that have been modified in this pull request (e.g., Test 17, 33, 41, 52, and 76).
| if command -v qsv &>/dev/null; then | |
| assert_contains "sh csv reports missing file" "File not found" "$(u7 sh csv nonexistent.csv 2>&1)" | |
| else | |
| echo -e "${GREEN}✓${NC} sh csv reports missing file (skipped - qsv not installed)" | |
| ((PASSED++)) | |
| fi | |
| if run_test_if_present "qsv" "sh csv reports missing file"; then | |
| assert_contains "sh csv reports missing file" "File not found" "$(u7 sh csv nonexistent.csv 2>&1)" | |
| fi |
There was a problem hiding this comment.
Code Review
This pull request updates the test suite to gracefully skip tests that depend on external tools (yq, ifconfig, zip, qsv) if they are not installed on the system. The review suggests refactoring the repeated tool-checking logic into a helper function to improve maintainability and recommends redirecting stderr to /dev/null for the tool execution commands to ensure cleaner test output.
| if command -v yq &>/dev/null; then | ||
| u7 cv json test.json to yaml yield test.yaml >/dev/null | ||
| if [[ -f "test.yaml" ]]; then | ||
| result=$(cat test.yaml) | ||
| assert_contains "JSON to YAML conversion has key" "key" "$result" | ||
| assert_contains "JSON to YAML conversion has value" "value" "$result" | ||
| else | ||
| echo -e "${RED}✗${NC} JSON to YAML conversion (file not created)" | ||
| ((FAILED++)) | ||
| fi | ||
| else | ||
| echo -e "${RED}✗${NC} JSON to YAML conversion (file not created)" | ||
| ((FAILED++)) | ||
| echo -e "${GREEN}✓${NC} JSON to YAML conversion (skipped - yq not installed)" | ||
| ((PASSED++)) | ||
| fi |
There was a problem hiding this comment.
While this guard correctly skips the test if yq is not installed, this if command -v ... pattern is now repeated multiple times in this file for different tools (yq, ifconfig, zip, qsv). This increases code duplication and makes the test script harder to maintain.
To improve this, you could introduce a helper function to handle the tool check and skip message. For example, you could add this function alongside the other test helpers:
require_tool() {
if ! command -v "$1" &> /dev/null; then
echo -e "${GREEN}✓${NC} $2 (skipped - $1 not installed)"
((PASSED++))
return 1
fi
return 0
}Then, this test and others could be simplified:
# Test 17: Convert JSON to YAML
test_name="JSON to YAML conversion"
if require_tool "yq" "$test_name"; then
echo '{"key": "value"}' > test.json
u7 cv json test.json to yaml yield test.yaml >/dev/null
if [[ -f "test.yaml" ]]; then
result=$(cat test.yaml)
assert_contains "${test_name} has key" "key" "$result"
assert_contains "${test_name} has value" "value" "$result"
else
echo -e "${RED}✗${NC} ${test_name} (file not created)"
((FAILED++))
fi
fiThis would make the tests cleaner and avoid repeating the same logic. You could apply this pattern to all the new guards added in this PR.
| assert_contains "JSON to YAML conversion has key" "key" "$result" | ||
| assert_contains "JSON to YAML conversion has value" "value" "$result" | ||
| if command -v yq &>/dev/null; then | ||
| u7 cv json test.json to yaml yield test.yaml >/dev/null |
There was a problem hiding this comment.
For consistency with other tests in this file (e.g., Test 9, Test 41), and to prevent potential error messages from yq from cluttering the test output, it's better to redirect stderr to /dev/null as well.
| u7 cv json test.json to yaml yield test.yaml >/dev/null | |
| u7 cv json test.json to yaml yield test.yaml >/dev/null 2>&1 |
| result=$(cat convert.json) | ||
| assert_contains "cv csv to json works" "Alice" "$result" | ||
| if command -v qsv &>/dev/null; then | ||
| u7 cv csv convert.csv to json yield convert.json >/dev/null |
There was a problem hiding this comment.
For consistency with other tests in this file and to prevent potential error messages from qsv from cluttering the test output, it's better to redirect stderr to /dev/null as well.
| u7 cv csv convert.csv to json yield convert.json >/dev/null | |
| u7 cv csv convert.csv to json yield convert.json >/dev/null 2>&1 |
1 participant
Summary
command -vguards to 6 tests that fail in CI becauseyq,qsv,ifconfig, orzipare not installed on the GitHub Actions runneryqanddockertests elsewhere in the fileTest plan
bash test.sh