Skip to content

fix(commit0): evaluation harness fails to score cachetools and parsel due to test command issues #539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
juanmichelini merged 4 commits into from
Mar 20, 2026
Merged

fix(commit0): evaluation harness fails to score cachetools and parsel due to test command issues #539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68feee6
Your message
VascoSch92 Mar 18, 2026
c9ae51f
Address PR review: tighten pytest detection, fix PYTHONPATH, add tests
VascoSch92 Mar 19, 2026
7652655
fix: handle pytest with extra flags in normalize_pytest_cmd
VascoSch92 Mar 19, 2026
1cc53f5
fix: read src_dir from instance.data, not instance.data["test"]
VascoSch92 Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions benchmarks/commit0/run_infer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json
import os
import re
import shlex
from typing import Any, List

Expand Down Expand Up @@ -62,6 +63,23 @@
""".strip()


def normalize_pytest_cmd(test_cmd: str) -> str:
"""Replace bare pytest/pytest3 with python -m pytest to avoid PATH/permission issues."""
if (
re.match(r"pytest\d?(\s|$)", test_cmd.strip())
and "python -m pytest" not in test_cmd
):
test_cmd = re.sub(r"\bpytest(\d?)", r"python -m pytest\1", test_cmd, count=1)
Copy link
Collaborator

@all-hands-bot all-hands-bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Logic redundancy - lacks "good taste"

You check if the stripped command is exactly pytest or pytest3 with re.match(r"pytest\d?$", ...), but then use word boundaries \b in the substitution. If you've already verified it's exactly "pytest", word boundaries are redundant.

Simpler approach:

def normalize_pytest_cmd(test_cmd: str) -> str:
    """Replace bare pytest/pytest3 with python -m pytest to avoid PATH/permission issues."""
    stripped = test_cmd.strip()
    if stripped in ("pytest", "pytest3") and "python -m pytest" not in test_cmd:
        return test_cmd.replace(stripped, f"python -m {stripped}", 1)
    return test_cmd

Or if you need the regex flexibility:

if re.match(r"^pytest\d?$", test_cmd.strip()):
    return re.sub(r"^pytest(\d?)$", r"python -m pytest\1", test_cmd.strip())
return test_cmd

Current code works but mixes two different validation strategies unnecessarily.

return test_cmd


def get_pythonpath_prefix(src_dir: str) -> str:
"""Return PYTHONPATH env prefix for src-layout repos."""
if src_dir and src_dir.startswith("src"):
return "PYTHONPATH=src:$PYTHONPATH "
Comment on lines +76 to +79
Copy link
Collaborator

@all-hands-bot all-hands-bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Misleading function signature

The function takes src_dir as a parameter but ignores its actual value and returns a hardcoded "PYTHONPATH=src:$PYTHONPATH ". This makes it look like the function would use different paths based on src_dir, but it only uses it as a boolean check.

Clearer alternatives:

  1. Inline it (simplest):
env_prefix = "PYTHONPATH=src:$PYTHONPATH " if src_dir and src_dir.startswith("src") else ""
  1. Rename to reflect actual behavior:
def is_src_layout(src_dir: str) -> bool:
    """Check if repo uses src-layout requiring PYTHONPATH."""
    return bool(src_dir and src_dir.startswith("src"))

# Then:
env_prefix = "PYTHONPATH=src:$PYTHONPATH " if is_src_layout(src_dir) else ""

The current name get_pythonpath_prefix suggests it derives a path from the parameter, but it doesn't.

return ""


def parse_report_summary(raw_json: str) -> dict:
"""Parse pytest-json-report summary extracted from the container.

Expand Down Expand Up @@ -440,10 +458,10 @@ def evaluate_instance(
# Run tests
test_cmd = instance.data["test"]["test_cmd"]
test_dir = instance.data["test"]["test_dir"]
# Use python -m pytest instead of pytest command to avoid permission issues
if test_cmd.strip() == "pytest":
test_cmd = "python -m pytest"
full_test_cmd = f"cd {repo_path} && {test_cmd} --json-report --json-report-file=report.json --continue-on-collection-errors {test_dir} > test_output.txt 2>&1"
test_cmd = normalize_pytest_cmd(test_cmd)
src_dir = instance.data.get("src_dir", "")
env_prefix = get_pythonpath_prefix(src_dir)
full_test_cmd = f"cd {repo_path} && {env_prefix}{test_cmd} --json-report --json-report-file=report.json --continue-on-collection-errors {test_dir} > test_output.txt 2>&1"
logger.info(f"Running test command: {full_test_cmd}")
test_result = workspace.execute_command(full_test_cmd, timeout=600)
logger.info(f"Test command exit code: {test_result.exit_code}")
Expand Down
54 changes: 54 additions & 0 deletions tests/test_commit0_run_infer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
"""Tests for commit0 run_infer test command helpers."""

import pytest

from benchmarks.commit0.run_infer import get_pythonpath_prefix, normalize_pytest_cmd


@pytest.mark.parametrize(
"input_cmd, expected",
[
("pytest", "python -m pytest"),
("pytest3", "python -m pytest3"),
("python -m pytest", "python -m pytest"),
("mypytest", "mypytest"),
("pytest-xdist", "pytest-xdist"),
("pytest_runner", "pytest_runner"),
(
"pytest --assert=plain --ignore=setup.py",
"python -m pytest --assert=plain --ignore=setup.py",
),
],
ids=[
"bare_pytest",
"bare_pytest3",
"already_module_form",
"substring_mypytest",
"substring_pytest-xdist",
"substring_pytest_runner",
"real-parsel-scenario",
],
)
def test_normalize_pytest_cmd(input_cmd, expected):
assert normalize_pytest_cmd(input_cmd) == expected


@pytest.mark.parametrize(
"src_dir, expected",
[
("src/cachetools", "PYTHONPATH=src:$PYTHONPATH "),
("src", "PYTHONPATH=src:$PYTHONPATH "),
("", ""),
("lib/mypackage", ""),
("tests/src/data", ""),
],
ids=[
"src_layout",
"bare_src",
"empty_string",
"no_src_dir",
"src_not_at_start",
],
)
def test_get_pythonpath_prefix(src_dir, expected):
assert get_pythonpath_prefix(src_dir) == expected
Loading