Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions src/excelbench/harness/adapters/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,15 +316,15 @@ def read_named_ranges(self, workbook: Any, sheet: str) -> list[JSONDict]:
- refers_to: reference formula (e.g. Sheet1!$A$1)
"""

return []
raise NotImplementedError(f"{self.name} does not implement named range reads")

def add_named_range(self, workbook: Any, sheet: str, named_range: JSONDict) -> None:
"""Add a named range.

named_range should include keys: name, scope, refers_to.
"""

return None
raise NotImplementedError(f"{self.name} does not implement named range writes")

def read_tables(self, workbook: Any, sheet: str) -> list[JSONDict]:
"""Read table (ListObject) definitions from a sheet.
Expand All @@ -339,15 +339,15 @@ def read_tables(self, workbook: Any, sheet: str) -> list[JSONDict]:
- autofilter: bool (optional)
"""

return []
raise NotImplementedError(f"{self.name} does not implement table reads")

def add_table(self, workbook: Any, sheet: str, table: JSONDict) -> None:
"""Add a table (ListObject) to a sheet.

table dict should include keys: name, ref, style, columns, header_row, totals_row.
"""

return None
raise NotImplementedError(f"{self.name} does not implement table writes")

# =========================================================================
# Write Operations
Expand Down
19 changes: 19 additions & 0 deletions src/excelbench/harness/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,23 @@
JSONDict = dict[str, Any]


def _failure_note_from_actual(actual: JSONDict) -> str:
if "error" in actual:
error_text = str(actual.get("error", "")).lower()
unsupported_markers = (
"notimplemented",
"not implemented",
"unsupported",
"not supported",
"read-only",
"write-only",
)
if any(marker in error_text for marker in unsupported_markers):
return "Not implemented"
return "Incorrect result"
return "Incorrect result"
Comment on lines +40 to +54

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_failure_note_from_actual bypassed for Tier 3 exceptions

When Tier 3 base methods (read_named_ranges, add_named_range, read_tables, add_table) raise NotImplementedError, the exception propagates up through the read_*_actual helper and is caught by the generic except Exception as e handler at line 532, which sets notes=f"Exception: {type(e).__name__}" — i.e., "Exception: NotImplementedError".

This means _failure_note_from_actual is never invoked for these cases, and the note will be "Exception: NotImplementedError" rather than "Not implemented". The function is only reached when the adapter call succeeds but returns a dict containing an "error" key.

This may be intentional (the exception handler note is arguably more descriptive), but it's worth confirming this is the desired behavior — since the PR description mentions wanting to "clearly distinguish" not-implemented from incorrect results, you may want the exception handler to also produce a "Not implemented" note for NotImplementedError specifically:

    except NotImplementedError as e:
        return TestResult(
            ...
            notes="Not implemented",
            ...
        )
    except Exception as e:
        ...
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/excelbench/harness/runner.py
Line: 40:54

Comment:
**`_failure_note_from_actual` bypassed for Tier 3 exceptions**

When Tier 3 base methods (`read_named_ranges`, `add_named_range`, `read_tables`, `add_table`) raise `NotImplementedError`, the exception propagates up through the `read_*_actual` helper and is caught by the generic `except Exception as e` handler at line 532, which sets `notes=f"Exception: {type(e).__name__}"` — i.e., `"Exception: NotImplementedError"`.

This means `_failure_note_from_actual` is never invoked for these cases, and the note will be `"Exception: NotImplementedError"` rather than `"Not implemented"`. The function is only reached when the adapter call succeeds but returns a dict containing an `"error"` key.

This may be intentional (the exception handler note is arguably more descriptive), but it's worth confirming this is the desired behavior — since the PR description mentions wanting to "clearly distinguish" not-implemented from incorrect results, you may want the exception handler to also produce a `"Not implemented"` note for `NotImplementedError` specifically:
```
    except NotImplementedError as e:
        return TestResult(
            ...
            notes="Not implemented",
            ...
        )
    except Exception as e:
        ...
```

How can I resolve this? If you propose a fix, please make it concise.



def _build_exception_diagnostic(
adapter: ExcelAdapter,
*,
Expand Down Expand Up @@ -417,6 +434,7 @@ def test_read_case(
passed=passed,
expected=expected,
actual=actual,
notes=None if passed else _failure_note_from_actual(actual),
diagnostics=(
[]
if passed
Expand Down Expand Up @@ -492,6 +510,7 @@ def test_read_case(
passed=passed,
expected=expected,
actual=actual,
notes=None if passed else _failure_note_from_actual(actual),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Map NotImplemented failures in read-case exception path

The new note mapping only runs in the non-exception return path, so capability failures raised by the new Tier-3 defaults (read_named_ranges/read_tables now raise NotImplementedError) still land in the generic exception handler and get notes="Exception: NotImplementedError" instead of "Not implemented". That means the change still does not consistently distinguish unsupported features from incorrect results in emitted test results.

Useful? React with 👍 / 👎.

diagnostics=(
[]
if passed
Expand Down
98 changes: 98 additions & 0 deletions src/excelbench/results/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ def render_results(results: BenchmarkResults, output_dir: Path) -> None:
render_markdown(results, output_dir / "README.md")
render_csv(results, output_dir / "matrix.csv")
_append_history(results, output_dir)
_render_fidelity_deltas(output_dir)


def render_json(results: BenchmarkResults, path: Path) -> None:
Expand Down Expand Up @@ -719,6 +720,103 @@ def _append_history(results: BenchmarkResults, output_dir: Path) -> None:
f.write(json.dumps(entry) + "\n")


def _render_fidelity_deltas(output_dir: Path) -> None:
"""Render a markdown report comparing the two most recent fidelity runs."""
history_path = output_dir / "history.jsonl"
out_path = output_dir / "FIDELITY_DELTAS.md"
if not history_path.exists():
out_path.write_text("# Fidelity Deltas\n\nNo history available.\n")
return

entries: list[dict[str, Any]] = []
for line in history_path.read_text().splitlines():
line = line.strip()
if not line:
continue
try:
parsed = json.loads(line)
except json.JSONDecodeError:
continue
if isinstance(parsed, dict):
entries.append(parsed)

if len(entries) < 2:
out_path.write_text("# Fidelity Deltas\n\nNeed at least two runs in history.jsonl.\n")
return

previous = entries[-2]
current = entries[-1]
Comment on lines +731 to +748

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To improve memory efficiency, especially if history.jsonl becomes very large, it's better to read the file line-by-line and only keep track of the last two entries. This avoids loading the entire history file into memory.

Suggested change
entries: list[dict[str, Any]] = []
for line in history_path.read_text().splitlines():
line = line.strip()
if not line:
continue
try:
parsed = json.loads(line)
except json.JSONDecodeError:
continue
if isinstance(parsed, dict):
entries.append(parsed)
if len(entries) < 2:
out_path.write_text("# Fidelity Deltas\n\nNeed at least two runs in history.jsonl.\n")
return
previous = entries[-2]
current = entries[-1]
entries: list[dict[str, Any]] = []
with history_path.open("r", encoding="utf-8") as f:
for line in f:
line = line.strip()
if not line:
continue
try:
parsed = json.loads(line)
except json.JSONDecodeError:
continue
if isinstance(parsed, dict):
entries.append(parsed)
if len(entries) > 2:
entries.pop(0)
if len(entries) < 2:
out_path.write_text("# Fidelity Deltas\n\nNeed at least two runs in history.jsonl.\n")
return
previous, current = entries

deltas = _compute_fidelity_deltas(previous, current)
Comment on lines +747 to +749

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Restrict fidelity deltas to comparable run profiles

_render_fidelity_deltas always compares the last two history entries, but it does not check their profile fields before computing deltas. I verified excelbench benchmark allows writing both xlsx and xls runs to the same output directory (default results), so a normal profile switch can produce regression/improvement rows that are just format differences rather than real fidelity changes; this makes the new report unreliable for tracking regressions.

Useful? React with 👍 / 👎.


lines: list[str] = ["# Fidelity Deltas", ""]
lines.append(f"- Previous run: `{previous.get('run_date', 'unknown')}`")
lines.append(f"- Current run: `{current.get('run_date', 'unknown')}`")
lines.append("")

if not deltas:
lines.append("No score changes detected.")
lines.append("")
out_path.write_text("\n".join(lines))
return

regressions = [d for d in deltas if d["delta"] < 0]
improvements = [d for d in deltas if d["delta"] > 0]

lines.append("## Summary")
lines.append("")
lines.append(f"- Regressions: **{len(regressions)}**")
lines.append(f"- Improvements: **{len(improvements)}**")
lines.append(f"- Net score change: **{sum(d['delta'] for d in deltas):+d}**")
lines.append("")

lines.append("## Changed Scores")
lines.append("")
lines.append("| Library | Feature | Mode | Previous | Current | Δ |")
lines.append("|---------|---------|------|----------|---------|---|")
for item in sorted(deltas, key=lambda d: (d["delta"], d["library"], d["feature"], d["mode"])):
lines.append(
f"| {item['library']} | {item['feature']} | {item['mode']} | "
f"{item['previous']} | {item['current']} | {item['delta']:+d} |"
)
lines.append("")

out_path.write_text("\n".join(lines))


def _compute_fidelity_deltas(
previous: dict[str, Any], current: dict[str, Any]
) -> list[dict[str, Any]]:
"""Compute score deltas between two history entries."""
deltas: list[dict[str, Any]] = []
prev_scores: dict[str, Any] = previous.get("scores", {})
curr_scores: dict[str, Any] = current.get("scores", {})

for library in sorted(set(prev_scores) | set(curr_scores)):
prev_lib = prev_scores.get(library, {})
curr_lib = curr_scores.get(library, {})
for feature in sorted(set(prev_lib) | set(curr_lib)):
prev_feature = prev_lib.get(feature, {})
curr_feature = curr_lib.get(feature, {})
for mode in ("read", "write"):
prev_value = prev_feature.get(mode)
curr_value = curr_feature.get(mode)
if prev_value is None or curr_value is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

New features/modes silently ignored in deltas

When one run has a library/feature/mode entry and the other doesn't (i.e., prev_value is None or curr_value is None), the delta is silently skipped. This means if a library adds support for a new feature between runs (going from no score to a score), or drops one entirely, it won't appear in the delta report. Depending on intent, this could mask meaningful regressions or improvements — particularly if a library is newly added or removed between runs.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/excelbench/results/renderer.py
Line: 803:803

Comment:
**New features/modes silently ignored in deltas**

When one run has a library/feature/mode entry and the other doesn't (i.e., `prev_value is None or curr_value is None`), the delta is silently skipped. This means if a library adds support for a new feature between runs (going from no score to a score), or drops one entirely, it won't appear in the delta report. Depending on intent, this could mask meaningful regressions or improvements — particularly if a library is newly added or removed between runs.

How can I resolve this? If you propose a fix, please make it concise.

continue
if prev_value == curr_value:
continue
deltas.append(
{
"library": library,
"feature": feature,
"mode": mode,
"previous": int(prev_value),
"current": int(curr_value),
"delta": int(curr_value) - int(prev_value),
}
)
return deltas


def _diagnostic_to_json(diagnostic: Diagnostic) -> dict[str, Any]:
return {
"category": diagnostic.category.value,
Expand Down
12 changes: 12 additions & 0 deletions tests/test_adapter_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,3 +332,15 @@ def test_build_mismatch_diagnostic() -> None:
)
assert diag.category == DiagnosticCategory.DATA_MISMATCH
assert "expected" in diag.adapter_message


def test_tier3_defaults_raise_not_implemented() -> None:
adapter = ConcreteReadOnly()
with pytest.raises(NotImplementedError, match="named range reads"):
adapter.read_named_ranges(None, "S")
with pytest.raises(NotImplementedError, match="named range writes"):
adapter.add_named_range(None, "S", {})
with pytest.raises(NotImplementedError, match="table reads"):
adapter.read_tables(None, "S")
with pytest.raises(NotImplementedError, match="table writes"):
adapter.add_table(None, "S", {})
10 changes: 6 additions & 4 deletions tests/test_named_ranges.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
class TestNamedRangesBase:
"""Base adapter API surface for named ranges."""

def test_read_named_ranges_default_returns_empty(self) -> None:
def test_read_named_ranges_default_raises(self) -> None:
adapter = StubExcelAdapter()
assert adapter.read_named_ranges(object(), "S1") == []
with pytest.raises(NotImplementedError, match="named range reads"):
adapter.read_named_ranges(object(), "S1")

def test_add_named_range_default_is_noop(self) -> None:
def test_add_named_range_default_raises(self) -> None:
adapter = StubExcelAdapter()
adapter.add_named_range(object(), "S1", {"name": "X", "refers_to": "S1!$A$1"})
with pytest.raises(NotImplementedError, match="named range writes"):
adapter.add_named_range(object(), "S1", {"name": "X", "refers_to": "S1!$A$1"})


class TestOpenpyxlNamedRanges:
Expand Down
54 changes: 54 additions & 0 deletions tests/test_renderer_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
TestResult,
)
from excelbench.results.renderer import (
_compute_fidelity_deltas,
_get_git_commit,
_group_test_cases,
_render_fidelity_deltas,
_render_per_test_table,
render_markdown,
score_emoji,
Expand Down Expand Up @@ -260,3 +262,55 @@ def test_render_markdown_write_only_lib_stats(tmp_path: Path) -> None:
content = out.read_text()
assert "xlsxwriter" in content
assert "Write" in content


# ─────────────────────────────────────────────────
# fidelity deltas
# ─────────────────────────────────────────────────


def test_compute_fidelity_deltas_detects_changes() -> None:
previous = {
"scores": {"openpyxl": {"cell_values": {"read": 3, "write": 3}}},
}
current = {
"scores": {"openpyxl": {"cell_values": {"read": 2, "write": 3}}},
}
deltas = _compute_fidelity_deltas(previous, current)
assert deltas == [
{
"library": "openpyxl",
"feature": "cell_values",
"mode": "read",
"previous": 3,
"current": 2,
"delta": -1,
}
]


def test_render_fidelity_deltas_needs_two_runs(tmp_path: Path) -> None:
out_dir = tmp_path / "results"
out_dir.mkdir(parents=True)
(out_dir / "history.jsonl").write_text('{"scores": {}}\n')
_render_fidelity_deltas(out_dir)
content = (out_dir / "FIDELITY_DELTAS.md").read_text()
assert "Need at least two runs" in content


def test_render_fidelity_deltas_writes_regression_table(tmp_path: Path) -> None:
out_dir = tmp_path / "results"
out_dir.mkdir(parents=True)
(out_dir / "history.jsonl").write_text(
"\n".join(
[
'{"run_date":"2026-01-01T00:00:00Z","scores":{"openpyxl":{"cell_values":{"read":3,"write":3}}}}',
'{"run_date":"2026-01-02T00:00:00Z","scores":{"openpyxl":{"cell_values":{"read":2,"write":3}}}}',
]
)
+ "\n"
)
_render_fidelity_deltas(out_dir)
content = (out_dir / "FIDELITY_DELTAS.md").read_text()
assert "Regressions: **1**" in content
assert "| openpyxl | cell_values | read | 3 | 2 | -1 |" in content
20 changes: 20 additions & 0 deletions tests/test_runner_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
_deep_compare,
_extract_column,
_extract_formula_sheet_names,
_failure_note_from_actual,
_find_by_key,
_find_range,
_find_rule,
Expand Down Expand Up @@ -619,3 +620,22 @@ def test_border_from_expected_edge_color_no_style() -> None:
assert border.top is not None
assert border.top.style == BorderStyle.THIN
assert border.top.color == "#FF0000"


# ─────────────────────────────────────────────────
# failure note mapping
# ─────────────────────────────────────────────────


def test_failure_note_from_actual_not_implemented() -> None:
assert _failure_note_from_actual({"error": "NotImplementedError: foo"}) == "Not implemented"


def test_failure_note_from_actual_unsupported() -> None:
actual = _failure_note_from_actual({"error": "feature unsupported by adapter"})
assert actual == "Not implemented"


def test_failure_note_from_actual_incorrect() -> None:
assert _failure_note_from_actual({"value": 1}) == "Incorrect result"
assert _failure_note_from_actual({"error": "ValueError: mismatch"}) == "Incorrect result"
10 changes: 6 additions & 4 deletions tests/test_tables.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ class _StubAdapter(StubExcelAdapter):
class TestTablesBase:
"""Base adapter API surface for tables."""

def test_read_tables_default_returns_empty(self) -> None:
def test_read_tables_default_raises(self) -> None:
adapter = _StubAdapter()
assert adapter.read_tables(object(), "S1") == []
with pytest.raises(NotImplementedError, match="table reads"):
adapter.read_tables(object(), "S1")

def test_add_table_default_is_noop(self) -> None:
def test_add_table_default_raises(self) -> None:
adapter = _StubAdapter()
adapter.add_table(object(), "S1", {"table": {"name": "T", "ref": "A1:B2"}})
with pytest.raises(NotImplementedError, match="table writes"):
adapter.add_table(object(), "S1", {"table": {"name": "T", "ref": "A1:B2"}})


class TestOpenpyxlTables:
Expand Down