Skip to content

Clean up legacy test scaffolding in cudf-polars#22535

Open
madsbk wants to merge 1 commit into
rapidsai:release/26.06from
madsbk:cleanup_legacy_tests
Open

Clean up legacy test scaffolding in cudf-polars#22535
madsbk wants to merge 1 commit into
rapidsai:release/26.06from
madsbk:cleanup_legacy_tests

Conversation

@madsbk
Copy link
Copy Markdown
Member

@madsbk madsbk commented May 16, 2026

Description

Removes assert_collect_raises, its callers, and the leftover executor=-era keyword plumbing that is no longer used anywhere else.

@madsbk madsbk self-assigned this May 16, 2026
@madsbk madsbk added improvement Improvement / enhancement to an existing function breaking Breaking change labels May 16, 2026
@github-actions github-actions Bot added Python Affects Python cuDF API. cudf-polars Issues specific to cudf-polars labels May 16, 2026
engine: GPUEngine,
collect_kwargs: CollectKwargs | None = None,
polars_collect_kwargs: CollectKwargs | None = None,
cudf_collect_kwargs: CollectKwargs | None = None,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

never used

@GPUtester GPUtester moved this to In Progress in cuDF Python May 16, 2026
Comment on lines -161 to -174
def _process_kwargs(
collect_kwargs: CollectKwargs | None,
polars_collect_kwargs: CollectKwargs | None,
cudf_collect_kwargs: CollectKwargs | None,
) -> tuple[CollectKwargs, CollectKwargs]:
if collect_kwargs is None:
collect_kwargs = {}
final_polars_collect_kwargs = collect_kwargs.copy()
final_cudf_collect_kwargs = collect_kwargs.copy()
if polars_collect_kwargs is not None: # pragma: no cover; not currently used
final_polars_collect_kwargs.update(polars_collect_kwargs)
if cudf_collect_kwargs is not None: # pragma: no cover; not currently used
final_cudf_collect_kwargs.update(cudf_collect_kwargs)
return final_polars_collect_kwargs, final_cudf_collect_kwargs
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Inlining, the function is only called once now.

return final_polars_collect_kwargs, final_cudf_collect_kwargs


def assert_collect_raises(
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Inlining the function is simpler to understand IMO. We now just do:

-        assert_collect_raises(
-            q,
-            cudf_except=pl.exceptions.ComputeError,
-            polars_except=pl.exceptions.InvalidOperationError,
-        )
+        with pytest.raises(pl.exceptions.InvalidOperationError):
+            q.collect()
+        with pytest.raises(pl.exceptions.ComputeError):
+            q.collect(engine=pl.GPUEngine(executor="in-memory", raise_on_fail=True))

@madsbk madsbk marked this pull request as ready for review May 16, 2026 10:36
@madsbk madsbk requested a review from a team as a code owner May 16, 2026 10:36
@madsbk madsbk requested a review from nirandaperera May 16, 2026 10:36
@rapidsai rapidsai deleted a comment from copy-pr-bot Bot May 16, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Simplified internal testing infrastructure and assertion utilities for improved code maintainability.

Walkthrough

This PR simplifies the cuDF Polars test assertion API by removing the cudf_collect_kwargs parameter from assert_gpu_result_equal, merging CPU/GPU kwargs logic, and deleting the assert_collect_raises helper. All test files are updated to use direct pytest.raises blocks for exception assertions.

Changes

Assertion helper refactor and test migration

Layer / File(s) Summary
Assertion utilities refactoring
python/cudf_polars/cudf_polars/testing/asserts.py
assert_gpu_result_equal removes cudf_collect_kwargs parameter; CPU kwargs are built by merging collect_kwargs with polars_collect_kwargs, GPU kwargs come from collect_kwargs alone. Private helper _process_kwargs and public helper assert_collect_raises are deleted.
Test migration to direct pytest.raises
python/cudf_polars/tests/expressions/test_casting.py, test_datetime_basic.py, test_stringfunction.py, testing/test_asserts.py
Five test modules remove assert_collect_raises imports and replace each call with explicit pytest.raises(...) blocks around both q.collect() and q.collect(engine=pl.GPUEngine(...)) paths. The test_collect_assert_raises() test is removed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Possibly related PRs

  • rapidsai/cudf#22493: Both PRs refactor asserts.py's assertion utilities and engine-related calling conventions in assert_gpu_result_equal.

Suggested labels

non-breaking


Suggested reviewers

  • mroeschke
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removal of legacy test scaffolding including assert_collect_raises and related keyword arguments.
Description check ✅ Passed The description is directly related to the changeset, clearly explaining what is being removed and why.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change cudf-polars Issues specific to cudf-polars improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants