UN-3636 [MISC] enable the unit-workers rig group and de-flake its suite#2176
UN-3636 [MISC] enable the unit-workers rig group and de-flake its suite#2176chandrasekharan-zipstack wants to merge 4 commits into
Conversation
The unit-workers group filtered `markers: "unit"`, but no worker test carries that marker, so it collected zero tests and silently never ran in CI. Switch to the negative filter the other unit groups use so the 734 worker tests actually run. Enabling collection surfaced pre-existing debt (the suite was never CI-gated): - Test isolation: worker Celery apps build a live-Postgres result backend from ambient DB_*/CELERY_BACKEND_DB_*, and building an app hijacks `current_app`. Both leaked across tests under xdist (psycopg2 connection errors, task `NotRegistered`). Fixed in conftest: strip the DB env before any app import, and pin celery's finalized default app as current around each test. - Stale assertions (prod drifted, tests never caught it): ExecutorToolShim / _run_agentic_extraction gained args; index result now carries usage_records; the highlight-disabled test over-asserted "plugin loader untouched" when lookup-enrichment is queried unconditionally (returns None in OSS). - Dropped tautological enum-existence tests that just mirror the source enum. - Renamed the cryptic test_sanity_phase6* / test_phaseN* files to intent-revealing names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019EN8hh518CbCVMP3BHTVmG
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughWorker test selection and Celery isolation were updated. Rig handling now treats empty default pytest groups as failures, while worker executor, plugin, pipeline, routing, logging, and challenge/evaluation tests were clarified or reformatted without production-code changes. ChangesWorker test configuration and execution safeguards
Executor, plugin, and operation coverage
Pipeline and dispatch test normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| tests/groups.yaml | Enables worker unit-test collection with the repository's negative marker pattern. |
| tests/rig/cli.py | Makes empty required pytest groups fail normal rig runs. |
| tests/rig/tests/test_cli.py | Covers empty required groups and explicit marker overrides. |
| workers/tests/conftest.py | Isolates worker tests from ambient database settings and Celery current-app changes. |
| workers/tests/test_agentic_operations.py | Updates agentic extraction tests for the required execution identifier. |
| workers/tests/test_completion_and_highlight.py | Checks that disabled highlighting skips only the highlight plugin lookup. |
| workers/tests/test_extraction_pipeline_contracts.py | Updates indexing mocks for usage-record flushing. |
Reviews (4): Last reviewed commit: "UN-3636 [DEV] fail the build on a non-op..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workers/tests/test_completion_and_highlight.py (1)
68-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dictionary check for
call_kwargs.In Python's
unittest.mock._Callobject,call_kwargs.kwargsandcall_kwargs[1]refer to the exact same dictionary of keyword arguments. If the intent was to check whether the callback was passed as a positional argument instead, you would need to inspectcall_kwargs.args(orcall_kwargs[0]).If
process_textis always expected to be passed as a keyword argument, you can simplify this assertion by removing the redundant check.♻️ Proposed refactor
- assert ( - call_kwargs.kwargs.get("process_text") is callback - or call_kwargs[1].get("process_text") is callback - ) + assert call_kwargs.kwargs.get("process_text") is callback🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers/tests/test_completion_and_highlight.py` around lines 68 - 71, Update the assertion around call_kwargs to inspect only call_kwargs.kwargs for the process_text callback, removing the redundant call_kwargs[1] check since process_text is expected as a keyword argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@workers/tests/test_completion_and_highlight.py`:
- Around line 68-71: Update the assertion around call_kwargs to inspect only
call_kwargs.kwargs for the process_text callback, removing the redundant
call_kwargs[1] check since process_text is expected as a keyword argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bf16fee-70b3-4f7f-97e8-3d275d20f635
📒 Files selected for processing (19)
tests/groups.yamlworkers/tests/conftest.pyworkers/tests/test_agentic_operations.pyworkers/tests/test_completion_and_highlight.pyworkers/tests/test_dispatch_with_callback.pyworkers/tests/test_extraction_pipeline_contracts.pyworkers/tests/test_log_streaming.pyworkers/tests/test_plugin_loader.pyworkers/tests/test_plugin_migration_regression.pyworkers/tests/test_prompt_studio_dispatch.pyworkers/tests/test_simple_prompt_studio_operation.pyworkers/tests/test_single_pass_extraction.pyworkers/tests/test_smart_table_extract_operation.pyworkers/tests/test_structure_pipeline.pyworkers/tests/test_structure_tool_pipeline.pyworkers/tests/test_summarize_operation.pyworkers/tests/test_table_extract_operation.pyworkers/tests/test_table_lineitem_challenge_eval.pyworkers/tests/test_variable_replacement_postprocessor.py
chandrasekharan-zipstack
changed the title
Drop stale-prone specifics (function names, file:line refs, vendor lists, class names) from groups.yaml comments; keep the WHY. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019EN8hh518CbCVMP3BHTVmG
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
Reviewed the marker fix, the conftest isolation work, the stale-assertion fixes, and the deletions/renames. Net: the direction is right and this is a strict improvement — enabling 734 previously-dead tests is a clear win, and both conftest mechanisms (env strip + current_app pinning) are correct as written.
I ran an adversarial pass over my own findings before posting, and three didn't survive. Recording them so nobody re-litigates:
- The
*_enum_existsdeletions are fine. I first read them as the only pin on sdk1'sOperationvalues — they aren't.test_plugin_migration_regression.py:131-176already pins the whole set bidirectionally (test_every_operation_is_mapped,test_no_extra_mappings,len(Operation) == 19). Genuine dedup, and sdk1 is an in-repo editable path dep, not a versioned external provider. Agreed with the call. shared/testsis not missed by the env strip.os.environ.pop()is process-global and both paths run in one pytest process with conftests loaded before any test executes. Only the autousecurrent_appfixture doesn't reachshared/tests, and nothing there touches celery — latent, zero impact.- The env strip is the right mechanism, not a workaround — it's the only thing that handles an ambient exported
DB_HOSTand the only thing coveringCELERY_BACKEND_DB_*. See the conftest note.
Four comments left. Two I think are worth acting on:
- The rig still scores "collected zero tests" as a pass for non-optional groups. This PR fixes the instance, not the class — the next marker typo goes green the same way.
- The
log_componentreplica tests are the real tautologies — and one is currently green while asserting the opposite of what production does.
Plus two nits. Nothing blocking; CI is green.
| markers: "unit" | ||
| # Negative filter like the other unit groups: worker tests aren't tagged | ||
| # `unit`; live-infra tests carry `integration`/`slow` and run elsewhere. | ||
| markers: "not integration and not slow" |
There was a problem hiding this comment.
The mechanism that hid this bug is still in place.
Flipping the marker fixes this group, but not the reason a zero-collect group reported green. tests/rig/cli.py:52 has _NON_FAILING_PYTEST_EXIT_CODES = (0, 5), and pytest exit 5 means "no tests collected". The aggregation at cli.py:481-486:
if (
exit_code not in _NON_FAILING_PYTEST_EXIT_CODES # exit 5 -> False, short-circuits
and not group.optional
and overall_exit == 0
):The not group.optional guard is never reached for exit 5, so a non-optional group that collects nothing folds in as a pass. That is precisely what unit-workers did. tests/rig/ is untouched by this PR, so the next marker typo, renamed marker, or moved directory goes green the same way.
Worth knowing: the rig already has a guard built for exactly this. _coverage_attesting_groups() (cli.py:664) excludes empty groups from attesting coverage, with the comment "a broken marker expression would otherwise report OK with zero tests run". It never fired here only because it applies to groups named in critical_paths.yaml covered_by, and unit-workers is in none of them.
Cheapest close: add unit-workers to a critical path's covered_by so the existing gate does its job. Note a blanket _NON_FAILING_PYTEST_EXIT_CODES = (0,) is not safe — hurl groups synthesize exit 5 at cli.py:897, and --marker/--paths overrides legitimately zero-collect.
Happy for this to be a follow-up ticket if you'd rather keep the PR scoped, but as it stands UN-3636 fixes the instance and leaves the class open.
There was a problem hiding this comment.
Good catch on the class-level gap. The fix lives in tests/rig/ (exit-5 handling), which this PR deliberately does not touch, and as you note a blanket (0,) is unsafe given hurl’s synthesized exit 5 and legitimate marker/path zero-collects. Taking your offer to track it as a follow-up rather than widen scope — will file a ticket for a rig-level empty-collect guard on non-optional groups.
There was a problem hiding this comment.
Decided to close the class here rather than defer. 2a1acc63 adds a runtime gate: a non-optional pytest group that collects zero tests (exit 5) now fails overall_exit instead of folding in green. Exemptions match the legit zero-collects you called out — optional groups, hurl (its synthesized exit 5 = "no files"), and dev runs with a --marker/--paths override. So _NON_FAILING_PYTEST_EXIT_CODES stays (0, 5) for those; the new guard only fires for the misconfiguration case. Two rig self-tests added (test_empty_nonoptional_group_fails_build, test_empty_group_with_marker_override_does_not_fail).
| for c in mock_plugin_get.call_args_list | ||
| if c.args and c.args[0] == "highlight-data" | ||
| ] | ||
| assert highlight_fetches == [] |
There was a problem hiding this comment.
Nit, but on-theme: this assertion stops asserting if the call style ever changes.
c.args and c.args[0] == "highlight-data" only matches positional calls. legacy_executor.py:1394 does ExecutorPluginLoader.get("highlight-data") today, so it works. But if that ever becomes get(name="highlight-data"), c.args is empty, highlight_fetches is [], and assert highlight_fetches == [] passes vacuously. A test that silently stops testing is the same failure class this PR exists to fix.
assert not [
c
for c in mock_plugin_get.call_args_list
if "highlight-data" in c.args or c.kwargs.get("name") == "highlight-data"
]There was a problem hiding this comment.
Fixed in f66b9db — assertion now matches "highlight-data" in c.args or c.kwargs.get("name") == "highlight-data", so a kwarg-style call cannot pass it vacuously.
| load_dotenv(_env_test) | ||
|
|
||
| # Worker Celery apps build a Postgres result backend from DB_*/CELERY_BACKEND_DB_*. | ||
| # Strip these before any app is imported so tests don't reach (or leak) a live DB |
There was a problem hiding this comment.
Nit / maintainability: the root conftest does the exact opposite of this block, four lines at a time.
workers/conftest.py:15-18 (which also covers shared/tests):
os.environ.setdefault("DB_HOST", "localhost")
os.environ.setdefault("DB_USER", "test")
os.environ.setdefault("DB_PASSWORD", "test")
os.environ.setdefault("DB_NAME", "testdb")Two adjacent conftests with opposing intent on the same four vars, resolving correctly only because pytest loads the root one first.
To be clear, the pop is the right mechanism — keep it. It's the only thing that neutralises an ambient exported DB_HOST (which setdefault would happily leave pointing at a live DB, and the rig copies the ambient env into every group subprocess at tests/rig/cli.py:83), and the only thing covering CELERY_BACKEND_DB_*, which the root conftest never sets and worker_config.py:129-152 checks first.
But those four setdefaults are now dead weight during the group run, and the next reader will reasonably conclude a DB is configured. Worth dropping them in addition to this pop (never instead of), or at minimum cross-referencing between the two files.
There was a problem hiding this comment.
Kept the pop and cross-referenced both conftests (f66b9db). One correction: those setdefaults are not dead during the group run — the pop only covers workers/tests/, so shared/tests/ (no local conftest) still relies on them. Dropping them would break shared/tests, so I left them and added the cross-reference instead.
…tions - Replace the log_component replica (copied tasks.py if/elif, drifted so the ide_index case asserted the opposite of production) with tests that drive the real execute_extraction, covering the two special-cased branches. - Match the highlight-plugin assertion on positional or kwarg calls so it can't pass vacuously if the call style changes. - Reword the unit-workers marker note: integration/slow worker tests are currently unrouted, not run "elsewhere". - Cross-reference the two workers conftests on the DB_* env they set vs strip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019EN8hh518CbCVMP3BHTVmG
…thing A zero-collect group returns pytest exit 5, which the rig treats as non-failing — so a broken marker or moved path silently reported green (the very way unit-workers stayed dormant). Gate on it: a non-optional pytest group that collects zero tests now fails the overall exit. Legit zero-collects are exempt: optional groups, hurl (its exit 5 means "no files"), and dev runs with a --marker/--paths override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019EN8hh518CbCVMP3BHTVmG
|
Unstract test resultsPer-group results
Critical paths
|
2 participants
What
The
unit-workersrig group filteredmarkers: "unit", but no worker test carries that marker — so it collected zero tests and silently never ran in CI. This switches it to the negative filter the other unit groups use, so all 734 worker tests actually run and gate.Enabling collection surfaced pre-existing debt (the suite was never CI-gated). All fixed here — no product code changed.
Fixes
Test isolation (parallel/xdist pollution) — in
workers/tests/conftest.py:DB_*/CELERY_BACKEND_DB_*; stripped before any app import so eager results stay in-memory.current_app; pin celery's finalized default app as current around each test so@worker_taskproxies resolve (wasNotRegistered/ psycopg2 errors, order-dependent).Stale assertions (prod drifted, tests never caught it since the suite wasn't running):
ExecutorToolShimgainedexecution_id/organization_id/file_execution_id._run_agentic_extraction()gained requiredexecution_id.usage_recordsfromembedding.flush_pending_usage().lookup-enrichmentis queried unconditionally and returnsNonein OSS; narrowed to assert the highlight plugin specifically.Vanity removal — dropped tautological
*_enum_existstests that just mirror the sourceOperationenum (kept the routing/registration/removed-guard tests).Renames —
test_sanity_phase6*/test_phaseN*→ intent-revealing names (git mv, history preserved).Verification
tox -e groups -- unit-workers→ 734 passed, stable across repeated parallel runs.tox -e rig -- validateOK. Lint clean under the repo's pinned ruff/ruff-format.🤖 Generated with Claude Code