diff --git a/src/excel_parser/parsers/cell_parser.py b/src/excel_parser/parsers/cell_parser.py index c2d5bdb..30af848 100644 --- a/src/excel_parser/parsers/cell_parser.py +++ b/src/excel_parser/parsers/cell_parser.py @@ -30,6 +30,9 @@ logger = logging.getLogger(__name__) +# Sentinel used to detect fonts with no non-default attributes. +_DEFAULT_FONT = FontStyle() + # Regex to extract cell and range references from formulas # Matches: A1, $A$1, Sheet1!A1, Sheet1!$A$1:$B$10, 'Sheet Name'!A1 _REF_RE = re.compile( @@ -279,12 +282,16 @@ def _extract_style(self, cell: OpenpyxlCell) -> CellStyle | None: alignment = self._extract_alignment(cell) number_format = cell.number_format if cell.number_format != "General" else None - # Check if any style is non-default + # Check if any style is non-default. Compare against the default DTO + # instead of enumerating attributes: an attribute-by-attribute check + # silently drops cells whose only formatting is an unlisted attribute + # (e.g. strikethrough-only or underline-only cells). The _extract_* + # helpers already return None when nothing is set. has_style = any([ - font and (font.bold or font.italic or font.name or font.size or font.color), - fill and fill.fg_color, - border and any([border.left, border.right, border.top, border.bottom]), - alignment and (alignment.horizontal or alignment.vertical or alignment.wrap_text), + font is not None and font != _DEFAULT_FONT, + fill is not None, + border is not None, + alignment is not None, number_format, ]) diff --git a/src/excel_parser/parsers/sheet_parser.py b/src/excel_parser/parsers/sheet_parser.py index 2819eb3..2412916 100644 --- a/src/excel_parser/parsers/sheet_parser.py +++ b/src/excel_parser/parsers/sheet_parser.py @@ -38,6 +38,11 @@ logger = logging.getLogger(__name__) +# openpyxl style-array slots that hold a non-zero id when the cell's format +# differs from the workbook default. These are the five aspects the parser +# turns into a CellStyle; see SheetParser._has_meaningful_style. +_STYLED_STYLE_SLOTS = ("fontId", "fillId", "borderId", "numFmtId", "alignmentId") + class SheetParser: """ @@ -97,8 +102,10 @@ def parse(self) -> SheetDTO: # Extract properties first sheet.properties = self._extract_properties() - # Extract merged regions - sheet.merged_regions = self._extract_merges() + # Extract merged regions. Overlapping regions cannot be represented + # consistently (a cell would be master of one and slave of another), + # so drop the losers before anything downstream sees them. + sheet.merged_regions = self._resolve_overlapping_merges(sheet, self._extract_merges()) # Build merge lookup for cell parsing merge_masters = self._build_merge_lookup(sheet.merged_regions) @@ -173,10 +180,14 @@ def _extract_cells( # Skip truly empty cells (no value, no formula, no style worth capturing) if cell.value is None and cell.data_type != "f" and not self._has_meaningful_style(cell): - # But still capture merged slaves + # But never drop a cell taking part in a merge. Slaves arrive as + # openpyxl MergedCell instances; the master is an ordinary Cell + # and can be empty, so it has to be recognised via the merge + # lookup. Dropping it strands every slave in the region with a + # merge_master that does not exist. from openpyxl.cell.cell import MergedCell as MergedCellType - if not isinstance(cell, MergedCellType): + if not isinstance(cell, MergedCellType) and (cell.row, cell.column) not in merge_masters: continue # Get computed value. Prefer the calamine-provided dict (populated @@ -195,19 +206,42 @@ def _extract_cells( cell_dto = self._cell_parser.parse(cell, computed_value) - # Annotate merge info + # Annotate merge info. CellParser pre-flags any openpyxl MergedCell + # as a slave without knowing its master, so the lookup is the single + # authority here: it must set master/slave exclusively and clear the + # provisional flag for merges we ended up not tracking. key = (cell.row, cell.column) - if key in merge_masters: - master_coord, row_span, col_span = merge_masters[key] + merge_info = merge_masters.get(key) + if merge_info is not None: + master_coord, row_span, col_span = merge_info if master_coord.row == cell.row and master_coord.col == cell.column: cell_dto.is_merged_master = True + cell_dto.is_merged_slave = False + cell_dto.merge_master = None cell_dto.merge_extent = row_span cell_dto.merge_col_extent = col_span else: cell_dto.is_merged_slave = True + cell_dto.is_merged_master = False cell_dto.merge_master = master_coord - - if not cell_dto.is_empty or cell_dto.is_merged_slave or cell_dto.is_merged_master: + elif cell_dto.is_merged_slave: + # openpyxl reported a MergedCell for a region we do not track + # (a dropped overlap, or a stale range): not a slave of anything. + cell_dto.is_merged_slave = False + cell_dto.merge_master = None + + # Keep a valueless cell when it carries formatting. Without this + # the _has_meaningful_style skip above was inert: it spared the + # cell from the early `continue` only for this gate to drop it, + # so styling on empty cells was parsed and then thrown away. + # Strikethrough or a fill on an otherwise blank row is exactly the + # "this is deprecated" marker downstream consumers need to see. + if ( + not cell_dto.is_empty + or cell_dto.is_merged_slave + or cell_dto.is_merged_master + or cell_dto.style is not None + ): sheet.set_cell(cell_dto) cell_count += 1 @@ -225,6 +259,85 @@ def _extract_merges(self) -> list[MergedRegion]: regions.append(MergedRegion(range=cell_range, master=master)) return regions + @staticmethod + def _ranges_overlap(a: CellRange, b: CellRange) -> bool: + """True when two rectangular ranges share at least one cell.""" + return not ( + a.bottom_right.row < b.top_left.row + or b.bottom_right.row < a.top_left.row + or a.bottom_right.col < b.top_left.col + or b.bottom_right.col < a.top_left.col + ) + + def _resolve_overlapping_merges( + self, sheet: SheetDTO, regions: list[MergedRegion] + ) -> list[MergedRegion]: + """ + Drop merged regions that overlap an already-accepted region. + + Excel forbids overlapping merges, but malformed and machine-generated + files contain them. They cannot be represented consistently: a cell in + the intersection would be the master of one region and a slave of + another, so ``_build_merge_lookup``'s flat dict silently kept whichever + region came last and left the cell flagged both master and slave (or a + slave whose master was never flagged). + + Resolution is first-come in reading order (top-left row, then column), + which is deterministic regardless of the order the file lists them in. + Dropped regions are reported as warnings rather than dropped silently. + """ + if len(regions) < 2: + return regions + + ordered = sorted( + range(len(regions)), + key=lambda i: ( + regions[i].range.top_left.row, + regions[i].range.top_left.col, + regions[i].range.bottom_right.row, + regions[i].range.bottom_right.col, + ), + ) + + kept: list[int] = [] + dropped: list[tuple[MergedRegion, MergedRegion]] = [] + for i in ordered: + clash = next( + (k for k in kept if self._ranges_overlap(regions[i].range, regions[k].range)), + None, + ) + if clash is None: + kept.append(i) + else: + dropped.append((regions[i], regions[clash])) + + if not dropped: + return regions + + for loser, winner in dropped: + sheet.errors.append( + ParseError( + severity=Severity.WARNING, + stage="parse", + message=( + f"Overlapping merged regions: {loser.range.to_a1()} overlaps " + f"{winner.range.to_a1()}; {loser.range.to_a1()} was dropped" + ), + sheet_name=self._sheet_name, + ) + ) + logger.warning( + "Sheet %s: dropped %d of %d merged regions for overlapping", + self._sheet_name, + len(dropped), + len(regions), + ) + + # Preserve the file's original ordering among the surviving regions so + # non-overlapping workbooks are completely unaffected by this pass. + survivors = set(kept) + return [r for i, r in enumerate(regions) if i in survivors] + def _build_merge_lookup( self, regions: list[MergedRegion] ) -> dict[tuple[int, int], tuple[CellCoord, int, int]]: @@ -668,17 +781,28 @@ def _update_master_cell(sheet: SheetDTO, master_coord: CellCoord, value: Any) -> @staticmethod def _has_meaningful_style(cell) -> bool: - """Check if a cell has non-default styling worth preserving.""" - try: - if cell.font and (cell.font.bold or cell.font.italic or cell.font.color): - return True - if cell.fill and cell.fill.patternType and cell.fill.patternType != "none": - return True - if cell.border: - for side in ("left", "right", "top", "bottom"): - s = getattr(cell.border, side, None) - if s and s.style: - return True - except Exception: - pass - return False + """ + Whether a valueless cell carries formatting worth keeping. + + This gates the skip for empty cells, so a false negative discards + formatting outright. The previous implementation enumerated three font + attributes (bold/italic/color) and was wrong in both directions: it + dropped cells whose only formatting was strikethrough, underline, a + font size or a font name, and it kept every *untouched* cell, because + an explicitly-built partial ``Font`` leaves ``color`` as None while the + inherited default font has one. + + openpyxl already records which aspects of a cell's format differ from + the workbook default, as non-zero style ids, so read that instead of + re-deriving it attribute by attribute — the enumeration is what grew + the blind spot. The slots checked are exactly the five aspects + ``_extract_style`` can turn into a CellStyle; protection and + quotePrefix are excluded because they carry nothing for an empty cell. + """ + # openpyxl exposes ``has_style`` but not the ids behind it, hence + # ``_style``; guarded so a future rename degrades to "not styled" + # rather than raising mid-parse. + style = getattr(cell, "_style", None) + if style is None: + return False + return any(getattr(style, slot, 0) for slot in _STYLED_STYLE_SLOTS) diff --git a/src/excel_parser/pipeline.py b/src/excel_parser/pipeline.py index 797cd04..c4fcc9f 100644 --- a/src/excel_parser/pipeline.py +++ b/src/excel_parser/pipeline.py @@ -106,9 +106,13 @@ def _chunk_cells(chunk: ChunkDTO, workbook: WorkbookDTO) -> list[dict[str, Any]] # colors: font and fill from style font_color = None fill_color = None + # strikethrough commonly marks deprecated/void rows, so downstream + # consumers need it alongside the colors. + font_strikethrough = False if cell.style: - if cell.style.font and cell.style.font.color: + if cell.style.font: font_color = cell.style.font.color + font_strikethrough = cell.style.font.strikethrough if cell.style.fill and cell.style.fill.fg_color: fill_color = cell.style.fill.fg_color cells.append({ @@ -117,6 +121,7 @@ def _chunk_cells(chunk: ChunkDTO, workbook: WorkbookDTO) -> list[dict[str, Any]] "formula": formula, "font_color": font_color, "fill_color": fill_color, + "font_strikethrough": font_strikethrough, }) return cells diff --git a/tests/conftest.py b/tests/conftest.py index 28842ec..9dd7c65 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -41,6 +41,9 @@ "freeze_panes_workbook", "wide_workbook", "styled_workbook", + "strikethrough_workbook", + "overlapping_merges_workbook", + "styled_empty_cells_workbook", "assumptions_workbook", "hyperlink_workbook", "two_tables_vertical", @@ -455,6 +458,87 @@ def wide_workbook(tmp_dir) -> Path: return path +@pytest.fixture +def strikethrough_workbook(tmp_dir) -> Path: + """Workbook whose only formatting on some cells is strikethrough/underline.""" + path = tmp_dir / "strikethrough.xlsx" + wb = Workbook() + ws = wb.active + ws.title = "Sheet1" + + ws["A1"] = "Normal" + ws["B1"] = "Strike" + ws["B1"].font = Font(strike=True) + ws["C1"] = "BoldStrike" + ws["C1"].font = Font(bold=True, strike=True, color="FF0000") + ws["D1"] = "Underline" + ws["D1"].font = Font(underline="single") + + wb.save(path) + return path + + +@pytest.fixture +def styled_empty_cells_workbook(tmp_dir) -> Path: + """ + Workbook whose formatting lives on cells that hold no value. + + One cell per style aspect, so a predicate that enumerates attributes + instead of asking openpyxl fails on whichever aspect it forgot. A6 is the + control: touched by nothing, and must stay out of the parse output. + """ + path = tmp_dir / "styled_empty.xlsx" + wb = Workbook() + ws = wb.active + ws.title = "Sheet1" + + ws["A1"] = "anchor" # keeps the sheet's used range honest + ws["B1"].font = Font(strike=True) + ws["B2"].font = Font(underline="single") + ws["B3"].font = Font(bold=True) + ws["B4"].font = Font(size=20) + ws["B5"].font = Font(name="Courier New") + ws["C1"].number_format = "0.00%" + ws["C2"].alignment = Alignment(horizontal="center") + ws["C3"].fill = PatternFill("solid", fgColor="FFFF00") + ws["C4"].border = Border(left=Side(style="thin")) + + wb.save(path) + return path + + +@pytest.fixture +def overlapping_merges_workbook(tmp_dir) -> Path: + """ + Workbook with merged regions that overlap each other. + + Excel forbids this, but machine-generated files contain it. Both shapes + that broke the merge invariants are represented: + + * ``B2:C5`` / ``C5:E8`` — C5 is the master of one region and a slave of + the other, so the region that lost the race left C5 flagged as a slave + whose master was never flagged (``M2``). + * ``H2:J5`` / ``I4:L7`` — I4 is a master and a slave simultaneously, + leaving it flagged both (``M4``) and slave-with-no-master (``M1``). + """ + path = tmp_dir / "overlapping_merges.xlsx" + wb = Workbook() + ws = wb.active + ws.title = "Sheet1" + + ws["A1"] = "Overlapping merge regions" + ws["B2"] = "first" + ws["C5"] = "second" + ws["H2"] = "third" + ws["I4"] = "fourth" + + for rng in ("B2:C5", "C5:E8", "H2:J5", "I4:L7"): + ws.merge_cells(rng) + + wb.save(path) + return path + + @pytest.fixture def styled_workbook(tmp_dir) -> Path: """Workbook with rich formatting: borders, fills, fonts, alignment.""" diff --git a/tests/helpers/corpus_downloader.py b/tests/helpers/corpus_downloader.py index 9cdb130..db37b98 100644 --- a/tests/helpers/corpus_downloader.py +++ b/tests/helpers/corpus_downloader.py @@ -1,16 +1,26 @@ """ Download real-world Excel corpora for robustness testing. -Supports downloading .xlsx files from: -- EUSES Spreadsheet Corpus (Zenodo) -- Enron Spreadsheet Corpus (SheetJS GitHub) -- Additional GitHub repositories with public xlsx samples +Sources are chosen for durability over volume: + +- **PyPI sdists** of libraries that bundle .xlsx test fixtures. A published + PyPI artifact is immutable, so these downloads cannot rot; only a yanked + release would break them, and the version is resolved at run time. +- **Upstream test-fixture directories** on GitHub, enumerated through the + contents API rather than a hand-written file list, so individual renames + upstream do not silently reduce the corpus to nothing. + +An earlier revision pointed at the SheetJS ``test_files`` submodule (now +blocked by GitHub), the EUSES archive on Zenodo (record withdrawn) and the +Enron ``.xls`` dump (321 MB, ~0 .xlsx). All four URLs 404'd or yielded +nothing, which is why every downloader here returns the paths it actually +obtained and the tests assert on that. """ - import io import logging +import tarfile import zipfile from pathlib import Path @@ -21,6 +31,57 @@ # Timeout for HTTP requests _TIMEOUT = 60 +# Upstream fixture directories: (repo, ref, path). Enumerated via the GitHub +# contents API. These are deliberately parser test-suites — they concentrate +# the malformed, edge-case and adversarial workbooks that a robustness corpus +# wants (missing dimensions, empty shared strings, merged ranges, pivots, +# rich text, shared formulas, non-standard XML namespaces). +_GITHUB_FIXTURE_DIRS: tuple[tuple[str, str, str], ...] = ( + ("pandas-dev/pandas", "main", "pandas/tests/io/data/excel"), + ("tafia/calamine", "master", "tests"), +) + +# PyPI packages whose sdists ship .xlsx fixtures. +_PYPI_PACKAGES: tuple[str, ...] = ( + "python-calamine", + "xlsx2csv", + "pyexcel-xlsx", + "excelrd", + "tablib", + "pyexcel", +) + + +def _is_safe_member(name: str) -> bool: + """Reject archive members that are not plain, in-tree .xlsx files.""" + if not name.lower().endswith(".xlsx"): + return False + if name.startswith(("/", "__MACOSX", ".")): + return False + # Path traversal guard: no member may escape the extraction directory. + return ".." not in Path(name).parts + + +def _write_member(target_dir: Path, name: str, data: bytes) -> Path | None: + """ + Write one archive member into target_dir under its basename. + + Members that are not real XLSX containers are skipped. Upstream test + suites ship negative fixtures under an .xlsx name — a zero-byte file, an + encrypted OLE workbook — which belong in targeted unit tests, not in a + corpus whose whole purpose is asserting that well-formed workbooks parse. + """ + safe_name = Path(name).name + if not safe_name: + return None + if not zipfile.is_zipfile(io.BytesIO(data)): + logger.debug("Skipping %s: not an XLSX (ZIP) container", name) + return None + dest = target_dir / safe_name + if not dest.exists(): + dest.write_bytes(data) + return dest + def download_and_extract_xlsx( url: str, @@ -28,45 +89,47 @@ def download_and_extract_xlsx( max_files: int = 50, ) -> list[Path]: """ - Download a ZIP archive and extract .xlsx files. + Download a ZIP or tar.gz archive and extract the .xlsx files it contains. - Returns list of extracted .xlsx paths. + Returns the list of extracted .xlsx paths (empty if the download failed). """ target_dir.mkdir(parents=True, exist_ok=True) files: list[Path] = [] try: logger.info("Downloading %s ...", url) - resp = requests.get(url, timeout=_TIMEOUT, stream=True) + resp = requests.get(url, timeout=_TIMEOUT) resp.raise_for_status() content = resp.content - - if not zipfile.is_zipfile(io.BytesIO(content)): - logger.warning("URL did not return a ZIP file: %s", url) - return files - - with zipfile.ZipFile(io.BytesIO(content)) as zf: - xlsx_names = [ - n for n in zf.namelist() - if n.lower().endswith(".xlsx") - and not n.startswith("__MACOSX") - and not n.startswith(".") - ] - logger.info("Found %d .xlsx files in archive", len(xlsx_names)) - - for name in xlsx_names[:max_files]: - safe_name = Path(name).name - if not safe_name: - continue - dest = target_dir / safe_name - if not dest.exists(): - dest.write_bytes(zf.read(name)) - files.append(dest) - except requests.RequestException as e: logger.warning("Failed to download %s: %s", url, e) - except zipfile.BadZipFile as e: - logger.warning("Bad ZIP from %s: %s", url, e) + return files + + buf = io.BytesIO(content) + try: + if zipfile.is_zipfile(buf): + buf.seek(0) + with zipfile.ZipFile(buf) as zf: + names = [n for n in zf.namelist() if _is_safe_member(n)] + logger.info("Found %d .xlsx files in %s", len(names), url) + for name in names[:max_files]: + dest = _write_member(target_dir, name, zf.read(name)) + if dest is not None: + files.append(dest) + else: + buf.seek(0) + with tarfile.open(fileobj=buf, mode="r:*") as tf: + members = [m for m in tf.getmembers() if m.isfile() and _is_safe_member(m.name)] + logger.info("Found %d .xlsx files in %s", len(members), url) + for member in members[:max_files]: + handle = tf.extractfile(member) + if handle is None: + continue + dest = _write_member(target_dir, member.name, handle.read()) + if dest is not None: + files.append(dest) + except (zipfile.BadZipFile, tarfile.TarError) as e: + logger.warning("Unreadable archive from %s: %s", url, e) return files @@ -87,70 +150,111 @@ def download_single_xlsx( try: resp = requests.get(url, timeout=_TIMEOUT) resp.raise_for_status() - dest.write_bytes(resp.content) - return dest except requests.RequestException as e: logger.warning("Failed to download %s: %s", url, e) return None + if not zipfile.is_zipfile(io.BytesIO(resp.content)): + logger.debug("Skipping %s: not an XLSX (ZIP) container", url) + return None + dest.write_bytes(resp.content) + return dest + + +def _list_github_xlsx(repo: str, ref: str, path: str) -> list[tuple[str, str]]: + """ + List .xlsx files in a GitHub directory as (raw_url, filename) pairs. + + Uses the contents API so an upstream rename shrinks the corpus by one file + instead of breaking a hard-coded URL list. + """ + api = f"https://api.github.com/repos/{repo}/contents/{path}?ref={ref}" + try: + resp = requests.get(api, timeout=_TIMEOUT, headers={"Accept": "application/vnd.github+json"}) + resp.raise_for_status() + entries = resp.json() + except (requests.RequestException, ValueError) as e: + logger.warning("Could not list %s/%s: %s", repo, path, e) + return [] + + if not isinstance(entries, list): + # A submodule or a file resolves to a dict, not a listing. + logger.warning("%s/%s is not a directory listing", repo, path) + return [] + + out: list[tuple[str, str]] = [] + for entry in entries: + name = entry.get("name", "") + download_url = entry.get("download_url") + if entry.get("type") == "file" and name.lower().endswith(".xlsx") and download_url: + out.append((download_url, name)) + return out + def download_github_xlsx_samples( target_dir: Path, max_files: int = 50, ) -> list[Path]: - """ - Download diverse .xlsx samples from known public GitHub repos. - - These are individual files from repos known to contain xlsx samples. - """ + """Download .xlsx fixtures from upstream parser test suites on GitHub.""" target_dir.mkdir(parents=True, exist_ok=True) files: list[Path] = [] - # Known public xlsx sample URLs (raw GitHub links) - sample_urls = [ - # SheetJS test files - ("https://raw.githubusercontent.com/SheetJS/sheetjs/master/test_files/comments_stress_test.xlsx", "comments_stress.xlsx"), - ("https://raw.githubusercontent.com/SheetJS/sheetjs/master/test_files/merge_cells.xlsx", "merge_cells.xlsx"), - ("https://raw.githubusercontent.com/SheetJS/sheetjs/master/test_files/number_format.xlsx", "number_format.xlsx"), - # openpyxl test fixtures - ("https://raw.githubusercontent.com/openpyxl/openpyxl/master/openpyxl/tests/data/genuine/empty.xlsx", "genuine_empty.xlsx"), - ] - - for url, fname in sample_urls: + for repo, ref, path in _GITHUB_FIXTURE_DIRS: if len(files) >= max_files: break - path = download_single_xlsx(url, target_dir, fname) - if path: - files.append(path) + for url, fname in _list_github_xlsx(repo, ref, path): + if len(files) >= max_files: + break + # Namespace by repo: pandas and calamine both ship merge_cells.xlsx. + prefix = repo.split("/")[-1] + dest = download_single_xlsx(url, target_dir, f"{prefix}_{fname}") + if dest is not None: + files.append(dest) return files -def download_euses_corpus( +def download_pypi_corpus( target_dir: Path, max_files: int = 50, ) -> list[Path]: """ - Download EUSES corpus from Zenodo and extract .xlsx files. + Download .xlsx fixtures from the sdists of Excel-handling PyPI packages. - Note: EUSES is mostly .xls files. The .xlsx subset may be small. + PyPI artifacts are immutable, making this the most durable of the sources. """ - url = "https://zenodo.org/records/581673/files/EUSES.zip" - return download_and_extract_xlsx(url, target_dir, max_files) + target_dir.mkdir(parents=True, exist_ok=True) + files: list[Path] = [] + for package in _PYPI_PACKAGES: + if len(files) >= max_files: + break + sdist_url = _resolve_pypi_sdist(package) + if sdist_url is None: + continue + files.extend( + download_and_extract_xlsx(sdist_url, target_dir, max_files=max_files - len(files)) + ) -def download_enron_corpus( - target_dir: Path, - max_files: int = 50, -) -> list[Path]: - """ - Download Enron spreadsheets from SheetJS GitHub repo. + return files - Note: Enron files are almost entirely .xls (pre-2007). - The .xlsx subset will likely be empty or very small. - """ - url = "https://github.com/SheetJS/enron_xls/archive/refs/heads/master.zip" - return download_and_extract_xlsx(url, target_dir, max_files) + +def _resolve_pypi_sdist(package: str) -> str | None: + """Resolve the sdist download URL for a package's current release.""" + try: + resp = requests.get(f"https://pypi.org/pypi/{package}/json", timeout=_TIMEOUT) + resp.raise_for_status() + payload = resp.json() + except (requests.RequestException, ValueError) as e: + logger.warning("Could not resolve sdist for %s: %s", package, e) + return None + + for url_entry in payload.get("urls", []): + if url_entry.get("packagetype") == "sdist": + return url_entry.get("url") + + logger.warning("No sdist published for %s", package) + return None def get_corpus_files(corpus_dir: Path) -> list[Path]: diff --git a/tests/test_corpus_robustness.py b/tests/test_corpus_robustness.py index 974b19b..f7de3a0 100644 --- a/tests/test_corpus_robustness.py +++ b/tests/test_corpus_robustness.py @@ -11,16 +11,18 @@ import json +import zipfile from pathlib import Path +import openpyxl import pytest +import requests from excel_parser.models.common import Severity from excel_parser.pipeline import parse_workbook from tests.helpers.corpus_downloader import ( - download_enron_corpus, - download_euses_corpus, download_github_xlsx_samples, + download_pypi_corpus, get_corpus_files, ) from tests.helpers.invariant_checker import check_invariants @@ -40,6 +42,30 @@ def _collect_corpus_files() -> list[Path]: corpus_files = _collect_corpus_files() +_loadable_cache: dict[Path, bool] = {} + + +def _is_loadable(path: Path) -> bool: + """ + True when openpyxl can open the file at all. + + Used to separate "the parser mishandled a workbook" from "this file is not + a workbook any reader can open". Cached because the corpus tests re-parse + each file per assertion. + """ + cached = _loadable_cache.get(path) + if cached is not None: + return cached + + loadable = True + try: + wb = openpyxl.load_workbook(path, read_only=True) + wb.close() + except Exception: + loadable = False + _loadable_cache[path] = loadable + return loadable + # --------------------------------------------------------------------------- # Corpus download tests (require network) @@ -48,23 +74,46 @@ def _collect_corpus_files() -> list[Path]: @pytest.mark.corpus class TestCorpusDownload: - """Download corpus files. Run once, then corpus tests use the files.""" + """ + Download corpus files. Run once, then corpus tests use the files. + + These assert that files actually arrived. An earlier revision asserted + only ``isinstance(files, list)``, which stayed green for months while + every upstream URL 404'd and the corpus directory sat empty — silently + skipping the eight robustness tests that depend on it. A genuinely + offline machine is a skip; a reachable source returning nothing usable + is a failure. + """ + + @staticmethod + def _require_network() -> None: + try: + requests.head("https://pypi.org/simple/", timeout=15) + except requests.RequestException as e: + pytest.skip(f"network unavailable: {e}") + + def test_download_pypi_corpus(self): + self._require_network() + target = CORPUS_DIR / "pypi" + files = download_pypi_corpus(target, max_files=40) + assert files, "no .xlsx fixtures extracted from any PyPI sdist" + assert all(f.exists() and f.stat().st_size > 0 for f in files) def test_download_github_samples(self): + self._require_network() target = CORPUS_DIR / "github_samples" - files = download_github_xlsx_samples(target, max_files=20) - # Some URLs may fail; just ensure we tried - assert isinstance(files, list) + files = download_github_xlsx_samples(target, max_files=40) + if not files: + pytest.skip("GitHub fixture listing unavailable (rate limit or upstream move)") + assert all(f.exists() and f.stat().st_size > 0 for f in files) - def test_download_euses(self): - target = CORPUS_DIR / "euses" - files = download_euses_corpus(target, max_files=50) - assert isinstance(files, list) - - def test_download_enron(self): - target = CORPUS_DIR / "enron" - files = download_enron_corpus(target, max_files=50) - assert isinstance(files, list) + def test_downloaded_files_are_parseable_xlsx(self): + """A download that yields unopenable bytes is worse than no download.""" + self._require_network() + files = download_pypi_corpus(CORPUS_DIR / "pypi", max_files=5) + assert files, "no .xlsx fixtures extracted from any PyPI sdist" + for path in files[:5]: + assert zipfile.is_zipfile(path), f"{path.name} is not a valid xlsx container" # --------------------------------------------------------------------------- @@ -91,11 +140,34 @@ def test_no_unhandled_exception(self, xlsx_path): pytest.fail(f"Parser crashed on {xlsx_path.name}: {e}") def test_has_sheets(self, xlsx_path): + """ + A workbook that can be opened at all must yield at least one sheet. + + Upstream parser test-suites contribute files that are valid ZIPs but + not valid OOXML packages (a missing ``[Content_Types].xml``, a missing + ``sharedStrings.xml``, a style attribute openpyxl rejects). No reader + can open those, so demanding a sheet from them asserts the impossible; + the contract that actually matters for them — degrade with a recorded + error instead of crashing — is covered by + ``test_unloadable_files_report_an_error``. + """ + if not _is_loadable(xlsx_path): + pytest.skip(f"{xlsx_path.name} is not a loadable OOXML package") result = parse_workbook(path=xlsx_path) assert len(result.workbook.sheets) >= 1, ( f"{xlsx_path.name}: no sheets parsed" ) + def test_unloadable_files_report_an_error(self, xlsx_path): + """An unopenable workbook must say so, not come back silently empty.""" + if _is_loadable(xlsx_path): + pytest.skip(f"{xlsx_path.name} loads fine") + result = parse_workbook(path=xlsx_path) + assert result.workbook.errors, ( + f"{xlsx_path.name}: failed to load but recorded no error" + ) + assert any(e.severity == Severity.ERROR for e in result.workbook.errors) + def test_workbook_hash_present(self, xlsx_path): result = parse_workbook(path=xlsx_path) assert result.workbook.workbook_hash, ( @@ -132,25 +204,33 @@ class TestCorpusAggregateStats: """Aggregate statistics across the whole corpus.""" def test_success_rate(self): - """At least 95% of corpus files should parse without ERROR-level errors.""" - total = len(corpus_files) - errors = 0 - for path in corpus_files: + """ + At least 95% of *loadable* corpus files parse without ERROR-level errors. + + The denominator is deliberately the loadable subset. Sources include + upstream parser test-suites, which seed the corpus with files that are + intentionally broken; counting those as parser failures would make the + threshold a measure of how many negative fixtures upstream happens to + ship rather than of this parser's health. + """ + loadable = [p for p in corpus_files if _is_loadable(p)] + total = len(loadable) + if total == 0: + pytest.skip("no loadable corpus files") + + failures = [] + for path in loadable: try: result = parse_workbook(path=path) - has_errors = any( - e.severity == Severity.ERROR - for e in result.workbook.errors - ) - if has_errors: - errors += 1 - except Exception: - errors += 1 + if any(e.severity == Severity.ERROR for e in result.workbook.errors): + failures.append(path.name) + except Exception as e: + failures.append(f"{path.name} ({type(e).__name__}: {e})") - rate = (total - errors) / total if total > 0 else 1.0 + rate = (total - len(failures)) / total assert rate >= 0.95, ( - f"Success rate {rate:.1%} ({total - errors}/{total}) " - f"below 95% threshold" + f"Success rate {rate:.1%} ({total - len(failures)}/{total}) " + f"below 95% threshold; failures: {failures[:10]}" ) def test_aggregate_stats(self): diff --git a/tests/test_parsers.py b/tests/test_parsers.py index 836d15e..d8a55c5 100644 --- a/tests/test_parsers.py +++ b/tests/test_parsers.py @@ -7,7 +7,15 @@ """ +import openpyxl +import pytest +from openpyxl import Workbook +from openpyxl.styles import Font + +from excel_parser.models.common import Severity from excel_parser.parsers import WorkbookParser +from excel_parser.parsers.sheet_parser import SheetParser +from tests.helpers.invariant_checker import check_invariants class TestSimpleWorkbook: @@ -87,6 +95,174 @@ def test_merge_slave_annotated(self, merged_cells_workbook): assert b1.is_merged_slave is True +class TestStyledEmptyCells: + """ + Formatting on a valueless cell must survive the parse. + + Two bugs met here. ``_has_meaningful_style`` enumerated three font + attributes and so missed strikethrough, underline, size and name, while + reporting *untouched* cells as styled (a partial ``Font`` leaves ``color`` + None; the inherited default font has one). And the gate that stores cells + dropped anything empty regardless, which made the predicate inert — the + styling was parsed and then discarded. + """ + + @pytest.mark.parametrize( + ("coord", "attr", "expected"), + [ + ((1, 2), "strikethrough", True), + ((2, 2), "underline", "single"), + ((3, 2), "bold", True), + ((4, 2), "size", 20.0), + ((5, 2), "name", "Courier New"), + ], + ) + def test_font_only_empty_cell_kept( + self, styled_empty_cells_workbook, coord, attr, expected + ): + result = WorkbookParser(path=styled_empty_cells_workbook).parse() + cell = result.sheets[0].get_cell(*coord) + assert cell is not None, f"empty cell with {attr} was discarded" + assert cell.style is not None and cell.style.font is not None + assert getattr(cell.style.font, attr) == expected + + @pytest.mark.parametrize( + ("coord", "attr"), + [ + ((1, 3), "number_format"), + ((2, 3), "alignment"), + ((3, 3), "fill"), + ((4, 3), "border"), + ], + ) + def test_non_font_only_empty_cell_kept(self, styled_empty_cells_workbook, coord, attr): + result = WorkbookParser(path=styled_empty_cells_workbook).parse() + cell = result.sheets[0].get_cell(*coord) + assert cell is not None, f"empty cell with {attr} was discarded" + assert cell.style is not None + assert getattr(cell.style, attr) is not None + + def test_untouched_empty_cell_still_dropped(self, styled_empty_cells_workbook): + """The fix must not start hoarding genuinely blank cells.""" + result = WorkbookParser(path=styled_empty_cells_workbook).parse() + sheet = result.sheets[0] + assert sheet.get_cell(6, 1) is None + assert sheet.get_cell(20, 20) is None + + def test_styled_empty_cells_are_still_empty(self, styled_empty_cells_workbook): + """Kept for their style, not misreported as carrying data.""" + result = WorkbookParser(path=styled_empty_cells_workbook).parse() + cell = result.sheets[0].get_cell(1, 2) + assert cell.is_empty is True + assert cell.raw_value is None + + def test_unstyled_empty_merge_master_is_kept(self, tmp_dir): + """ + A merge master that is empty *and* unstyled must survive the skip. + + It arrives as an ordinary Cell, not a MergedCell, so the empty-cell + skip only spares it via the merge lookup. Dropping it strands every + slave in the region with a merge_master that does not exist — which + the old predicate hid by calling every untouched cell styled. + """ + path = tmp_dir / "empty_unstyled_master.xlsx" + wb = Workbook() + ws = wb.active + ws["A1"] = "anchor" + ws.merge_cells("B2:D2") # master B2 left empty and unformatted + wb.save(path) + + result = WorkbookParser(path=path).parse() + sheet = result.sheets[0] + master = sheet.get_cell(2, 2) + assert master is not None, "empty unstyled merge master was discarded" + assert master.is_merged_master is True + assert check_invariants(result) == [] + + def test_predicate_rejects_unstyled_cell(self, tmp_dir): + """_has_meaningful_style must not call every default cell styled.""" + path = tmp_dir / "one_styled.xlsx" + wb = Workbook() + ws = wb.active + ws["A1"] = "anchor" + ws["B1"].font = Font(strike=True) + wb.save(path) + + loaded = openpyxl.load_workbook(path).active + assert SheetParser._has_meaningful_style(loaded["B1"]) is True + # A1 carries a value but no styling; C1 was never touched at all. + assert SheetParser._has_meaningful_style(loaded["A1"]) is False + assert SheetParser._has_meaningful_style(loaded["C1"]) is False + + +class TestOverlappingMerges: + """ + Overlapping merged regions must not corrupt the merge invariants. + + A flat (row, col) -> master lookup let a later region overwrite an + earlier one, so a cell in the intersection came out flagged both master + and slave, or a slave pointing at a master that was never flagged. + """ + + def test_invariants_hold(self, overlapping_merges_workbook): + result = WorkbookParser(path=overlapping_merges_workbook).parse() + assert check_invariants(result) == [] + + def test_no_cell_is_both_master_and_slave(self, overlapping_merges_workbook): + result = WorkbookParser(path=overlapping_merges_workbook).parse() + for sheet in result.sheets: + for cell in sheet.cells.values(): + assert not (cell.is_merged_master and cell.is_merged_slave), ( + f"{cell.a1_ref} is both master and slave" + ) + + def test_every_slave_points_at_a_flagged_master(self, overlapping_merges_workbook): + result = WorkbookParser(path=overlapping_merges_workbook).parse() + for sheet in result.sheets: + for cell in sheet.cells.values(): + if not cell.is_merged_slave: + continue + assert cell.merge_master is not None, f"{cell.a1_ref} slave with no master" + master = sheet.get_cell(cell.merge_master.row, cell.merge_master.col) + assert master is not None and master.is_merged_master + + def test_overlaps_are_dropped_not_silently_kept(self, overlapping_merges_workbook): + result = WorkbookParser(path=overlapping_merges_workbook).parse() + sheet = result.sheets[0] + # Four regions declared, two of them overlapping an earlier one. + assert len(sheet.merged_regions) == 2 + kept = {r.range.to_a1() for r in sheet.merged_regions} + assert kept == {"B2:C5", "H2:J5"} + + def test_dropped_overlaps_are_reported(self, overlapping_merges_workbook): + result = WorkbookParser(path=overlapping_merges_workbook).parse() + sheet = result.sheets[0] + warnings = [e for e in sheet.errors if "Overlapping merged regions" in e.message] + assert len(warnings) == 2 + assert all(w.severity == Severity.WARNING for w in warnings) + + def test_resolution_is_reading_order_not_file_order(self, tmp_dir): + """The surviving region is the upper-left one regardless of declaration order.""" + path = tmp_dir / "reversed_overlap.xlsx" + wb = Workbook() + ws = wb.active + # Declared bottom-right first; B2:C5 must still be the survivor. + for rng in ("C5:E8", "B2:C5"): + ws.merge_cells(rng) + wb.save(path) + + result = WorkbookParser(path=path).parse() + kept = {r.range.to_a1() for r in result.sheets[0].merged_regions} + assert kept == {"B2:C5"} + + def test_non_overlapping_merges_are_untouched(self, merged_cells_workbook): + """The overlap pass must not perturb well-formed workbooks.""" + result = WorkbookParser(path=merged_cells_workbook).parse() + sheet = result.sheets[0] + assert not [e for e in sheet.errors if "Overlapping" in e.message] + assert len(sheet.merged_regions) >= 2 + + class TestEmptyMasterRecovery: """Test OOXML recovery of values from empty merge masters.""" @@ -289,6 +465,35 @@ def test_border_extracted(self, styled_workbook): assert a1.style.border is not None +class TestStrikethroughOnlyStyles: + """Cells whose only non-default font attribute must keep their style.""" + + def test_strikethrough_only_cell_keeps_style(self, strikethrough_workbook): + result = WorkbookParser(path=strikethrough_workbook).parse() + b1 = result.sheets[0].get_cell(1, 2) + assert b1.style is not None + assert b1.style.font is not None + assert b1.style.font.strikethrough is True + + def test_underline_only_cell_keeps_style(self, strikethrough_workbook): + result = WorkbookParser(path=strikethrough_workbook).parse() + d1 = result.sheets[0].get_cell(1, 4) + assert d1.style is not None + assert d1.style.font is not None + assert d1.style.font.underline == "single" + + def test_strikethrough_with_other_attributes(self, strikethrough_workbook): + result = WorkbookParser(path=strikethrough_workbook).parse() + c1 = result.sheets[0].get_cell(1, 3) + assert c1.style.font.strikethrough is True + assert c1.style.font.bold is True + + def test_unstruck_cell_reports_false(self, strikethrough_workbook): + result = WorkbookParser(path=strikethrough_workbook).parse() + a1 = result.sheets[0].get_cell(1, 1) + assert a1.style.font.strikethrough is False + + class TestWideSheet: """Test wide sheets with many columns.""" diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 60ad4f8..8b196cb 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -108,6 +108,31 @@ def test_serializer_records(self, simple_workbook): assert "metadata" in entry +class TestChunkCellStrikethrough: + """Strikethrough is exposed on chunk cells in to_json output.""" + + @staticmethod + def _cells_by_address(result): + data = result.to_json() + return { + cell["address"]: cell + for chunk in data["chunks"] + for cell in chunk.get("cells", []) + } + + def test_struck_cell_flagged(self, strikethrough_workbook): + result = parse_workbook(path=strikethrough_workbook) + cells = self._cells_by_address(result) + assert cells["B1"]["font_strikethrough"] is True + assert cells["C1"]["font_strikethrough"] is True + + def test_unstruck_cell_not_flagged(self, strikethrough_workbook): + result = parse_workbook(path=strikethrough_workbook) + cells = self._cells_by_address(result) + assert cells["A1"]["font_strikethrough"] is False + assert cells["D1"]["font_strikethrough"] is False + + class TestAssumptionsPipeline: """Test the pipeline on an assumptions/results workbook."""