Skip to content

Commit b1808da

Browse files
committed
Address code review comments
- Fix script name typo: runtests.sh -> runtest.sh - Fix missing $ in color variable: {noColor} -> ${noColor} - Fix race condition: use atomic file operations (temp file + mv) for marker file - Add kernel validation: validate kernel exists before running notebook tests - Add numeric validation for marker file values to handle corruption
1 parent d572afc commit b1808da

File tree

1 file changed

+35
-11
lines changed

1 file changed

+35
-11
lines changed

runtest.sh

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ function install_deps {
3838
python3 -m pip install -e .[dev]
3939
fi;
4040
echo "dependencies installed"
41-
# Reset the marker after successful install (timestamp and run count)
42-
echo "$(date +%s)" > "$DEPS_MARKER"
43-
echo "0" >> "$DEPS_MARKER"
41+
# Reset the marker after successful install (atomic write)
42+
local tmp_marker="${DEPS_MARKER}.tmp.$$"
43+
printf "%s\n%s\n" "$(date +%s)" "0" > "$tmp_marker"
44+
mv "$tmp_marker" "$DEPS_MARKER"
4445
}
4546

4647
function should_install_deps {
@@ -55,8 +56,12 @@ function should_install_deps {
5556

5657
# Read marker file (line 1 = timestamp, line 2 = run count)
5758
local install_timestamp run_count
58-
install_timestamp=$(sed -n '1p' "$DEPS_MARKER" 2>/dev/null || echo "0")
59-
run_count=$(sed -n '2p' "$DEPS_MARKER" 2>/dev/null || echo "0")
59+
install_timestamp=$(sed -n '1p' "$DEPS_MARKER" 2>/dev/null)
60+
run_count=$(sed -n '2p' "$DEPS_MARKER" 2>/dev/null)
61+
62+
# Validate numeric values, default to 0 if empty or malformed
63+
[[ ! "$install_timestamp" =~ ^[0-9]+$ ]] && install_timestamp=0
64+
[[ ! "$run_count" =~ ^[0-9]+$ ]] && run_count=0
6065

6166
# Check age (days since last install)
6267
local now marker_age_days
@@ -74,10 +79,11 @@ function should_install_deps {
7479
return 0
7580
fi
7681

77-
# Deps are cached and valid - increment run count (preserve timestamp)
82+
# Deps are cached and valid - increment run count (atomic write)
7883
echo "Dependencies cached (${run_count}/${DEPS_MAX_RUNS} runs, ${marker_age_days}/${DEPS_MAX_AGE_DAYS} days) - skipping install"
79-
echo "$install_timestamp" > "$DEPS_MARKER"
80-
echo $((run_count + 1)) >> "$DEPS_MARKER"
84+
local tmp_marker="${DEPS_MARKER}.tmp.$$"
85+
printf "%s\n%s\n" "$install_timestamp" "$((run_count + 1))" > "$tmp_marker"
86+
mv "$tmp_marker" "$DEPS_MARKER"
8187
return 1
8288
}
8389

@@ -124,7 +130,7 @@ function print_error_msg() {
124130

125131
function print_style_fail_msg() {
126132
echo "${red}Check failed!${noColor}"
127-
echo "Please run auto style fixes: ${green}./runtests.sh -f {noColor}"
133+
echo "Please run auto style fixes: ${green}./runtest.sh -f${noColor}"
128134
}
129135
function report_status() {
130136
status="$1"
@@ -258,11 +264,29 @@ function get_current_kernel() {
258264
echo ""
259265
}
260266

267+
function validate_kernel() {
268+
local kernel_name="$1"
269+
if ! command -v jupyter >/dev/null 2>&1; then
270+
echo "${red}Error: jupyter not found. Install with: pip install jupyter${noColor}"
271+
return 1
272+
fi
273+
if ! jupyter kernelspec list 2>/dev/null | awk '{print $1}' | grep -qx "$kernel_name"; then
274+
echo "${red}Error: kernel '$kernel_name' not found${noColor}"
275+
echo "Available kernels:"
276+
jupyter kernelspec list 2>/dev/null | tail -n +2
277+
return 1
278+
fi
279+
return 0
280+
}
281+
261282
function notebook_test() {
262283
echo "${separator}${blue}notebook-test${noColor}"
263284

264285
local kernel_opt=""
265286
if [[ -n "$nb_kernel" ]]; then
287+
if ! validate_kernel "$nb_kernel"; then
288+
exit 1
289+
fi
266290
kernel_opt="--kernel=$nb_kernel"
267291
echo "Using kernel: $nb_kernel"
268292
else
@@ -293,7 +317,7 @@ export PYTHONPATH=${WORK_DIR}:${PYTHONPATH} && echo "PYTHONPATH is ${PYTHONPATH}
293317
function help() {
294318
echo "Add description of the script functions here."
295319
echo
296-
echo "Syntax: runtests.sh [-h|--help]
320+
echo "Syntax: runtest.sh [-h|--help]
297321
[-l|--check-license]
298322
[-s|--check-format]
299323
[-f|--fix-format]
@@ -323,7 +347,7 @@ function help() {
323347
echo ""
324348
echo "Notebook test options (used with -n):"
325349
echo " --timeout=SECONDS : timeout per notebook (default: 1200)"
326-
echo " --kernel=NAME : Jupyter kernel name (default: current venv/conda env)"
350+
echo " --kernel=NAME : Jupyter kernel name (must be valid, see: jupyter kernelspec list)"
327351
echo " --nb-clean=MODE : clean outputs: always, on-success, never (default: on-success)"
328352
echo ""
329353
echo "Examples:"

0 commit comments

Comments
 (0)