Test script - #390
Conversation
Walkthroughtest.ps1 is updated to recursively discover and execute all setup.ps1 files under a script path, configure global Git user settings, and add try/catch/finally error handling with logging. test.sh is populated with active commands to run basic-commits/setup.sh plus a large commented-out alternative scaffold. ChangesTest Setup Script Updates
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
test.sh (1)
10-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the large commented-out scaffold.
This block is invalid bash (mixes PowerShell syntax like
Set-Location,-ForegroundColor) and is dead code left over from an unfinished migration. Either implement the recursive scaffold or delete it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test.sh` around lines 10 - 45, The commented-out scaffold in test.sh is dead, invalid bash that mixes PowerShell-only syntax and should not remain in the script. Remove the entire obsolete block, or if the recursive setup runner is still needed, reimplement it properly in bash using the existing test.sh flow rather than the discarded try/catch and Set-Location logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test.ps1`:
- Around line 22-30: The loop in test.ps1 restores the working directory with a
fixed one-level ascent, which breaks when setup.ps1 is found at different
nesting depths. Update the iteration around Set-Location and .\setup.ps1 so it
always returns to the original $scriptPath after each invocation instead of
using relative cd..; this keeps each pass independent regardless of where the
matching setup.ps1 was discovered.
- Around line 9-18: The script is overwriting the machine’s global git identity
with hardcoded values and never restoring it. Update the test script around the
git config setup so it first captures the existing global user.name and
user.email, applies the temporary values only for the test, and then restores
the originals in the cleanup/finally path. Use the existing git config commands
and the surrounding cleanup logic in test.ps1 to ensure the developer/CI
identity is preserved.
- Line 20: The setup file lookup in the test script uses an undefined
$scriptPath variable, so fix the path source in the Get-ChildItem call to use a
defined location from this script or initialize $scriptPath before it is used.
Locate the change around the Get-ChildItem invocation in test.ps1 and ensure the
variable is assigned a valid path before the recursive search for setup.ps1
runs.
- Around line 20-24: Wrap the Get-ChildItem result in @(...) in the setup.ps1
discovery logic so $file is always an array-like collection instead of a single
FileInfo object. Update the test.ps1 loop that uses $file.length and $file[$i]
so it works reliably in the search-and-iterate block even when only one
setup.ps1 is found.
- Around line 5-31: The discovery loop in test.ps1 is wrapped in a single try
block, so one failing setup.ps1 stops the rest from running. Move error handling
into the per-file execution path around the .\setup.ps1 call inside the for loop
(or otherwise continue after each failure) so every discovered setup script is
attempted independently while preserving the loop in test.ps1.
In `@test.sh`:
- Around line 6-8: The test.sh setup flow does not guard the cd into
basic-commits, so the script can continue in the wrong directory if that change
fails. Update the setup sequence to fail fast on the directory change by adding
an explicit cd failure guard before running ./setup.sh, using the existing
basic-commits cd step as the place to fix this Shellcheck SC2164 issue.
- Around line 6-8: The shell test script is only running the basic-commits kata
setup, so the other kata setup scripts are skipped. Update the test.sh flow to
mirror test.ps1 by discovering and executing every setup.sh recursively, using
the same overall setup routine but without hardcoding basic-commits. Reference
the existing test.sh execution block and the setup.sh invocation logic when
making the change.
---
Nitpick comments:
In `@test.sh`:
- Around line 10-45: The commented-out scaffold in test.sh is dead, invalid bash
that mixes PowerShell-only syntax and should not remain in the script. Remove
the entire obsolete block, or if the recursive setup runner is still needed,
reimplement it properly in bash using the existing test.sh flow rather than the
discarded try/catch and Set-Location logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5f49713-7800-46cd-b3b5-4ca4fdf77ca4
📒 Files selected for processing (2)
test.ps1test.sh
| try | ||
| { | ||
| write-host "Executing the exercise setup.ps1 files to make sure they are running properly." -ForegroundColor Black -BackgroundColor Green | ||
|
|
||
| git config --global --replace-all user.name "Tomi Peltonen" | ||
| git config --global --replace-all user.email "top@praqma.net" | ||
|
|
||
| Write-Host "Git user updated" -foregroundcolor blue | ||
|
|
||
| Write-Host "user.name: " -foregroundcolor blue -nonewline | ||
| git config --global user.name | ||
|
|
||
| Write-Host "user.email: " -foregroundcolor blue -nonewline | ||
| git config --global user.email | ||
|
|
||
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 | ||
|
|
||
| for ($i=0; $i -lt $file.length; $i++) | ||
| { | ||
| Set-Location $file[$i].Directory | ||
| Write-Host Directory: $file[$i].Directory -ForegroundColor Black -BackgroundColor Yellow | ||
|
|
||
| .\setup.ps1 | ||
|
|
||
| cd.. | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wrapping the entire discovery loop in one try defeats the script's stated purpose.
The header comment states the goal is to "execute them to check if any errors occure" across all exercises, but since the whole for loop sits inside a single try, the first setup.ps1 that throws aborts execution of all remaining setup scripts — only one failure is ever reported per run.
♻️ Proposed fix: per-file error handling
for ($i=0; $i -lt $file.length; $i++)
{
- Set-Location $file[$i].Directory
- Write-Host Directory: $file[$i].Directory -ForegroundColor Black -BackgroundColor Yellow
+ try
+ {
+ Set-Location $file[$i].Directory
+ Write-Host Directory: $file[$i].Directory -ForegroundColor Black -BackgroundColor Yellow
+ .\setup.ps1
+ }
+ catch
+ {
+ write-host "Error running $($file[$i].FullName): $($_.Exception.Message)" -ForegroundColor Red
+ }
+ finally
+ {
+ Set-Location $scriptPath
+ }
+}
-
- .\setup.ps1
-
- cd..
-} 📝 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.
| try | |
| { | |
| write-host "Executing the exercise setup.ps1 files to make sure they are running properly." -ForegroundColor Black -BackgroundColor Green | |
| git config --global --replace-all user.name "Tomi Peltonen" | |
| git config --global --replace-all user.email "top@praqma.net" | |
| Write-Host "Git user updated" -foregroundcolor blue | |
| Write-Host "user.name: " -foregroundcolor blue -nonewline | |
| git config --global user.name | |
| Write-Host "user.email: " -foregroundcolor blue -nonewline | |
| git config --global user.email | |
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 | |
| for ($i=0; $i -lt $file.length; $i++) | |
| { | |
| Set-Location $file[$i].Directory | |
| Write-Host Directory: $file[$i].Directory -ForegroundColor Black -BackgroundColor Yellow | |
| .\setup.ps1 | |
| cd.. | |
| } | |
| } | |
| try | |
| { | |
| write-host "Executing the exercise setup.ps1 files to make sure they are running properly." -ForegroundColor Black -BackgroundColor Green | |
| git config --global --replace-all user.name "Tomi Peltonen" | |
| git config --global --replace-all user.email "top@praqma.net" | |
| Write-Host "Git user updated" -foregroundcolor blue | |
| Write-Host "user.name: " -foregroundcolor blue -nonewline | |
| git config --global user.name | |
| Write-Host "user.email: " -foregroundcolor blue -nonewline | |
| git config --global user.email | |
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 | |
| for ($i=0; $i -lt $file.length; $i++) | |
| { | |
| try | |
| { | |
| Set-Location $file[$i].Directory | |
| Write-Host Directory: $file[$i].Directory -ForegroundColor Black -BackgroundColor Yellow | |
| .\setup.ps1 | |
| } | |
| catch | |
| { | |
| write-host "Error running $($file[$i].FullName): $($_.Exception.Message)" -ForegroundColor Red | |
| } | |
| finally | |
| { | |
| Set-Location $scriptPath | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.ps1` around lines 5 - 31, The discovery loop in test.ps1 is wrapped in a
single try block, so one failing setup.ps1 stops the rest from running. Move
error handling into the per-file execution path around the .\setup.ps1 call
inside the for loop (or otherwise continue after each failure) so every
discovered setup script is attempted independently while preserving the loop in
test.ps1.
| git config --global --replace-all user.name "Tomi Peltonen" | ||
| git config --global --replace-all user.email "top@praqma.net" | ||
|
|
||
| Write-Host "Git user updated" -foregroundcolor blue | ||
|
|
||
| Write-Host "user.name: " -foregroundcolor blue -nonewline | ||
| git config --global user.name | ||
|
|
||
| Write-Host "user.email: " -foregroundcolor blue -nonewline | ||
| git config --global user.email |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Global git identity is overwritten with hardcoded values and never restored.
git config --global --replace-all permanently overwrites the developer's/CI machine's actual global user.name/user.email with hardcoded values, with no capture/restore of the previous values in the finally block. Anyone running this test script (e.g., locally) will have their real git identity silently replaced.
🔧 Proposed fix to preserve and restore the original identity
+$prevName = git config --global user.name
+$prevEmail = git config --global user.email
+
git config --global --replace-all user.name "Tomi Peltonen"
git config --global --replace-all user.email "top@praqma.net" finally
{
+ git config --global --replace-all user.name "$prevName"
+ git config --global --replace-all user.email "$prevEmail"
cd..
write-host "Finally block reached, we are good!" -ForegroundColor Black -BackgroundColor Green
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.ps1` around lines 9 - 18, The script is overwriting the machine’s global
git identity with hardcoded values and never restoring it. Update the test
script around the git config setup so it first captures the existing global
user.name and user.email, applies the temporary values only for the test, and
then restores the originals in the cleanup/finally path. Use the existing git
config commands and the surrounding cleanup logic in test.ps1 to ensure the
developer/CI identity is preserved.
| Write-Host "user.email: " -foregroundcolor blue -nonewline | ||
| git config --global user.email | ||
|
|
||
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
$scriptPath is never defined.
Get-ChildItem -Path $scriptPath references a variable that isn't declared or assigned anywhere in this file. This will resolve to $null, causing Get-ChildItem to error or default to an unintended location.
🐛 Proposed fix
+$scriptPath = $PSScriptRoot
+
$file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1📝 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.
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 | |
| $scriptPath = $PSScriptRoot | |
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.ps1` at line 20, The setup file lookup in the test script uses an
undefined $scriptPath variable, so fix the path source in the Get-ChildItem call
to use a defined location from this script or initialize $scriptPath before it
is used. Locate the change around the Get-ChildItem invocation in test.ps1 and
ensure the variable is assigned a valid path before the recursive search for
setup.ps1 runs.
| $file = Get-ChildItem -Path $scriptPath -Recurse -Include setup.ps1 | ||
|
|
||
| for ($i=0; $i -lt $file.length; $i++) | ||
| { | ||
| Set-Location $file[$i].Directory |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- test.ps1 (numbered) ---\n'
cat -n test.ps1 | sed -n '1,120p'
printf '\n--- grep for $file usage in test.ps1 ---\n'
rg -n '\$file\b|Get-ChildItem -Path \$scriptPath -Recurse -Include setup\.ps1' test.ps1 || true
printf '\n--- repository search for similar patterns ---\n'
rg -n '\$[A-Za-z_][A-Za-z0-9_]*\.length|\[[0-9]+\]' -g '*.ps1' .Repository: eficode-academy/git-katas
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'pwsh path: '
command -v pwsh || true
printf '\n'
if command -v pwsh >/dev/null 2>&1; then
pwsh -NoLogo -NoProfile -Command '
$scalar = [System.IO.FileInfo]::new("/tmp/example.txt")
$arr = @($scalar)
"scalar type: $($scalar.GetType().FullName)"
"scalar Length property: $($scalar.Length)"
"scalar[0] type: $($scalar[0].GetType().FullName)"
"scalar[1] is null: $($null -eq $scalar[1])"
"array length: $($arr.Length)"
'
fi
printf '\n--- test.ps1 ---\n'
cat -n test.ps1 | sed -n '1,80p'Repository: eficode-academy/git-katas
Length of output: 2042
Wrap Get-ChildItem in @(...) here
When only one setup.ps1 exists, $file is a FileInfo, so .Length is the file size and the loop can iterate the wrong number of times. @(...) keeps $file as a collection and makes $file[$i] safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.ps1` around lines 20 - 24, Wrap the Get-ChildItem result in @(...) in
the setup.ps1 discovery logic so $file is always an array-like collection
instead of a single FileInfo object. Update the test.ps1 loop that uses
$file.length and $file[$i] so it works reliably in the search-and-iterate block
even when only one setup.ps1 is found.
| for ($i=0; $i -lt $file.length; $i++) | ||
| { | ||
| Set-Location $file[$i].Directory | ||
| Write-Host Directory: $file[$i].Directory -ForegroundColor Black -BackgroundColor Yellow | ||
|
|
||
| .\setup.ps1 | ||
|
|
||
| cd.. | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Loop always ascends exactly one directory level regardless of nesting depth.
Since setup.ps1 files are found recursively (potentially at varying depths), returning with a single cd.. after each iteration only correctly restores the working directory when every match is exactly one level below $scriptPath. If any exercise nests setup.ps1 deeper, subsequent iterations use a stale directory as their base for Set-Location.
Consider setting the location back to $scriptPath directly after each script invocation instead of relying on relative cd.., as shown in the previous suggested diff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.ps1` around lines 22 - 30, The loop in test.ps1 restores the working
directory with a fixed one-level ascent, which breaks when setup.ps1 is found at
different nesting depths. Update the iteration around Set-Location and
.\setup.ps1 so it always returns to the original $scriptPath after each
invocation instead of using relative cd..; this keeps each pass independent
regardless of where the matching setup.ps1 was discovered.
| cd basic-commits | ||
| ./setup.sh | ||
| cd .. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add cd failure guards per Shellcheck.
cd basic-commits at Line 6 can fail silently (e.g., missing/renamed directory) and the script would continue executing ./setup.sh from the wrong directory. Shellcheck SC2164 flags this.
🐛 Proposed fix
-cd basic-commits
-./setup.sh
-cd ..
+cd basic-commits || exit 1
+./setup.sh
+cd .. || exit 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.
| cd basic-commits | |
| ./setup.sh | |
| cd .. | |
| cd basic-commits || exit 1 | |
| ./setup.sh | |
| cd .. || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 8-8: Use a ( subshell ) to avoid having to cd back.
(SC2103)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.sh` around lines 6 - 8, The test.sh setup flow does not guard the cd
into basic-commits, so the script can continue in the wrong directory if that
change fails. Update the setup sequence to fail fast on the directory change by
adding an explicit cd failure guard before running ./setup.sh, using the
existing basic-commits cd step as the place to fix this Shellcheck SC2164 issue.
Source: Linters/SAST tools
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files | rg '(^|/)(test\.sh|test\.ps1|setup\.sh|README\.md)$'
printf '\n== test.sh ==\n'
cat -n test.sh
printf '\n== test.ps1 ==\n'
cat -n test.ps1
printf '\n== setup.sh files ==\n'
find . -type f -name setup.sh | sort
printf '\n== mention of "Get all the setup.sh files" ==\n'
rg -n "Get all the setup\.sh files|setup\.sh" -S .Repository: eficode-academy/git-katas
Length of output: 7911
Run all kata setup scripts, not just basic-commits test.sh:6-8 only executes basic-commits/setup.sh, so the rest of the katas' setup.sh files never get exercised. test.ps1 already discovers them recursively, so the shell version should match that behavior.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 6-6: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 8-8: Use a ( subshell ) to avoid having to cd back.
(SC2103)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test.sh` around lines 6 - 8, The shell test script is only running the
basic-commits kata setup, so the other kata setup scripts are skipped. Update
the test.sh flow to mirror test.ps1 by discovering and executing every setup.sh
recursively, using the same overall setup routine but without hardcoding
basic-commits. Reference the existing test.sh execution block and the setup.sh
invocation logic when making the change.
2 participants
Summary by CodeRabbit