Skip to content

Commit 49db91c

Browse files
lmeyerovclaude
andauthored
test(gfql): make the polars lane see the 280 tests it was skipping everywhere (#1805)
* test(gfql): make the polars lane see the 280 tests it was skipping everywhere polars is installed in exactly ONE CI lane (test-polars, driven by bin/test-polars.sh), and that lane runs a hand-maintained file list. Every other lane collection-skips a module-level `pytest.importorskip("polars")` and every polars/polars-gpu parameter. A polars-gated test the list omits therefore runs NOWHERE while both lanes report green. Measured on run 30311675717 (PR #1799): 280 tests executed locally with polars installed and appear in ZERO job logs of that run, including six whole modules — four of them named `test_engine_polars_*`. Two real defects were sitting in the blind spot: - `test_explicit_polars_gpu_declines_indexed_helper_and_falls_back` executes engine='polars-gpu' but only gates on `importorskip("polars")`, so it raises ImportError wherever polars is present without the RAPIDS cudf_polars stack. It has never once run: 9 SKIPPED, 0 executed, across the whole run. - `test_case_regex_unicode_trick_matrix[toupper-eq-ss-fold]` and `[tolower-turkish-dotted-i]` catch a genuine silent cross-engine divergence: under pandas>=3 the pandas engine answers [9] where polars answers [8, 9] for `toUpper(n.name) = 'STRASSE'`. Filed as #1802 with the repro; both rows are marked xfail(strict=True) keyed on the pandas accessor's own behaviour, so the fix XPASSes and retires the marks. Changes: - bin/test-polars.sh gains the six invisible modules plus the four whose polars parameters were invisible. - New `test_polars_lane_completeness.py` parses POLARS_TEST_FILES and fails when a polars-mentioning test module is neither in the lane nor in a documented exemption, when a module-level polars gate sits outside the lane, when a listed path is gone, or when a polars-gated test in the `-k polars` phase is unselectable by that filter. Mutation-checked: dropping one entry turns two of its assertions red. - Split the polars-gpu test so the CPU half (helper must decline) runs wherever polars does, and the execution half skips honestly without cudf_polars. - Renamed the two test_lowering.py polars tests `-k polars` could not select. Test-only; no product code touched, so no CHANGELOG entry. Widened lane, py3.11 + polars 1.43.1 + pandas 3.0.5: 2726 passed, 142 skipped, 3 xfailed, 0 failed (was 1636 passed on the same box). ORDERING NOTE: #1797 records that the py3.12 coverage cell is already at its 10-minute ceiling and that adding a single file tipped it into a timeout; the pending ci.yml split (comment on PR #1794) must land BEFORE this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB * test(gfql): granular AST types in the polars-lane completeness guard Replace the getattr/ast.AST pairing in the guard's two helpers with the concrete node types they are actually called with: _node_source takes ast.stmt | ast.expr and reads .lineno/.end_lineno directly (end_lineno is Optional[int], so the None case is handled rather than defaulted through getattr), and _selected_by_k_polars takes a FuncDef alias so .name and .decorator_list are checked attributes instead of getattr lookups with invented fallbacks. No behaviour change; guard still 8 passed, ruff clean, mypy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 233b64c commit 49db91c

5 files changed

Lines changed: 283 additions & 7 deletions

File tree

bin/test-polars.sh

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ set -ex
1212
python -m pytest --version
1313

1414
# Single source of truth for the polars test file list (CI reuses this script).
15+
#
16+
# COMPLETENESS IS ENFORCED, NOT REMEMBERED: polars is installed in NO other CI lane, so a
17+
# module gated by `pytest.importorskip("polars")` that is missing from this array runs
18+
# NOWHERE — it is collection-skipped everywhere else and silently reports green.
19+
# `graphistry/tests/compute/gfql/test_polars_lane_completeness.py` parses this array and
20+
# fails if any module-level polars-gated test file is absent from it (or listed but gone).
1521
POLARS_TEST_FILES=(
1622
graphistry/tests/compute/test_polars.py
1723
graphistry/tests/compute/gfql/test_engine_polars_hop.py
@@ -29,6 +35,20 @@ POLARS_TEST_FILES=(
2935
graphistry/tests/compute/gfql/test_polars_rows_entity_groupby.py
3036
graphistry/tests/compute/gfql/test_seeded_typed_hop_fastpath.py
3137
graphistry/tests/compute/gfql/test_residual_polars_native.py
38+
# module-level `importorskip("polars")` files that previously ran in no lane at all
39+
graphistry/tests/compute/gfql/test_engine_polars_narrow_combine.py
40+
graphistry/tests/compute/gfql/test_engine_polars_semi_key_dedup.py
41+
graphistry/tests/compute/gfql/test_engine_polars_call_modality.py
42+
graphistry/tests/compute/gfql/test_engine_polars_gpu.py
43+
graphistry/tests/compute/gfql/test_rows_table_named_middle.py
44+
graphistry/tests/compute/gfql/test_viz_pipeline_conformance.py
45+
# polars-parametrized cases inside otherwise-pandas modules: these files DO run in the
46+
# pandas lanes, but their polars/polars-gpu parameters are skipped there for want of the
47+
# wheel, so the polars lane is the only place those parameters can execute
48+
graphistry/tests/compute/gfql/index/test_indexed_bindings.py
49+
graphistry/tests/compute/gfql/test_reentry_caller_graph_immutability.py
50+
graphistry/tests/compute/gfql/test_rewrite_param_discard.py
51+
graphistry/tests/compute/test_engine_coercion.py
3252
# index tests exercise the seeded-index hook in the polars hop entry (hop.py) — without
3353
# them the hook dominates the now-thin file and trips its per-file coverage floor
3454
graphistry/tests/compute/gfql/index/test_index.py

graphistry/tests/compute/gfql/cypher/test_lowering.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15913,7 +15913,7 @@ def dtypes(self) -> Any:
1591315913
assert dict(dtypes) == {}
1591415914

1591515915

15916-
def test_node_dtypes_for_pushdown_defers_the_conversion() -> None:
15916+
def test_node_dtypes_for_pushdown_on_polars_defers_the_conversion() -> None:
1591715917
# Reading dtypes costs a full engine conversion, and only connected-join pushdown asks --
1591815918
# a path most queries never reach. Computing eagerly charged every string query for a
1591915919
# frame it discarded. Nothing may convert until a lookup actually happens.
@@ -15944,7 +15944,7 @@ def spy(*args: Any, **kwargs: Any) -> Any:
1594415944
filter_by_dict._read_node_dtypes = original
1594515945

1594615946

15947-
def test_node_dtypes_for_pushdown_matches_the_full_conversion() -> None:
15947+
def test_node_dtypes_for_pushdown_on_polars_matches_the_full_conversion() -> None:
1594815948
# polars -> pandas is DATA-dependent: an empty probe reports `bool` for a nullable Boolean
1594915949
# while the real conversion yields `object`. The gate must report what the executor sees,
1595015950
# so compare against the full conversion rather than asserting probe dtypes in isolation.

graphistry/tests/compute/gfql/index/test_indexed_bindings.py

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1110,9 +1110,17 @@ def test_use_policy_sparse_serves_dense_declines(
11101110
assert decisions[0]["reason"] == expected_reason
11111111

11121112

1113-
def test_explicit_polars_gpu_declines_indexed_helper_and_falls_back(
1114-
monkeypatch: pytest.MonkeyPatch,
1115-
) -> None:
1113+
def _cudf_polars_available() -> bool:
1114+
"""Mirrors chain.py's own gate: engine='polars-gpu' EXECUTION raises ImportError without
1115+
the RAPIDS cudf_polars stack. The helper-declines half needs no GPU at all."""
1116+
import importlib.util
1117+
1118+
return importlib.util.find_spec("cudf_polars") is not None
1119+
1120+
1121+
def test_explicit_polars_gpu_declines_indexed_helper() -> None:
1122+
"""CPU-only half: the indexed helper must decline for Engine.POLARS_GPU. Split out of the
1123+
fall-back test so it runs wherever polars does, instead of being lost behind a GPU gate."""
11161124
import graphistry.compute.gfql.index.bindings as indexed_bindings
11171125

11181126
g = _graph("polars-gpu")
@@ -1124,6 +1132,16 @@ def test_explicit_polars_gpu_declines_indexed_helper_and_falls_back(
11241132
assert indexed_bindings.try_indexed_connected_bindings_state(
11251133
g, ops, engine=Engine.POLARS_GPU
11261134
) is None
1135+
1136+
1137+
@pytest.mark.skipif(
1138+
not _cudf_polars_available(),
1139+
reason="engine='polars-gpu' execution requires the RAPIDS cudf_polars stack",
1140+
)
1141+
def test_explicit_polars_gpu_declines_indexed_helper_and_falls_back(
1142+
monkeypatch: pytest.MonkeyPatch,
1143+
) -> None:
1144+
g = _graph("polars-gpu")
11271145
_assert_parity(
11281146
g,
11291147
CONNECTED_QUERY,
Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
"""Guard: a polars-gated test may not be invisible to CI.
2+
3+
WHY THIS EXISTS (measured, not hypothetical). polars is installed in exactly ONE CI lane
4+
-- ``test-polars``, driven by ``bin/test-polars.sh`` -- and that lane runs an explicit file
5+
list. Every other lane collection-skips a module-level ``pytest.importorskip("polars")`` and
6+
per-test ``polars``/``polars-gpu`` parameters for want of the wheel. So a polars-gated test
7+
that the lane's file list omits executes NOWHERE, while both lanes report green.
8+
9+
Measured on GitHub Actions run 30311675717: 280 tests ran locally with polars installed and
10+
appeared in no CI job log at all, including whole modules named ``test_engine_polars_*``.
11+
Two real defects were sitting in that blind spot (a polars-gpu test that fails without the
12+
RAPIDS stack, and a pandas/polars ``toUpper`` divergence).
13+
14+
The rule enforced here: every test module that mentions polars is EITHER in the lane's file
15+
list OR carries a written reason for being out of it. Silence is not a valid answer.
16+
"""
17+
from __future__ import annotations
18+
19+
import ast
20+
import re
21+
from pathlib import Path
22+
from typing import Dict, List, Set, Union
23+
24+
import pytest
25+
26+
27+
REPO_ROOT = Path(__file__).resolve().parents[4]
28+
LANE_SCRIPT = REPO_ROOT / "bin" / "test-polars.sh"
29+
TEST_ROOTS = ("graphistry/tests", "tests")
30+
31+
# Second lane phase: this module runs under ``-k polars``, so only node ids containing
32+
# "polars" execute. Kept out of the main array on purpose (runtime), but its polars-gated
33+
# tests still have to be selectable -- enforced by
34+
# ``test_lowering_polars_gated_tests_are_named_so_the_k_filter_selects_them``.
35+
K_FILTERED_MODULE = "graphistry/tests/compute/gfql/cypher/test_lowering.py"
36+
37+
# This guard itself: it is pure static analysis over source text, needs no polars wheel, and
38+
# only matches its own scanners because it QUOTES the gate it looks for.
39+
SELF = "graphistry/tests/compute/gfql/test_polars_lane_completeness.py"
40+
41+
# Modules that mention polars but are NOT polars-gated, each with the reason. A new entry
42+
# here is a deliberate, reviewable statement -- which is the point of the guard.
43+
NOT_POLARS_GATED: Dict[str, str] = {
44+
"graphistry/tests/compute/gfql/cypher/test_row_pushdown.py": (
45+
"single engine-agnostic to_pandas() branch; every test runs on the pandas lane"
46+
),
47+
"graphistry/tests/test_compute_filter_by_dict.py": (
48+
"pandas oracle only; the sentence naming polars is a docstring cross-reference"
49+
),
50+
"graphistry/tests/compute/gfql/index/test_index_gpu_edge_match.py": (
51+
"cudf/GPU-gated (module-level importorskip('cudf') + skipif no GPU), not polars-gated; "
52+
"belongs to the separate GPU-lane gap, and the polars CPU lane could not run it"
53+
),
54+
}
55+
56+
_POLARS_IMPORTORSKIP = re.compile(r"""importorskip\(\s*["']polars["']""")
57+
58+
59+
def _lane_file_list() -> List[str]:
60+
"""Parse ``POLARS_TEST_FILES`` out of bin/test-polars.sh.
61+
62+
Deliberately strict: if the array cannot be found the guard fails rather than passing
63+
vacuously, so restructuring the script cannot silently disable this check.
64+
"""
65+
text = LANE_SCRIPT.read_text(encoding="utf-8")
66+
match = re.search(r"^POLARS_TEST_FILES=\((.*?)^\)", text, re.MULTILINE | re.DOTALL)
67+
assert match is not None, (
68+
f"{LANE_SCRIPT} no longer declares a POLARS_TEST_FILES=( ... ) array; this guard "
69+
"parses it as the lane's single source of truth"
70+
)
71+
files: List[str] = []
72+
for raw in match.group(1).splitlines():
73+
line = raw.split("#", 1)[0].strip()
74+
if line:
75+
files.append(line)
76+
assert files, "POLARS_TEST_FILES parsed as empty"
77+
return files
78+
79+
80+
FuncDef = Union[ast.FunctionDef, ast.AsyncFunctionDef]
81+
82+
83+
def _node_source(source_lines: List[str], node: Union[ast.stmt, ast.expr]) -> str:
84+
"""py3.8-safe source slice for an AST node (``ast.unparse`` is 3.9+)."""
85+
start: int = node.lineno
86+
end: int = node.end_lineno if node.end_lineno is not None else start
87+
return "\n".join(source_lines[max(start - 1, 0):end])
88+
89+
90+
def _selected_by_k_polars(source_lines: List[str], node: FuncDef) -> bool:
91+
"""True when ``-k polars`` can select the node: either the function name carries it, or a
92+
decorator does (a ``parametrize`` over an engine id containing 'polars' puts it in the
93+
node id, which is what ``-k`` matches against)."""
94+
if "polars" in node.name:
95+
return True
96+
return any(
97+
"polars" in _node_source(source_lines, dec) for dec in node.decorator_list
98+
)
99+
100+
101+
def _test_modules_mentioning_polars() -> Set[str]:
102+
found: Set[str] = set()
103+
for root in TEST_ROOTS:
104+
base = REPO_ROOT / root
105+
if not base.is_dir():
106+
continue
107+
for path in base.rglob("test_*.py"):
108+
if "polars" in path.read_text(encoding="utf-8").lower():
109+
found.add(path.relative_to(REPO_ROOT).as_posix())
110+
return found
111+
112+
113+
def test_lane_file_list_paths_all_exist() -> None:
114+
"""A renamed or deleted test file must not silently shrink the lane."""
115+
missing = [rel for rel in _lane_file_list() if not (REPO_ROOT / rel).is_file()]
116+
assert missing == [], f"bin/test-polars.sh lists files that do not exist: {missing}"
117+
118+
119+
def test_every_polars_mentioning_test_module_is_in_the_lane_or_justified() -> None:
120+
"""The completeness rule. New polars test file => lane entry or written exemption."""
121+
lane = set(_lane_file_list()) | {K_FILTERED_MODULE, SELF}
122+
unclassified = sorted(
123+
_test_modules_mentioning_polars() - lane - set(NOT_POLARS_GATED)
124+
)
125+
assert unclassified == [], (
126+
"these test modules mention polars but run in NO CI lane (polars is installed only "
127+
"in test-polars, which runs an explicit file list): "
128+
f"{unclassified}. Add each to POLARS_TEST_FILES in bin/test-polars.sh, or to "
129+
"NOT_POLARS_GATED here with the reason it needs no polars lane."
130+
)
131+
132+
133+
def test_not_polars_gated_entries_are_not_stale() -> None:
134+
"""An exemption for a file that is gone, or that the lane now runs, must be removed."""
135+
lane = set(_lane_file_list()) | {K_FILTERED_MODULE}
136+
gone = sorted(rel for rel in NOT_POLARS_GATED if not (REPO_ROOT / rel).is_file())
137+
assert gone == [], f"NOT_POLARS_GATED names files that no longer exist: {gone}"
138+
contradictory = sorted(set(NOT_POLARS_GATED) & lane)
139+
assert contradictory == [], (
140+
f"NOT_POLARS_GATED claims these need no polars lane, but the lane runs them: "
141+
f"{contradictory}"
142+
)
143+
144+
145+
def test_no_module_level_polars_gate_outside_the_lane() -> None:
146+
"""The sharpest form of the hole: a module-level importorskip skips the WHOLE file
147+
everywhere else, so omission from the lane costs every test in it at once."""
148+
lane = set(_lane_file_list()) | {K_FILTERED_MODULE, SELF}
149+
offenders: List[str] = []
150+
for root in TEST_ROOTS:
151+
base = REPO_ROOT / root
152+
if not base.is_dir():
153+
continue
154+
for path in base.rglob("test_*.py"):
155+
rel = path.relative_to(REPO_ROOT).as_posix()
156+
if rel in lane:
157+
continue
158+
source = path.read_text(encoding="utf-8")
159+
source_lines = source.splitlines()
160+
for node in ast.parse(source).body: # module level only
161+
if _POLARS_IMPORTORSKIP.search(_node_source(source_lines, node)):
162+
offenders.append(rel)
163+
break
164+
assert offenders == [], (
165+
"module-level pytest.importorskip('polars') outside the polars lane -- every test in "
166+
f"these files is collection-skipped in every CI lane: {offenders}"
167+
)
168+
169+
170+
def test_lowering_polars_gated_tests_are_named_so_the_k_filter_selects_them() -> None:
171+
"""``test_lowering.py`` runs under ``-k polars``. A test that calls
172+
``importorskip("polars")`` but has no 'polars' in its function name is deselected in the
173+
polars lane and skipped in the pandas lanes -- i.e. it runs nowhere."""
174+
path = REPO_ROOT / K_FILTERED_MODULE
175+
source = path.read_text(encoding="utf-8")
176+
source_lines = source.splitlines()
177+
offenders = [
178+
node.name
179+
for node in ast.parse(source).body
180+
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
181+
and node.name.startswith("test_")
182+
and not _selected_by_k_polars(source_lines, node)
183+
and _POLARS_IMPORTORSKIP.search(_node_source(source_lines, node))
184+
]
185+
assert offenders == [], (
186+
f"{K_FILTERED_MODULE} is run with `-k polars`; these polars-gated tests are not "
187+
f"selected by that filter and therefore run in no lane: {offenders}. Put 'polars' in "
188+
"the test name (or parametrize it over a 'polars' engine id)."
189+
)
190+
191+
192+
@pytest.mark.parametrize("rel", sorted(NOT_POLARS_GATED))
193+
def test_exemption_reasons_are_substantive(rel: str) -> None:
194+
"""An empty or placeholder reason would re-open the hole under the guard's own cover."""
195+
reason = NOT_POLARS_GATED[rel]
196+
assert len(reason) >= 40, f"exemption for {rel} needs a real reason, got {reason!r}"

graphistry/tests/compute/gfql/test_viz_pipeline_conformance.py

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -466,8 +466,50 @@ def _trick_matrix():
466466
]
467467

468468

469-
@pytest.mark.parametrize("label,query,pinned,mirror", _trick_matrix(),
470-
ids=[t[0] for t in _trick_matrix()])
469+
def _pandas_str_accessor_is_full_case_mapping() -> bool:
470+
"""pandas<3 stores text as object and delegates .str.upper()/.lower() to Python, which
471+
applies FULL Unicode case mapping ('straße' -> 'STRASSE', 'İ' -> 'i̇').
472+
pandas>=3 backs the default `str` dtype with Arrow, whose utf8_upper/utf8_lower are
473+
SIMPLE per-codepoint mappings ('straße' -> 'STRAẞE', 'İ' -> 'i')."""
474+
upper = pd.Series(["straße"]).str.upper().iloc[0]
475+
lower = pd.Series(["İ"]).str.lower().iloc[0]
476+
return bool(upper == "STRASSE" and lower == "i̇")
477+
478+
479+
# GFQL's pandas toLower/toUpper delegate to that accessor, so under pandas>=3 they return
480+
# SIMPLE mappings while the polars engine (Rust to_uppercase/to_lowercase) and Cypher itself
481+
# (neo4j toUpper == Java String.toUpperCase) return FULL ones. That is a silent cross-engine
482+
# divergence, not a decline: pandas answers [9] where polars answers [8, 9]. Tracked in
483+
# https://github.com/graphistry/pygraphistry/issues/1802 — the hand pins below stay at the
484+
# Cypher-correct values, so `strict=True` turns the fix into an XPASS that retires this mark.
485+
_SIMPLE_CASE_MAPPING_XFAIL = pytest.mark.xfail(
486+
not _pandas_str_accessor_is_full_case_mapping(),
487+
reason=(
488+
"pandas>=3 Arrow-backed str dtype does SIMPLE case mapping; GFQL pandas toLower/"
489+
"toUpper inherit it and diverge from the polars engine and from Cypher (#1802)"
490+
),
491+
strict=True,
492+
)
493+
494+
_SIMPLE_CASE_MAPPING_LABELS = frozenset({"toupper-eq-ss-fold", "tolower-turkish-dotted-i"})
495+
496+
497+
def _trick_params():
498+
return [
499+
pytest.param(
500+
*case,
501+
id=case[0],
502+
marks=(
503+
[_SIMPLE_CASE_MAPPING_XFAIL]
504+
if case[0] in _SIMPLE_CASE_MAPPING_LABELS
505+
else []
506+
),
507+
)
508+
for case in _trick_matrix()
509+
]
510+
511+
512+
@pytest.mark.parametrize("label,query,pinned,mirror", _trick_params())
471513
def test_case_regex_unicode_trick_matrix(label, query, pinned, mirror):
472514
g = _panel_graph()
473515
nd = _panel_nodes()

0 commit comments

Comments
 (0)