Skip to content

Commit cb4a97e

Browse files
author
Lasse Benninga
committed
feat(autograder): add shared grader_lib + code hygiene warnings
Same grader_lib.sh as week-1 — shared bash library with pass/fail/warn helpers and static-analysis checks from c55 PR review analysis. New warning checks for week-2 specific patterns: - check_silent_zero_in_except: AST-based detection of 'x = 0' in except blocks (reviewer c55#3: "sets price/revenue/vat to 0 instead of skipping the row — silently corrupts output") - check_exception_logged: warns when except-block log call doesn't include the exception variable 'e' (reviewer c55#5,#7: "log the error type!") - check_ruff F401/E302: unused imports + missing blank lines between functions (c55#3,#1: "This import isn't used" / "add new line after func") - check_no_print_statements: logging over print() - check_no_notimplemented: stubs left in (c55#3) - check_gitignore_python: __pycache__/ and .env ignored - AI_DEBUG.md traceback check - Screenshot format: 7/20 pts (warn) for .jpg, full 10/20 for .png Scoring ladder unchanged (60/20/20, passing=60).
1 parent 940becd commit cb4a97e

2 files changed

Lines changed: 318 additions & 18 deletions

File tree

.hyf/grader_lib.sh

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
#!/usr/bin/env bash
2+
# grader_lib.sh — shared helpers for HYF Data Track autograders.
3+
# Source this at the top of test.sh:
4+
# source "$(dirname "$0")/grader_lib.sh"
5+
#
6+
# Provides: pass(), fail(), warn(), print_results(), write_score(),
7+
# and a set of common static-analysis checks derived from recurring
8+
# PR review patterns across cohort c55.
9+
10+
_grader_details=()
11+
12+
pass() { _grader_details+=("✓ PASS $1"); }
13+
fail() { _grader_details+=("✗ FAIL $1"); }
14+
warn() { _grader_details+=("⚠ WARN $1"); }
15+
16+
print_results() {
17+
local header="${1:-Autograder Results}"
18+
echo ""
19+
echo "=== $header ==="
20+
for line in "${_grader_details[@]}"; do echo " $line"; done
21+
echo ""
22+
}
23+
24+
write_score() {
25+
# write_score <score> <passing> [<outfile>]
26+
local score="$1"
27+
local passing="$2"
28+
local outfile="${3:-$(dirname "${BASH_SOURCE[0]}")/score.json}"
29+
local pass_flag="false"
30+
[[ "$score" -ge "$passing" ]] && pass_flag="true"
31+
cat > "$outfile" << JSON
32+
{
33+
"score": $score,
34+
"pass": $pass_flag,
35+
"passingScore": $passing
36+
}
37+
JSON
38+
echo "Score: $score / 100 (passing: $passing) pass=$pass_flag"
39+
}
40+
41+
# ── Common static-analysis checks ────────────────────────────────────────────
42+
# Each function: returns 0 on pass, 1 on fail/warn (for caller logic).
43+
# All feedback goes through pass()/fail()/warn() so it appears in print_results.
44+
45+
check_no_print_statements() {
46+
# Usage: check_no_print_statements <dir> [label]
47+
# Flags bare print() calls that should be logging calls.
48+
local dir="${1:-.}"
49+
local label="${2:-$dir}"
50+
local found
51+
found=$(grep -rn "^[[:space:]]*print(" "$dir" --include="*.py" 2>/dev/null | grep -v "# noqa" || true)
52+
if [[ -n "$found" ]]; then
53+
local count
54+
count=$(echo "$found" | wc -l | tr -d ' ')
55+
warn "$label: $count print() call(s) found — use logging.info/warning/error instead (see Week 1 Ch1)"
56+
return 1
57+
fi
58+
return 0
59+
}
60+
61+
check_no_notimplemented() {
62+
# Usage: check_no_notimplemented <dir> [label]
63+
# Flags NotImplementedError stubs left in after implementation.
64+
local dir="${1:-.}"
65+
local label="${2:-$dir}"
66+
local found
67+
found=$(grep -rn "raise NotImplementedError" "$dir" --include="*.py" 2>/dev/null || true)
68+
if [[ -n "$found" ]]; then
69+
fail "$label: raise NotImplementedError still present — remove stubs before submitting"
70+
return 1
71+
fi
72+
return 0
73+
}
74+
75+
check_no_relative_imports() {
76+
# Usage: check_no_relative_imports <dir> [label]
77+
# Flags `from .module import x` in scripts not inside a proper package.
78+
# Relative imports break the grader: python3 src/cleaner.py fails with
79+
# "attempted relative import with no known parent package".
80+
local dir="${1:-.}"
81+
local label="${2:-$dir}"
82+
local found
83+
found=$(grep -rn "^from \." "$dir" --include="*.py" 2>/dev/null || true)
84+
if [[ -n "$found" ]]; then
85+
fail "$label: relative import found (from .module) — use absolute: 'from src.module import x'"
86+
return 1
87+
fi
88+
return 0
89+
}
90+
91+
check_no_logging_in_utils() {
92+
# Usage: check_no_logging_in_utils <utils_file>
93+
# utils.py should be pure helpers; logging config belongs in the entry point.
94+
local file="${1:-task-1/src/utils.py}"
95+
if [[ ! -f "$file" ]]; then return 0; fi
96+
if grep -qE "logging\.basicConfig|logging\.getLogger" "$file"; then
97+
warn "$file: logging.basicConfig/getLogger found — logging setup belongs in cleaner.py or the entry-point, not in utils"
98+
return 1
99+
fi
100+
return 0
101+
}
102+
103+
check_gitignore_python() {
104+
# Usage: check_gitignore_python [<gitignore_path>]
105+
# Warns when Python cache patterns are absent from .gitignore.
106+
local gi="${1:-.gitignore}"
107+
if [[ ! -f "$gi" ]]; then
108+
warn ".gitignore is missing — add one so __pycache__/ and *.pyc are not committed"
109+
return 1
110+
fi
111+
local ok=true
112+
if ! grep -q "__pycache__" "$gi"; then
113+
warn ".gitignore missing __pycache__/ — Python bytecode cache dirs should not be committed"
114+
ok=false
115+
fi
116+
if ! grep -qE "^\*\.pyc$|^.*\*\.pyc" "$gi"; then
117+
warn ".gitignore missing *.pyc — compiled Python files should not be committed"
118+
ok=false
119+
fi
120+
if ! grep -qE "^\.env$|^\.env\b" "$gi"; then
121+
warn ".gitignore missing .env — secret files should not be committed"
122+
ok=false
123+
fi
124+
[[ "$ok" = true ]] && pass ".gitignore correctly excludes __pycache__/, *.pyc, and .env"
125+
}
126+
127+
check_screenshot_is_png() {
128+
# Usage: check_screenshot_is_png <expected_path> [<wrong_ext_glob>]
129+
# Awards full credit for .png, warns (and still credits) for .jpg/.jpeg,
130+
# zero for missing. Matches the pattern flagged in c55 PR reviews.
131+
local expected_png="$1"
132+
local dir
133+
dir="$(dirname "$expected_png")"
134+
local base
135+
base="$(basename "$expected_png" .png)"
136+
137+
if [[ -s "$expected_png" ]]; then
138+
pass "screenshot is $expected_png (.png format ✓)"
139+
return 0
140+
fi
141+
for ext in jpg jpeg; do
142+
if [[ -s "$dir/$base.$ext" ]]; then
143+
warn "screenshot is .$ext but should be .png — rename to $base.png (partial credit still given)"
144+
return 1
145+
fi
146+
done
147+
fail "screenshot missing: $expected_png not found"
148+
return 2
149+
}
150+
151+
check_silent_zero_in_except() {
152+
# Usage: check_silent_zero_in_except <file>
153+
# Detects the pattern: try: x = compute() / except: x = 0
154+
# which silently corrupts data instead of skipping or raising.
155+
local file="$1"
156+
if [[ ! -f "$file" ]]; then return 0; fi
157+
local found
158+
found=$(python3 - "$file" 2>/dev/null << 'PY'
159+
import ast, sys
160+
try:
161+
tree = ast.parse(open(sys.argv[1]).read())
162+
except SyntaxError:
163+
sys.exit(0)
164+
for node in ast.walk(tree):
165+
if isinstance(node, ast.ExceptHandler):
166+
for stmt in node.body:
167+
if isinstance(stmt, ast.Assign):
168+
if isinstance(stmt.value, ast.Constant) and stmt.value.value == 0:
169+
print(f"line {stmt.lineno}: '{ast.unparse(stmt)}' — sets field to 0 in except block (silent data corruption)")
170+
PY
171+
)
172+
if [[ -n "$found" ]]; then
173+
warn "$file: silent 0-assignment in except block — skip the row or raise instead of setting to 0:\n $found"
174+
return 1
175+
fi
176+
return 0
177+
}
178+
179+
check_exception_logged() {
180+
# Usage: check_exception_logged <dir>
181+
# Warns when except blocks log/print a message but don't include the
182+
# exception variable (e, err, exc), meaning the error type is lost.
183+
local dir="${1:-.}"
184+
local found
185+
found=$(python3 - "$dir" 2>/dev/null << 'PY'
186+
import ast, os, sys
187+
issues = []
188+
for root, _, files in os.walk(sys.argv[1]):
189+
for fname in files:
190+
if not fname.endswith(".py"):
191+
continue
192+
path = os.path.join(root, fname)
193+
try:
194+
tree = ast.parse(open(path).read())
195+
except SyntaxError:
196+
continue
197+
for node in ast.walk(tree):
198+
if not isinstance(node, ast.ExceptHandler):
199+
continue
200+
exc_var = node.name # e.g. "e" in `except ValueError as e`
201+
if not exc_var:
202+
continue
203+
for stmt in node.body:
204+
for call in ast.walk(stmt):
205+
if not isinstance(call, ast.Call):
206+
continue
207+
# Is it a logging.* or print call?
208+
func = call.func
209+
is_log = (isinstance(func, ast.Attribute) and
210+
isinstance(func.value, ast.Name) and
211+
func.value.id == "logging")
212+
is_print = isinstance(func, ast.Name) and func.id == "print"
213+
if not (is_log or is_print):
214+
continue
215+
# Does the call reference the exception variable?
216+
src = ast.unparse(call)
217+
if exc_var not in src:
218+
issues.append(f"{path}:{call.lineno}: log message doesn't include exception variable '{exc_var}' — add it for easier debugging")
219+
if issues:
220+
for i in issues[:3]: # cap at 3 to keep output readable
221+
print(i)
222+
PY
223+
)
224+
if [[ -n "$found" ]]; then
225+
warn "exception variable not included in log message (harder to debug):\n $found"
226+
return 1
227+
fi
228+
return 0
229+
}
230+
231+
check_ruff() {
232+
# Usage: check_ruff <dir> [<select>]
233+
# Runs ruff on <dir> if available; warns on violations.
234+
# Default select: F401 (unused imports), E302 (missing blank lines).
235+
local dir="${1:-.}"
236+
local select="${2:-F401,E302,E303}"
237+
if ! command -v ruff &>/dev/null && ! python3 -m ruff --version &>/dev/null 2>&1; then
238+
return 0 # ruff not installed — skip silently
239+
fi
240+
local out
241+
out=$(python3 -m ruff check --select="$select" --output-format=text "$dir" 2>/dev/null || true)
242+
if [[ -n "$out" ]]; then
243+
local count
244+
count=$(echo "$out" | grep -c "\.py:" || true)
245+
warn "$dir: ruff found $count style issue(s) (unused imports / missing blank lines) — run 'ruff check $dir' to see details"
246+
return 1
247+
fi
248+
pass "$dir: no ruff style issues (F401/E302/E303)"
249+
return 0
250+
}

.hyf/test.sh

Lines changed: 68 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
1111
REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
1212
cd "$REPO_ROOT"
1313

14+
# shellcheck source=.hyf/grader_lib.sh
15+
source "$SCRIPT_DIR/grader_lib.sh"
16+
1417
PASSING=60
1518

1619
# --- Task 1: Cleaner Pipeline (60 points) ---
@@ -186,40 +189,87 @@ fi
186189
# Screenshot is required (10 pts); blob_url.txt with a valid Azure Storage
187190
# URL earns the remaining 10 pts. Both checks live inside the screenshot
188191
# branch — no screenshot means 0/20 regardless of blob_url.txt.
192+
# Full credit for .png (the required format). Partial credit + warning for
193+
# .jpg/.jpeg (wrong extension — matches c55 review pattern).
189194
task3=0
190195
task3_msg="missing task-3/assets/azure_blob_week2.png (or .jpg/.jpeg)"
196+
screenshot_found=false
191197
for ext in png jpg jpeg; do
192198
if [ -s "task-3/assets/azure_blob_week2.$ext" ]; then
193-
task3=10
199+
screenshot_found=true
200+
if [ "$ext" != "png" ]; then
201+
warn "Task 3: screenshot is .$ext but the assignment requires .png — rename to azure_blob_week2.png"
202+
task3=7 # partial credit for wrong format
203+
task3_msg="azure_blob_week2.$ext present but wrong format — rename to .png"
204+
else
205+
task3=10
206+
task3_msg="azure_blob_week2.png present"
207+
fi
194208
if [ -s "task-3/assets/blob_url.txt" ]; then
195209
# Require at least <container>/<blob> after the host so a bare
196210
# storage-account root URL doesn't satisfy the check.
197211
if grep -qE "https://[a-z0-9]+\.blob\.core\.windows\.net/[^/]+/[^/]+" task-3/assets/blob_url.txt; then
198-
task3=20
199-
task3_msg="screenshot and blob URL both present"
212+
task3=$((task3 + 10))
213+
task3_msg="${task3_msg}; blob_url.txt valid"
200214
else
201-
task3_msg="blob_url.txt present but URL format is wrong — expected https://<account>.blob.core.windows.net/<container>/<blob>"
215+
task3_msg="${task3_msg}; blob_url.txt present but URL format is wrong — expected https://<account>.blob.core.windows.net/<container>/<blob>"
202216
fi
203217
else
204-
task3_msg="screenshot present but task-3/assets/blob_url.txt is missing"
218+
task3_msg="${task3_msg}; task-3/assets/blob_url.txt is missing"
205219
fi
206220
break
207221
fi
208222
done
209223

210224
score=$((task1 + task2 + task3))
211-
if [ "$score" -ge "$PASSING" ]; then pass=true; else pass=false; fi
212-
213-
cat > "$SCRIPT_DIR/score.json" <<EOF
214-
{
215-
"score": $score,
216-
"pass": $pass,
217-
"passingScore": $PASSING
218-
}
219-
EOF
220-
221-
echo "Task 1 (Cleaner Pipeline): $task1/60 — $task1_msg"
222-
echo "Task 2 (AI Debug Report): $task2/20 — $task2_msg"
225+
226+
# ── Code hygiene warnings (0 pts — informational only) ──────────────────────
227+
# These checks mirror recurring review comments from cohort c55. They do not
228+
# affect the score but surface issues the teacher would otherwise flag manually.
229+
echo ""
230+
echo "--- Code Hygiene (warnings only, do not affect score) ---"
231+
232+
# print() should be replaced with logging.* — chapter mandates logging.
233+
check_no_print_statements "task-1/src" "task-1/src"
234+
235+
# NotImplementedError stubs left in after implementation.
236+
# Reviewer comment: "please remove the NotImplementedError from your helpers
237+
# since you've already added the functionality".
238+
check_no_notimplemented "task-1/src" "task-1/src"
239+
240+
# Silent 0-assignment in except blocks corrupts data silently.
241+
# Reviewer comment: "sets price/revenue/vat to 0 instead of skipping the row,
242+
# a bad price would silently corrupt the output rather than being dropped".
243+
check_silent_zero_in_except "task-1/src/transforms.py"
244+
245+
# Exception variable not included in log message loses error context.
246+
# Reviewer comment: "I would log the error type for easier debug!"
247+
check_exception_logged "task-1/src"
248+
249+
# Unused imports and missing blank lines — caught by ruff F401/E302.
250+
# Reviewer comments: "This import isn't used anywhere" and
251+
# "need to add new line after the end of each function".
252+
check_ruff "task-1/src" "F401,E302,E303"
253+
254+
# AI_DEBUG.md should include the full traceback.
255+
# Reviewer comment: "Would be good if the full traceback error was pasted here".
256+
if [ -s task-2/AI_DEBUG.md ]; then
257+
if ! grep -q "Traceback" task-2/AI_DEBUG.md; then
258+
warn "AI_DEBUG.md: no 'Traceback' found — paste the full error traceback in the '## The Error' section"
259+
fi
260+
fi
261+
262+
# .gitignore should exclude Python cache files.
263+
check_gitignore_python ".gitignore"
264+
265+
print_results "Week 2 Autograder"
266+
267+
# ── Final score ──────────────────────────────────────────────────────────────
268+
write_score "$score" "$PASSING" "$SCRIPT_DIR/score.json"
269+
270+
echo ""
271+
echo "Task 1 (Cleaner Pipeline): $task1/60 — $task1_msg"
272+
echo "Task 2 (AI Debug Report): $task2/20 — $task2_msg"
223273
echo "Task 3 (Azure Blob Upload): $task3/20 — $task3_msg"
224274
echo "----------------------------------------"
225-
echo "Total: $score/100 — pass=$pass (passing threshold: $PASSING)"
275+
echo "Total: $score/100 — (passing threshold: $PASSING)"

0 commit comments

Comments
 (0)