Skip to content

Extract test set crud module and retire the split - #2620

Merged
akwasigroch merged 2 commits into
mainfrom
crud-extract-testset
Aug 27, 2026
Merged

Extract test set crud module and retire the split#2620
akwasigroch merged 2 commits into
mainfrom
crud-extract-testset

Conversation

@akwasigroch

Copy link
Copy Markdown
Collaborator

Purpose

Finishes the crud monolith split that #2410 started. TestSet was the last entity left in crud/__init__.py, so after this the file is empty and app/crud/ is simply one module per entity. The second half of the PR removes the now-false "split in progress" narration from the codebase.

Stacked on #2617 (Test extraction) — review and merge that one first; this PR's base is crud-extract-test, and the diff shown here is just the TestSet half.

What Changed

The move. All nine TestSet functions (get_test_set, get_test_sets, create_test_set, update_test_set, delete_test_set, get_test_set_by_nano_id_or_slug, resolve_test_set, get_test_sets_for_test, get_test_set_tests) go into crud/test_set.py, joining the bulk_delete_test_sets that already lived there. _TEST_SET_RELATED_FIELDS moves with them, so crud/explorer.py now imports it from crud.test_set rather than from the parent package. Around 90 call sites across routers, services, jobs and tests switch to test_set_crud., and the five patch("…app.crud.resolve_test_set") targets in tests/backend/services/test_test_set.py become …app.crud.test_set.resolve_test_set.

crud/__init__.py is now a single docstring line. That also takes away the names it had been re-exporting by accident, and four modules turned out to be leaning on them. services/experiment.py reached crud.get_item_detail and crud.include, and routers/experiments.py reached crud.delete_item — all three are crud_utils/query_utils names that only resolved because __init__.py happened to import them. jobs/execution/modes.py reached schemas through crud.schemas.TestConfigurationUpdate. Each now imports from where the thing actually lives. services/tool/mcp/operations.py had a crud import that nothing in the file used.

One real bug fixed. crud.delete_test_configuration does not exist anywhere in the repo — the function was dropped when test_configuration was extracted, so DELETE /test_configurations/{id} has been raising AttributeError on every call since. This adds delete_test_configuration to crud/test_configuration.py (a delete_item wrapper, matching its siblings) and points the router at it. No test covered the route, which is why it went unnoticed.

The history comes out (second commit, reviewable on its own). The "Part of the incremental split of the crud monolith: crud/__init__.py still holds…" paragraph is gone from all 29 crud module docstrings; metric.py and telemetry.py keep their "import the functions directly" guidance but lose the "Split out of crud/__init__.py" line; the monolith comment above explorer.py's _TEST_SET_RELATED_FIELDS import is rewritten. AGENTS.md's CRUD layout rule now describes the finished layout — one module per entity, __init__.py empty on purpose, a new function goes in its entity's module — instead of a migration in progress. Two stale "doesn't exist in crud.py" comments in tests and three crud.foo(...) docstring examples in utils/database_exceptions.py are reworded to the module form so the docs stop teaching the pattern that no longer works.

tests/backend/test_secret_equality.py pins its allowlist by line number, and two added imports in services/experiment.py shifted the two whitelisted content_hash comparisons from 150/206 to 152/208; the entries are repinned.

Additional Context

Testing

  • Full backend suite green on this branch: 8234 passed, 62 skipped, 1 xfailed, 0 failed (5m37s). Same counts on Extract test crud module #2617's branch alone, and the 62 skips are the pre-existing ones (DEFAULT_EVALUATION_MODEL not configured, the rollback_initial_data OOM hang).
  • Verified every module under rhesis.backend.app and rhesis.backend.jobs still imports, and that no crud.foo() package-level access survives anywhere in apps/ or tests/.
  • ruff check and ruff format clean on all changed files.
  • DELETE /test_configurations/{id} is worth exercising by hand, since nothing in the suite covers it.

@peqy

peqy Bot commented Aug 27, 2026

Copy link
Copy Markdown

Looks good — nice cleanup finishing the crud split, and good catch restoring delete_test_configuration.

[Improvement] Add a small router-level test for DELETE /test_configurations/{id}

Fix: add a minimal happy-path + 404 test to prevent the AttributeError regression from coming back (and to exercise tenant scoping in the route).

Base automatically changed from crud-extract-test to main August 27, 2026 09:52
Arkadiusz Kwasigroch added 2 commits August 27, 2026 11:54
The crud package is one module per entity now, so the "part of the
incremental split, crud/__init__.py still holds the monolith" paragraph
in every module docstring describes a state that no longer exists. Same
for the AGENTS.md rule, which now states the finished layout instead of
a migration in progress.
Finishes the crud split. The TestSet functions move into crud/test_set.py
and crud/__init__.py is left empty, so callers switch to direct submodule
imports.

Emptying __init__.py also removes the names it re-exported by accident,
which several modules were relying on: crud.get_item_detail, crud.include
and crud.delete_item are crud_utils/query_utils names, and
crud.schemas.TestConfigurationUpdate reached schemas through the package.
Each now imports from its real home.

Also restores delete_test_configuration, which went missing when
test_configuration was extracted -- DELETE /test_configurations/{id} has
been raising AttributeError since.

test_service_security.py passed the crud package itself into its table of
functions to check for an organization_id parameter. The loop guards with
hasattr, so an empty __init__.py would not fail it -- the five test set
assertions would just quietly stop checking anything. Points them at
test_set_crud.
@akwasigroch
akwasigroch force-pushed the crud-extract-testset branch from 13d51b6 to 7137cc8 Compare August 27, 2026 09:58

@peqy peqy Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-reviewed latest commits — the TestSet CRUD extraction and cleanup still look solid, and the delete_test_configuration fix is wired through the router correctly.

Only remaining suggestion is the same as before: please add a minimal router-level test for DELETE /test_configurations/{id} (happy path + 404) so this regression can’t sneak back in.

@akwasigroch
akwasigroch merged commit 96fde24 into main Aug 27, 2026
16 of 17 checks passed
@akwasigroch
akwasigroch deleted the crud-extract-testset branch August 27, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant