Skip to content

Remove low-value tests from the remaining test suite #4588

Description

@snopoke

Goal

Remove low-value tests from the rest of the suite, a few files per PR. These are mostly tests written by agents to verify their own changes rather than to protect behaviour: they restate the implementation, assert nothing that could fail for a real reason, or duplicate a test that already exists.

PR #4587 did the first batch (51 tests, 663 lines) and is the worked example — read its description for the reasoning before starting a task.

Each task names 1-3 test files. That is deliberately small: the removals are judgement calls, and a PR touching a few files can be checked line by line.

Context

The test that a test has to fail

Remove a test if deleting it cannot make a real bug pass. That is the whole criterion. The categories below are what it looks like in this codebase:

Mock-delegation change detectors. Everything is mocked, and the only assertion restates the body of the method under test. The clearest case was apps/ocs_notifications/tests/test_notifications.py, where nine tests each mocked create_notification and asserted the exact kwargs dict the thin wrapper passes it. Same shape wherever a sender or client method forwards its arguments to a service:

def test_send_text(self):
    service = MagicMock()
    sender = FacebookMessengerSender(service=service, page_id=PAGE_ID)
    sender.bind(_bound_context())
    sender.send_text("hi", recipient=RECIPIENT)
    service.send_text_message.assert_called_once_with(message="hi", from_=PAGE_ID, ...)

Tautological. Asserting a no-op does nothing; isinstance checks on a one-line factory that constructs exactly that class; pydantic or dataclass tests asserting the constructor stores what you passed it; asserting a class attribute equals its declared value.

Duplicates and strict subsets. Two tests with identical bodies and different names, or one test whose setup and assertions are wholly contained in another. test_task_legacy_payload_falls_back_to_routing was byte-identical to test_routes_and_processes_message apart from its docstring.

Change-verification artifacts. A test that asserts the absence of a string, class or behaviour that no longer exists anywhere in the repo. Grep for the thing being asserted absent — if the test file is the only hit, the test is a fossil of a past change. Two tests asserted "Messages to clone" not in content for copy that had already been deleted.

Do not remove

These look similar to the categories above but earn their place:

  • Permission and auth checks. A 401/403/404 assertion with no body assertion is exactly what you want pinned. There are ~240 status-code-only tests and almost all are this.
  • Assertions against an external API's call shape. provider_client.vector_stores.files.delete.assert_called_once_with(...) is a contract with OpenAI, not a restatement of our code.
  • "Does not raise" tests where the code would genuinely raise without a guard — an early-return on a None session, a swallowed exception, a no-op for a deleted row.
  • Registry and consistency checks. assert {q.name for q in settings.CELERY_TASK_QUEUES} == {m.value for m in Queues}, LOADERS[SourceType.JSON_COLLECTION] is JSONCollectionLoader, for tool in AgentTools.values: assert tool in TOOL_CLASS_MAP. These catch a real class of mistake.
  • Query-count tests (django_assert_num_queries) — N+1 guards, even with no other assertion.
  • Documented regression guards. A test whose docstring cites an issue number and explains what it stops from coming back is a human decision. Leave it.
  • A worthless assertion on top of a valuable call. Separate the two. If the test builds real model instances and calls the real function, and only the terminal collaborator is mocked, then the call itself is doing work even when the assertion is a kwargs transcription. This matters most where the function under test is wrapped in @silence_exceptions (apps/utils/decorators.py), which catches Exception and only logs: a renamed attribute there stops the feature with nothing failing anywhere. Before removing such a test, grep for other call sites — if they all patch the function, that test is the only thing executing its body. When the assertion is worthless but the call is not, delete the assertion and keep a thin test that asserts the function reaches its collaborator. See Remove low-value tests #4587 and commit e3fba3c, where this was got wrong first and corrected on review; it applies directly to Tasks 18, 21 and 28.

Out of scope

  • Do not fold near-duplicates into pytest.mark.parametrize. Two tests that share structure but cover genuinely distinct scenarios are both pulling their weight. Folding them is a refactor, it makes the diff much harder to review, and it is not what these PRs are for. The one exception is when you are already deleting one of a pair and the survivors are trivially mergeable.
  • Do not touch production code. If removing a test reveals an untested branch worth covering, note it in the PR description rather than writing the test.
  • Do not add tests.
  • Files not named in a task. The 465 test files not listed below were scored and fall under the threshold — typically one or two short tests each. They are not worth a PR.

It is fine to remove nothing

If a task's files turn out to be sound, close the task with a comment saying so and check the box. A PR that removes two tests is a good outcome; a PR that removes fifteen because the quota felt low is not.

Finding candidates

Parse each test file with ast and flag test functions where:

  • the body contains no assert, self.assert*, pytest.raises or assertRaises at all;
  • the only assertions are mock assertions (assert_called_once_with, assert_not_called, ...) and the mock is standing in for the collaborator the method under test merely forwards to;
  • the body is <= 4 lines with at most one assertion;
  • two test bodies in the same file hash identically after stripping docstrings.

The counts in each task come from exactly those four flags. They are a shortlist to read, not a verdict — in the first batch ~150 flagged tests were read and 51 removed. Every removal needs a judgement call about whether the behaviour is covered elsewhere; check that before deleting, and say so in the PR description when it is the reason.

Per-task workflow

  1. Work only the files named in the task.
  2. Delete whole test functions or classes. If a class is left empty, remove the class too.
  3. uv run inv ruff --paths <the test dirs you touched> — this cleans up imports left unused by the deletion.
  4. uv run pytest <the apps you touched> -q and confirm it is green.
  5. In the PR description, state the criterion and list the judgement calls — which removals depended on coverage existing elsewhere, and which behaviours are now untested. That is the part a reviewer cannot get from a diff of deletions.

Tasks

Task 1: service_providers — index managers and 2 more

  • Task 1

Most of the mock assertions in test_index_managers.py are against the OpenAI SDK call shape — that is a contract with a third party, not a restatement of our code. Keep them. The 6 tiny tests are the likelier candidates. test_llm_integration.py is the densest single file in the repo by no-assertion count.

  • apps/service_providers/tests/test_index_managers.py — 51 tests; 9 mock-only, 6 tiny
  • apps/service_providers/tests/test_llm_integration.py — 9 tests; 9 no-assertion
  • apps/service_providers/tests/test_models.py — 32 tests; 4 mock-only, 2 no-assertion, 5 tiny

Task 2: channels — email channel and 2 more

  • Task 2

The inbound-handler filter tests in test_email_channel.py (spam, disallowed domain, wildcard match) are security-relevant — keep them. #4587 already removed the one exact duplicate at the task level.

  • apps/channels/tests/test_email_channel.py — 87 tests; 12 mock-only, 1 no-assertion, 5 tiny
  • apps/channels/tests/test_whatsapp_integration.py — 18 tests; 6 mock-only
  • apps/channels/tests/test_widget_versions.py — 30 tests; 17 tiny

Task 3: pipelines — nodes and 2 more

  • Task 3

  • apps/pipelines/tests/test_nodes.py — 36 tests; 9 mock-only, 4 tiny

  • apps/pipelines/tests/test_node_context.py — 37 tests; 22 tiny

  • apps/pipelines/tests/test_node_type.py — 33 tests; 21 tiny

Task 4: web — json tags and 2 more

  • Task 4

Leave apps/web/tests/test_app_shell_layout.py alone — it is a documented guard against #4408 regressing.

  • apps/web/tests/test_json_tags.py — 50 tests; 28 tiny
  • apps/web/tests/test_elevation_views.py — 23 tests; 5 no-assertion, 5 tiny
  • apps/web/tests/test_waf_analysis.py — 28 tests; 11 tiny

Task 5: service_providers — voice providers and 2 more

  • Task 5

  • apps/service_providers/tests/test_voice_providers.py — 33 tests; 7 no-assertion, 1 tiny

  • apps/service_providers/tests/test_messaging_providers.py — 47 tests; 3 mock-only, 1 no-assertion, 4 tiny

  • apps/service_providers/tests/test_obfusticating_form.py — 6 tests; 3 no-assertion, 5 tiny

Task 6: evaluations — evaluation coordination and 2 more

  • Task 6

  • apps/evaluations/tests/test_evaluation_coordination.py — 43 tests; 6 mock-only, 1 no-assertion, 2 tiny

  • apps/evaluations/tests/test_tagging_logic.py — 21 tests; 1 no-assertion, 15 tiny

  • apps/evaluations/tests/test_aggregations.py — 21 tests; 7 tiny

Task 7: channels — models and 2 more

  • Task 7

test_sending_error_handler.py was read in #4587 and kept in full; the mock assertions there cover a real handler chain. Re-read before cutting.

  • apps/channels/tests/test_models.py — 23 tests; 1 no-assertion, 11 tiny
  • apps/channels/tests/test_whatsapp_attachments.py — 38 tests; 14 tiny
  • apps/channels/tests/channels/stages/test_sending_error_handler.py — 7 tests; 4 mock-only, 2 tiny

Task 8: chat — pipeline bot and 2 more

  • Task 8

  • apps/chat/tests/test_pipeline_bot.py — 12 tests; 5 mock-only

  • apps/chat/tests/test_openapi_executor.py — 7 tests; 4 no-assertion

  • apps/chat/tests/test_tools.py — 46 tests; 11 tiny

Task 9: service_providers — views and 2 more

  • Task 9

  • apps/service_providers/tests/test_views.py — 58 tests; 4 mock-only, 1 tiny

  • apps/service_providers/tests/test_migrate_twilio_webhooks.py — 7 tests; 4 mock-only

  • apps/service_providers/tests/test_ocs_tracer.py — 21 tests; 1 mock-only, 3 no-assertion

Task 10: channels — persistence and 2 more

  • Task 10

All three files were reviewed in #4587. test_pipeline_orchestrator.py lost two tests and the rest were kept deliberately; test_slack_sender.py lost two. Re-read rather than assume there is more to take.

  • apps/channels/tests/channels/stages/test_persistence.py — 9 tests; 3 mock-only, 1 no-assertion, 1 tiny
  • apps/channels/tests/channels/test_pipeline_orchestrator.py — 20 tests; 4 mock-only
  • apps/channels/tests/channels/senders/test_slack_sender.py — 4 tests; 4 mock-only

Task 11: experiments — views and 2 more

  • Task 11

Read docs/agents/django_model_versioning.md before touching test_versioning.py. Versioning behaviour is subtle and the tests are load-bearing.

  • apps/experiments/tests/test_views.py — 23 tests; 4 mock-only, 1 no-assertion, 1 tiny
  • apps/experiments/tests/test_models.py — 60 tests; 1 no-assertion, 8 tiny
  • apps/experiments/tests/test_versioning.py — 27 tests; 1 no-assertion, 5 tiny

Task 12: utils — restricted http and 2 more

  • Task 12

test_restricted_http.py is SSRF protection. Be conservative — a tiny test of one blocked address range is still pinning a security boundary.

  • apps/utils/tests/test_restricted_http.py — 49 tests; 1 mock-only, 2 no-assertion, 14 tiny
  • apps/utils/tests/test_rate_limit.py — 53 tests; 6 tiny
  • apps/utils/tests/test_time.py — 5 tests; 5 tiny

Task 13: channels — stage span status and 2 more

  • Task 13

test_stage_span_status.py was read in full in #4587 and kept — its mock assertions are against span state, which is the behaviour under test.

  • apps/channels/tests/channels/stages/test_stage_span_status.py — 8 tests; 4 mock-only
  • apps/channels/tests/channels/senders/test_telegram_sender.py — 5 tests; 3 mock-only, 2 tiny
  • apps/channels/tests/test_connect_channel_v2.py — 8 tests; 3 mock-only

Task 14: service_providers — whatsapp provider and 2 more

  • Task 14

  • apps/service_providers/tests/test_whatsapp_provider.py — 22 tests; 2 mock-only, 1 no-assertion, 1 tiny

  • apps/service_providers/tests/test_twilio_webhook_management.py — 7 tests; 3 mock-only, 1 tiny

  • apps/service_providers/tracing/tests/test_ocs_tracer_cost.py — 10 tests; 3 mock-only

Task 15: pipelines — repository and 2 more

  • Task 15

  • apps/pipelines/tests/test_repository.py — 26 tests; 12 tiny

  • apps/pipelines/tests/test_jinja_validation.py — 30 tests; 9 tiny

  • apps/pipelines/tests/test_runnable_builder.py — 41 tests; 1 no-assertion, 5 tiny

Task 16: usage_metrics — characterisation and 1 more

  • Task 16

The file is named test_characterisation.py. Check whether these tests deliberately pin current behaviour before removing any; that is a legitimate reason for a thin assertion.

  • apps/usage_metrics/tests/test_characterisation.py — 25 tests; 18 tiny
  • apps/usage_metrics/tests/test_equality.py — 16 tests; 8 tiny

Task 17: channels — webhooks and 2 more

  • Task 17

  • apps/channels/tests/test_webhooks.py — 7 tests; 3 mock-only

  • apps/channels/tests/test_datamodels.py — 17 tests; 8 tiny

  • apps/channels/tests/test_turn_webhook.py — 17 tests; 8 tiny

Task 18: data_migrations — notify deprecated widget versions and 2 more

  • Task 18

  • apps/data_migrations/tests/test_notify_deprecated_widget_versions.py — 9 tests; 4 mock-only, 1 tiny

  • apps/data_migrations/tests/test_notify_deprecated_models.py — 7 tests; 2 mock-only

  • apps/data_migrations/tests/test_notify_widget_version_release.py — 6 tests; 1 mock-only, 2 tiny

Task 19: help — checks and 2 more

  • Task 19

apps/help/evals/ holds eval tests, not unit tests — see docs/developer_guides/testing/help_agent_evals.md before touching them.

  • apps/help/evals/test_checks.py — 20 tests; 14 tiny
  • apps/help/tests/test_filter_agent.py — 16 tests; 1 mock-only, 2 tiny
  • apps/help/tests/test_help.py — 31 tests; 5 tiny

Task 20: channels — meta cloud api webhook and 2 more

  • Task 20

  • apps/channels/tests/test_meta_cloud_api_webhook.py — 18 tests; 1 mock-only, 5 tiny

  • apps/channels/tests/channels/stages/test_span_field_summary.py — 12 tests; 8 tiny

  • apps/channels/tests/channels/stages/test_session_activation.py — 4 tests; 2 mock-only, 1 tiny

Task 21: ocs_notifications — notifications and 1 more

  • Task 21

#4587 already cut test_notifications.py from 19 tests to 9. The survivors were kept for stated reasons (session-less branch, slug and title derivation, link de-collision, message composition). Read that PR before cutting further.

  • apps/ocs_notifications/tests/test_notifications.py — 9 tests; 4 mock-only, 2 tiny
  • apps/ocs_notifications/tests/test_slack_notification.py — 12 tests; 2 mock-only, 2 tiny

Task 22: service_providers — output parsing and 2 more

  • Task 22

  • apps/service_providers/tests/test_output_parsing.py — 19 tests; 8 tiny

  • apps/service_providers/tests/test_speech_integration.py — 6 tests; 2 no-assertion

  • apps/service_providers/tests/test_langfuse_tracer.py — 9 tests; 2 mock-only

Task 23: channels — chat message creation and 2 more

  • Task 23

  • apps/channels/tests/channels/stages/test_chat_message_creation.py — 8 tests; 2 mock-only, 1 tiny

  • apps/channels/tests/test_tasks.py — 2 tests; 2 mock-only

  • apps/channels/tests/test_utils.py — 11 tests; 6 tiny

Task 24: channels — consent check and 2 more

  • Task 24

test_api_channel.py was cut in #4587. The two remaining mock-only tests cover a real version-metadata conditional — they were kept on purpose.

  • apps/channels/tests/channels/stages/test_consent_check.py — 7 tests; 2 no-assertion
  • apps/channels/tests/channels/stages/test_attachment_hydration.py — 16 tests; 6 tiny
  • apps/channels/tests/channels/concrete/test_api_channel.py — 15 tests; 2 mock-only

Task 25: service_providers — model parameters and 2 more

  • Task 25

  • apps/service_providers/tests/llm_service/test_model_parameters.py — 9 tests; 6 tiny

  • apps/service_providers/tracing/tests/test_tracing_service_sample_rate.py — 4 tests; 2 mock-only

  • apps/service_providers/tests/test_file_limits.py — 19 tests; 5 tiny

Task 26: pipelines — pipeline runs and 2 more

  • Task 26

  • apps/pipelines/tests/test_pipeline_runs.py — 3 tests; 2 no-assertion

  • apps/pipelines/tests/test_versioning_registry.py — 7 tests; 1 no-assertion, 2 tiny

  • apps/pipelines/tests/test_flow.py — 10 tests; 5 tiny

Task 27: documents — tasks and 2 more

  • Task 27

  • apps/documents/tests/test_tasks.py — 28 tests; 2 mock-only

  • apps/documents/test_readers.py — 15 tests; 1 mock-only, 2 tiny

  • apps/documents/tests/test_json_collection_loader.py — 32 tests; 5 tiny

Task 28: events — migration lock firing and 1 more

  • Task 28

  • apps/events/tests/test_migration_lock_firing.py — 4 tests; 3 mock-only

  • apps/events/tests/test_static_trigger.py — 4 tests; 1 mock-only, 1 no-assertion

Task 29: chatbots — test_broadcast.py

  • Task 29

  • apps/chatbots/tests/test_broadcast.py — 22 tests; 5 mock-only

Task 30: web — health checks and 1 more

  • Task 30

  • apps/web/tests/test_health_checks.py — 16 tests; 1 mock-only, 6 tiny

  • apps/web/tests/test_elevation.py — 17 tests; 5 tiny

Task 31: api — chatbots inspect and 1 more

  • Task 31

test_chatbots_inspect.py contains django_assert_num_queries tests with no other assertion. Those are N+1 guards and stay.

  • apps/api/v2/tests/test_chatbots_inspect.py — 24 tests; 1 no-assertion, 6 tiny
  • apps/api/tests/test_chat_session_token.py — 23 tests; 5 tiny

Task 32: custom_actions — schema utils and 1 more

  • Task 32

  • apps/custom_actions/tests/test_schema_utils.py — 14 tests; 7 tiny

  • apps/custom_actions/tests/test_validate_api_schema.py — 5 tests; 1 no-assertion, 3 tiny

Task 33: files — test_content_type.py

  • Task 33

  • apps/files/tests/test_content_type.py — 14 tests; 12 tiny

Task 34: teams — importer transforms and 1 more

  • Task 34

  • apps/teams/export/tests/test_importer_transforms.py — 8 tests; 6 tiny

  • apps/teams/export/tests/test_command.py — 28 tests; 1 no-assertion, 2 tiny

Task 35: channels — web channel and 1 more

  • Task 35

test_web_channel.py was cut in #4587; the remaining mock-only tests cover a real branch.

  • apps/channels/tests/channels/concrete/test_web_channel.py — 14 tests; 2 mock-only
  • apps/channels/tests/test_credential_mode.py — 10 tests; 5 tiny

Task 36: service_providers — usages and 1 more

  • Task 36

  • apps/service_providers/tests/test_usages.py — 38 tests; 5 tiny

  • apps/service_providers/tests/test_message_splitting.py — 10 tests; 5 tiny

Task 37: cost_tracking — test_estimation.py

  • Task 37

  • apps/cost_tracking/tests/test_estimation.py — 13 tests; 9 tiny

Task 38: annotations — test_tags.py

  • Task 38

  • apps/annotations/tests/test_tags.py — 13 tests; 2 no-assertion, 1 tiny

Task 39: human_annotations — test_views.py

  • Task 39

test_views.py had 5 tests removed in #4587. The 6 remaining tiny ones are a thin seam — check whether anything is left worth taking before opening a PR, and close the task with a comment if not.

  • apps/human_annotations/tests/test_views.py — 109 tests; 6 tiny

Task 40: chat — test_chat_tags.py

  • Task 40

  • apps/chat/tests/test_chat_tags.py — 8 tests; 5 tiny

Learnings

  • Task 1 (service_providers) merged as Remove low-value tests from service_providers (issue #4588, task 1) #4604 but its checkbox was left unticked. Ticked while doing task 2.

  • The three files a task names are not equally productive, and the flag counts do not predict which. In task 2, test_whatsapp_integration.py yielded nothing: its six mock-only tests each drive a real Celery task through parsing, routing, participant resolution and the pipeline, mocking only the terminal messaging service, so send_text_message.assert_called() is the observable end of a real run rather than a restatement of a forwarding method.

  • A recurring strict-subset shape in this codebase: a file has both a TestXTask class and a TestXEndToEnd class, and the end-to-end test is the task test's body plus one extra assertion. test_email_channel.py had three copies of that shape; Remove low-value tests #4587 took one, task 2 took the second.

  • A thin assert x is not None on a property that is only a platform guard plus a delegation is removable once you can name the test pinning the guard and the test pinning the delegation. In test_widget_versions.py both were in the same class.

  • Tooling note for headless runs: gh issue view and shell redirection/heredocs are blocked in the sandbox, but pipes are not. gh api repos/<repo>/issues/<n> --jq .body | diff - <file> verifies a reconstructed body before gh issue edit <n> --body-file <file> writes it back. The body file must live inside the repo directory.

  • Tautology has a mechanical test: read the implementation of both sides of the assertion. In task 3, set(NodeType(t).default_params()) <= NodeType(t).declared_params and for t in node_types_declaring(p): assert NodeType(t).declares(p) each compare two expressions that read the same source (node_class.model_fields, and declares itself) — neither can fail for any input. A consistency check is only worth keeping when one side is hand-written, as server_managed_node_types() is.

  • Accessor-class test files (test_node_context.py) come in happy-path/default-path pairs, and within a pair both are load-bearing: if the property returned the default unconditionally the default test would still pass. The removable half is the pair whose property has no default at all — a plain state["key"] read — and only once you can name the real test that runs it. context.input, context.inputs and context.session are each read by Passthrough/BooleanNode/CodeNode, so every pipeline run in test_runnable_builder.py covers them.

  • A null-object test that parametrizes the whole surface (TestNullObject.test_whole_surface_answers) makes every other unknown-type test in the file a strict subset. Whole test functions asserting only the unknown-type answer go; single parametrize rows asserting it inside a domain table stay, because pulling rows out of a table is a refactor.

  • Two tests can be duplicates through a field default rather than through identical bodies. test_static_email_still_works built a SendEmail without body, test_empty_body_defaults_to_input passed body=""; the pydantic default made them the same scenario with a different subject string.

  • A security-boundary test file can be entirely off-limits even when the heuristic flags a third of it. test_elevation_views.py yielded nothing: its 5 "no-assertion" flags were all assertRedirects, which the AST scan does not recognise as an assertion, and every test pins a status code or redirect for a role, a stale elevation stash, a lost role, a concurrency cap or an expiry. Check what the flagged assertion helper actually is before reading further.

  • When a filter has no branch on its argument's type, per-type tests of it are copies of one smoke test. highlight_json is json.dumps fed to Pygments, so test_none_renders_as_null and test_list_renders only asserted stdlib behaviour; the dict case was enough. Conversely readable_value branches on str/list/dict and on block type, which is why its 28 tiny tests are nearly all load-bearing.

  • A "nested path" test is worth checking for tautology. test_format_diff_nested_path passed the dotted string "preferences.lang" and asserted the output path was "preferences.lang", but format_participant_data_diff only joins a path when it is a list — the assertion restated its own input. The list-path test next to it was real.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Fields

    Priority

    None yet

    Effort

    None yet

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions