UN-3636 [FIX] Scope rig --fail-on-critical-gap to in-tier coverage so main runs green#2115
Open
chandrasekharan-zipstack wants to merge 9 commits into
Open
UN-3636 [FIX] Scope rig --fail-on-critical-gap to in-tier coverage so main runs green#2115chandrasekharan-zipstack wants to merge 9 commits into
chandrasekharan-zipstack wants to merge 9 commits into
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (30)
WalkthroughCritical-path statuses now include ChangesCritical-path scope gating
Backend test harness and configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
|
| Filename | Overview |
|---|---|
| tests/rig/critical_paths.py | Adds in_scope field to CriticalPathStatus; evaluate() correctly computes it as scope is None or any(g in scope for g in path.covered_by), covering all three gap flavours (in-tier red group, out-of-tier group, no declared coverage). |
| tests/rig/cli.py | Splits gap list into in_scope_gaps / out_of_scope_gaps; only in_scope_gaps gate --fail-on-critical-gap. Moves install_editable into _pytest_command via --with-editable so it survives uv run venv re-sync. |
| tests/rig/tests/test_cli.py | Adds two new CLI integration tests: one proving in-scope gaps gate the build (optional group ran red), and one proving out-of-scope gaps (empty covered_by) never gate, directly exercising the PR's main fix. |
| tests/rig/tests/test_critical_paths.py | Two new unit tests validate in_scope flag: test_in_scope_flag_distinguishes_gap_flavours checks all three gap categories in one call; test_covered_path_is_in_scope confirms covered paths always carry in_scope=True. |
| tests/groups.yaml | Removes unit-tool-registry (broken build-system), comments out unit-runner and unit-prompt-service (decommissioning), drops optional: true from unit-connectors and unit-core, and narrows unit-backend paths + env to ones that provision cleanly today. |
| tests/critical_paths.yaml | Wires adapter-register-llm to unit-sdk1 (gives the path real teeth), comments out tool-sandbox-exec path (runner decommissioning), and clarifies the in-scope/out-of-scope gap semantics in the file header. |
| unstract/connectors/tests/databases/test_mariadb.py | Fixes the authentication-error assertion to match the actual error string in mysql_handler.py (SSL SETTINGS uppercase). All other tests in this file use @patch mocks so no live connection is required. |
| backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py | Adds _SAVED_MODULES + _restore_modules() in a module-level finally block so the account_v2 / utils stubs are removed after the helper is imported, preventing them from leaking into sibling test modules that need real imports. |
| backend/usage_v2/tests/test_helper.py | Replaces the sys.modules stub-injection approach with a direct module-attribute swap (helper_mod.Usage = FakeUsage), relying on the fact that Django is now set up via pytest-django in the backend test environment. |
| pyproject.toml | Removes the global DJANGO_SETTINGS_MODULE from root pytest.ini_options; Django settings are now supplied per-group via env: in groups.yaml, preventing the root-level rig tests from inheriting Django configuration they don't need. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmd_run: collect runnable groups] --> B[scope_groups = set of runnable groups]
B --> C[Run each group, collect group_results]
C --> D[_green_group_names: groups with exit 0]
D --> E[evaluate: compute CriticalPathStatus per path]
E --> F{For each path}
F --> G{any covered_by group ran green?}
G -- Yes --> H[state = covered, in_scope = True]
G -- No --> I{any covered_by group in scope_groups?}
I -- Yes --> J[state = gap, in_scope = True, IN-SCOPE GAP]
I -- No --> K[state = gap, in_scope = False, OUT-OF-SCOPE GAP]
J --> L{--fail-on-critical-gap?}
L -- Yes --> M[overall_exit = 1, Build fails]
L -- No --> N[Warn only]
K --> O[Info: Warn only, never gates build]
H --> P[No action needed]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[cmd_run: collect runnable groups] --> B[scope_groups = set of runnable groups]
B --> C[Run each group, collect group_results]
C --> D[_green_group_names: groups with exit 0]
D --> E[evaluate: compute CriticalPathStatus per path]
E --> F{For each path}
F --> G{any covered_by group ran green?}
G -- Yes --> H[state = covered, in_scope = True]
G -- No --> I{any covered_by group in scope_groups?}
I -- Yes --> J[state = gap, in_scope = True, IN-SCOPE GAP]
I -- No --> K[state = gap, in_scope = False, OUT-OF-SCOPE GAP]
J --> L{--fail-on-critical-gap?}
L -- Yes --> M[overall_exit = 1, Build fails]
L -- No --> N[Warn only]
K --> O[Info: Warn only, never gates build]
H --> P[No action needed]
Reviews (3): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/critical_paths.yaml`:
- Around line 57-63: Keep `tool-sandbox-exec` active in
`tests/critical_paths.yaml` until the runner/tool-registry flow is fully
removed; the current commented-out entry is excluded from `evaluate()`, so
restore the `tool-sandbox-exec` item in place with `covered_by: []` and keep its
`entry`/`description` intact so the warn-only path still reports the gap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 247fbb34-d0f1-417c-97a5-76915cbe3c0f
📒 Files selected for processing (2)
tests/critical_paths.yamltests/groups.yaml
Contributor
Unstract test resultsPer-group results
Critical paths
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
unstract/connectors/tests/filesystems/test_sharepoint_fs.py (1)
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEncode the current schema contract instead of skipping it.
If
is_personalis intentionally not part of the JSON schema, please assert that explicitly here rather than disabling the test. Leaving it skipped means accidental schema drift in either direction will pass unnoticed.🤖 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 `@unstract/connectors/tests/filesystems/test_sharepoint_fs.py` around lines 118 - 122, The SharePoint filesystem test is skipped instead of asserting the intended schema contract, so replace the skip in test_sharepoint_fs.py with an explicit assertion in the relevant test around is_personal/json_schema.json behavior. Use the existing SharePoint filesystem test class and helpers to verify that is_personal is intentionally absent from the JSON schema (and that personal/site is inferred from site_url), so future schema drift is caught by the test instead of being ignored.
🤖 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.
Inline comments:
In
`@backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py`:
- Around line 86-96: The module restore helper currently clears only the saved
collaborator stubs, but it also needs to evict prompt_studio_helper from
sys.modules so later imports reload it with real dependencies. Update
_restore_modules in test_build_index_payload.py to pop the prompt_studio_helper
module entry after _psh_mod has already been bound, and make sure the same
cleanup is applied wherever the tests reset module state (including the later
restore block referenced in the comment).
In `@backend/usage_v2/tests/test_helper.py`:
- Around line 20-30: Avoid the module-level reassignment of the Usage symbol in
test_helper.py, because it leaves backend.usage_v2.helper patched for the rest
of the test run. Move the FakeUsage substitution into a test fixture or
monkeypatch setup/teardown so helper_mod.Usage is only overridden within the
scope of each test, while keeping get_usage_by_model and UsageHelper as the
relevant symbols to target.
In `@tests/rig/cli.py`:
- Around line 680-683: The project-root editable install in the CLI test setup
is redundant because uv run already installs the current project editable, so
the install_editable flag is not actually controlling behavior for unit-core.
Update the logic around group.install_editable in cli.py so it does not
unconditionally add --with-editable str(workdir) for the project root; instead,
either remove this branch or switch to the appropriate uv flags (--no-project or
--no-editable) if the flag is intended to control project installation.
In `@unstract/connectors/tests/databases/test_redshift_db.py`:
- Around line 8-18: The Redshift integration test only skips when
REDSHIFT_TEST_PASSWORD is set, but test_user_name_and_password still directly
requires REDSHIFT_TEST_HOST through os.environ[], so the test can fail instead
of skipping. Update the unittest.skipUnless guard on test_user_name_and_password
in test_redshift_db.py to require all mandatory Redshift env vars used by
Redshift initialization, especially REDSHIFT_TEST_PASSWORD and
REDSHIFT_TEST_HOST, so the test is skipped unless every required setting is
present.
In `@unstract/connectors/tests/databases/test_snowflake_db.py`:
- Around line 8-21: Update the test guard in test_snowflake_db.py so the
Snowflake integration test is skipped unless all required mandatory environment
variables are set, not just SNOWFLAKE_TEST_PASSWORD. In test_something, adjust
the `@unittest.skipUnless` condition to check the presence of SNOWFLAKE_TEST_USER,
SNOWFLAKE_TEST_PASSWORD, and SNOWFLAKE_TEST_ACCOUNT before constructing
SnowflakeDB, while keeping the optional defaults for database, schema,
warehouse, and role unchanged.
In `@unstract/connectors/tests/filesystems/test_google_drive_fs.py`:
- Around line 8-10: The Google Drive integration test is only gated on
GDRIVE_GOOGLE_SERVICE_ACCOUNT, so it can still run and fail when the Google
Drive environment is only partially configured. Update the unittest.skipUnless
condition in test_google_drive_fs to require the full Google Drive env contract
used by GoogleDriveFS initialization, including both
GDRIVE_GOOGLE_SERVICE_ACCOUNT and GDRIVE_GOOGLE_PROJECT_ID, so the test is
skipped unless all required settings are present.
In `@unstract/connectors/tests/filesystems/test_miniofs.py`:
- Around line 35-37: The MinIO integration test skip guard only checks
MINIO_ACCESS_KEY_ID, so it can still run without MINIO_SECRET_ACCESS_KEY and
fail later. Update the skip condition in test_miniofs.py around the
unittest.skipUnless on the MinioFSTest class to require both MINIO_ACCESS_KEY_ID
and MINIO_SECRET_ACCESS_KEY before running, so the test cleanly skips unless a
usable MinIO client can be built.
In `@unstract/connectors/tests/filesystems/test_pcs.py`:
- Around line 8-10: The PCS integration test gating is incomplete because it
only checks GOOGLE_STORAGE_ACCESS_KEY_ID while the test setup also depends on
GOOGLE_STORAGE_SECRET_ACCESS_KEY. Update the skip condition in the test
decorator for the PCS test in test_pcs.py so it skips unless both credentials
are present, using the existing unittest.skipUnless on the test class or method
that configures the PCS client.
---
Nitpick comments:
In `@unstract/connectors/tests/filesystems/test_sharepoint_fs.py`:
- Around line 118-122: The SharePoint filesystem test is skipped instead of
asserting the intended schema contract, so replace the skip in
test_sharepoint_fs.py with an explicit assertion in the relevant test around
is_personal/json_schema.json behavior. Use the existing SharePoint filesystem
test class and helpers to verify that is_personal is intentionally absent from
the JSON schema (and that personal/site is inferred from site_url), so future
schema drift is caught by the test instead of being ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ead960d-5f5a-4850-8f85-5e9cb6850294
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
backend/dashboard_metrics/tests/test_tasks.pybackend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.pybackend/pyproject.tomlbackend/usage_v2/tests/test_helper.pybackend/utils/file_storage/__init__.pybackend/utils/file_storage/helpers/__init__.pybackend/utils/file_storage/helpers/prompt_studio_file_helper.pypyproject.tomltests/groups.yamltests/rig/cli.pyunstract/connectors/tests/databases/test_mariadb.pyunstract/connectors/tests/databases/test_mssql_db.pyunstract/connectors/tests/databases/test_mysql_db.pyunstract/connectors/tests/databases/test_postgresql_db.pyunstract/connectors/tests/databases/test_redshift_db.pyunstract/connectors/tests/databases/test_snowflake_db.pyunstract/connectors/tests/filesystems/test_box_fs.pyunstract/connectors/tests/filesystems/test_google_drive_fs.pyunstract/connectors/tests/filesystems/test_miniofs.pyunstract/connectors/tests/filesystems/test_pcs.pyunstract/connectors/tests/filesystems/test_sharepoint_fs.pyunstract/connectors/tests/filesystems/test_zs_dropbox_fs.py
💤 Files with no reviewable changes (1)
- pyproject.toml
✅ Files skipped from review due to trivial changes (2)
- backend/utils/file_storage/helpers/prompt_studio_file_helper.py
- unstract/connectors/tests/databases/test_mariadb.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/groups.yaml
Comment on lines
+680
to
+683
| # Inject the project as editable here so it survives the venv re-sync, | ||
| # same as the plugins above. | ||
| if group.install_editable: | ||
| with_args += ["--with-editable", str(workdir)] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo "== _pytest_command / subprocess context =="
rg -n -C3 '_pytest_command\(|subprocess\.run\(' tests/rig/cli.py
echo
echo "== groups declaring install_editable =="
rg -n -C2 'install_editable:' tests/groups.yaml
echo
echo "== check whether those workdirs are already project roots =="
python - <<'PY'
from pathlib import Path
import re
lines = Path("tests/groups.yaml").read_text().splitlines()
groups = []
current = None
workdir = None
install_editable = False
for line in lines:
m = re.match(r'^\s{2}([A-Za-z0-9_.-]+):\s*$', line)
if m:
if current and install_editable:
groups.append((current, workdir))
current = m.group(1)
workdir = None
install_editable = False
continue
if current is None:
continue
m = re.match(r'^\s{4}workdir:\s*(.+?)\s*$', line)
if m:
workdir = m.group(1)
continue
if re.match(r'^\s{4}install_editable:\s*true\s*$', line):
install_editable = True
if current and install_editable:
groups.append((current, workdir))
for name, wd in groups:
print(f"{name}: workdir={wd}")
if not wd:
continue
path = Path(wd)
for marker in ("pyproject.toml", "setup.py", "setup.cfg"):
print(f" {marker}: {'yes' if (path / marker).exists() else 'no'}")
PYRepository: Zipstack/unstract
Length of output: 1964
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '620,710p' tests/rig/cli.py | cat -n
printf '\n== groups.yaml excerpt ==\n'
sed -n '120,160p' tests/groups.yaml | cat -nRepository: Zipstack/unstract
Length of output: 5664
🌐 Web query:
uv run --with-editable current project installed by default --no-install-project --no-editable docs
💡 Result:
The command uv run --with-editable [package] [command] executes a specified command in an environment where the requested package is installed in editable mode [1][2]. When used within a project, this command layers the requested dependency on top of the project's environment in a separate, ephemeral environment, allowing the dependency to conflict with those defined in the project [1][3][2]. Regarding the other flags mentioned: --no-install-project: This flag is not a standard, built-in option for the uv run command as of June 2026 [1][2][4]. Users attempting to avoid installing the current project when running commands typically use the --no-project flag instead [5][4]. --no-editable: This flag is primarily used with uv sync or uv run to instruct uv to install the project (or dependencies) in non-editable (standard) mode, rather than the default editable mode [6]. --no-project: This is the authoritative flag to prevent uv run from installing the current project into the environment [5]. When --no-project is used, uv ignores the project's dependencies and configuration, allowing you to run a command in an isolated environment (or one defined only by the command's requirements/inline metadata) [7][5]. In summary, if your goal is to run a command without installing the project, use --no-project [5]. If you need to install specific packages in editable mode alongside that command, combine --with-editable with your package specifications [1][2].
Citations:
- 1: https://docs.astral.sh/uv/reference/cli/index.md
- 2: https://docs.rs/uv-cli/latest/uv_cli/struct.RunArgs.html
- 3: astral-sh/uv@e5dd67f
- 4: Add
--no-install-*toadd/remove/runand add env vars astral-sh/uv#6578 - 5: https://docs.astral.sh/uv/guides/scripts/index.md
- 6: https://docs.astral.sh/uv/concepts/projects/config/
- 7: https://github.com/astral-sh/uv/blob/main/docs/guides/scripts.md
install_editable is redundant for project-root groups
uv run already installs the current project editable, so --with-editable str(workdir) doesn’t change behavior here and makes install_editable=False ineffective for unit-core. If the flag is meant to control project installation, use --no-project/--no-editable; otherwise drop this branch.
🤖 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 `@tests/rig/cli.py` around lines 680 - 683, The project-root editable install
in the CLI test setup is redundant because uv run already installs the current
project editable, so the install_editable flag is not actually controlling
behavior for unit-core. Update the logic around group.install_editable in cli.py
so it does not unconditionally add --with-editable str(workdir) for the project
root; instead, either remove this branch or switch to the appropriate uv flags
(--no-project or --no-editable) if the flag is intended to control project
installation.
… main runs green The rig's unit/integration CI job had never passed on main. `--fail-on-critical-gap` (passed only on the main push) failed the build on EVERY uncovered critical path, including e2e-only paths run during the unit/integration tier and paths with no test anywhere. Every group-level failure was already non-gating (optional groups, or exit 5 = no tests collected), so the gap gate was the sole cause of redness. - Split critical-path gaps into in-scope vs out-of-scope (add `in_scope` to CriticalPathStatus; evaluate() already computed it). --fail-on-critical-gap now gates only on in-scope gaps — a declared in-tier covering group that didn't run green (real coverage regressed). Out-of-scope gaps (covered only by an unrun tier, or no declared coverage) are reported + logged but never gate that tier. - Wire honest coverage that exists today: adapter-register-llm -> unit-sdk1. Gives the path teeth: if sdk1 regresses, it flips to an in-scope gap and fails. - Drop unit-tool-registry group (component slated for removal; can't even collect). - Delete 3 dead tests referencing removed code: core test_pandora_account.py (account_services removed), core test_pubsub_helper.py (LogHelper -> LogPublisher), platform-service test_auth_middleware.py (platform_service.main removed; also made live Postgres calls). unit-platform-service is green via its hermetic memory-leak test; unit-core has no valid tests left and skips as a placeholder. New self-tests cover the in-scope/out-of-scope split at both evaluate() and cmd_run(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XJqp7xMdd1kjLUKbvrJsq4
…kend tests
Follow-up cleanup on the rig manifests (UN-3636):
- Park unit-runner and unit-prompt-service (commented out, not run by
default) with a TODO to delete when those services are removed — both are
being decommissioned; no value testing components on their way out.
- Drop the unit-tool-registry NOTE block entirely.
- Prune 6 unit-backend paths that collect zero tests (account_v2,
api_deployment_v2, connector_v2, file_management, project,
tenant_account_v2 — dirs missing or empty); optional skip-if-missing was
hiding them and implying coverage that was never written.
- Wire two real backend tests into unit-backend:
* middleware/test_exception.py — 5 hermetic tests, pass now.
* prompt_studio/prompt_studio_core_v2/tests — pins the executor
_handle_ide_index async path (no prompt-service coupling); runs once
unit-backend is un-gated, skips safely until then.
- Comment out the tool-sandbox-exec critical path (TODO: remove with
tool-registry/runner) — its covering group unit-runner is now parked.
`python -m tests.rig validate` → OK (13 groups, 9 critical paths).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
…jango setting unit-core ran 0 tests / 2 collection errors (ModuleNotFoundError: No module named 'unstract'). Root cause: _prepare_group_env did `uv pip install -e .` for install_editable groups, but _pytest_command runs `uv run`, which re-syncs the venv every call and wipes that install before pytest imports the package — the same hazard the code already flagged for plugins. Inject the package via `uv run --with-editable <workdir>` (survives the re-sync, same mechanism as the RIG_PYTEST_PLUGINS `--with` specs) and drop the wiped `uv pip install -e .`. unit-core now 27 passed, 0 errors. Also remove DJANGO_SETTINGS_MODULE from the repo-root [tool.pytest.ini_options]: it's a pytest-django option that warns "Unknown config option" for every non-Django group, points at a non-existent module (backend.settings.test_cases), and Django settings don't belong at the polyglot repo root. The rig injects it per-group via groups.yaml env for unit-backend only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
The unit-backend group pointed at a non-existent settings module
(backend.settings.test_cases) and ran without pytest-django, so the
DB-backed tests errored at collection (Django uninitialised) and the
django_db tests had no test-DB lifecycle. Make the group actually runnable:
- Add pytest-django to the backend test group (bootstraps Django before
collection; provides the test-DB + django_db fixtures).
- Point DJANGO_SETTINGS_MODULE at the existing backend.settings.test and
inject the import-time-required settings via the group env — base.py reads
them before any dotenv load, so they must exist in the process env, not a
settings module. ENCRYPTION_KEY is an all-zero (valid, zero-entropy) Fernet
placeholder, not a real secret.
- Set DB_SCHEMA=public: the app's fixed schema doesn't exist in the fresh
test DB and tenancy is row-level, so migrations run in public.
- Drop workflow_manager/endpoint_v2/tests from the wired paths: its
destination-connector tests import the enterprise `plugins` package,
absent in OSS.
- Add the missing utils/file_storage{,/helpers}/__init__.py so those
modules import as a package under pytest.
- Stop test_build_index_payload's sys.modules stubs from leaking into
sibling collection: record + restore the originals once the helper is
imported (a stubbed account_v2.models was breaking other modules'
real imports).
unit-backend now collects clean; 126 passed, 4 skipped. The remaining 6
failures (usage_v2 helper stubs, dashboard_metrics cleanup tasks) are
pre-existing test bugs tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
dashboard_metrics: organization FK targets Organization's int PK, but the tests passed a UUID string as organization_id, and verified rows through the org-scoped default manager (empty without a UserContext). Create a real Organization and read via _base_manager. usage_v2: drop the fragile "stub usage_v2.models into sys.modules before import" trick — under pytest-django Django imports the real module first, so the stub never took and the helper hit the DB. Rebind the Usage symbol the helper resolved instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
The connector suite had 12 reds that needed real external services or
per-developer credentials no one has by default. Two were genuine bugs;
the rest are integration tests masquerading as unit tests.
Fixes (not credential-related):
- mariadb: assertion text drifted from the connector's actual message
("SSL SETTINGS", not "ssl-settings").
- sharepoint: skip test_json_schema_has_is_personal — is_personal is read
from settings in code but was never exposed in json_schema.json (personal
vs site is inferred from an empty site_url). Whether the schema should
expose it is a product decision; tracked under UN-3414.
Gating (skipUnless, mirrors the existing SharePoint integration tests):
- filesystems (box, gdrive, minio, pcs, dropbox) already read creds from
env; add the missing skip guard so they SKIP instead of failing.
- databases (mssql, mysql, postgresql, redshift, snowflake) had hardcoded
personal creds (incl. a live-looking neon.tech URL and a Snowflake
account) querying bespoke tables. Move creds to *_TEST_* env vars and
skip unless provided, removing the secrets from the repo.
CI can run these by injecting the corresponding secrets as env vars in a
dedicated integration job; by default they skip cleanly.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
Both now run green and standalone (no external services; integration tests skip cleanly when credentials are absent), so drop optional: true to make them blocking merge gates per UN-3635. unit-backend stays optional until the rig provisions a reachable DB_HOST for it (UN-3636 follow-up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
for more information, see https://pre-commit.ci
- in_scope defaults False on CriticalPathStatus so a future evaluate() regression that forgets it under-gates (warning) rather than over-gates (spurious build block). [greptile] - widen connector integration skip guards (redshift, snowflake, gdrive, minio, pcs) to require every env var the test hard-references, so a partially configured env skips cleanly instead of failing. [coderabbit] - usage_v2 test_helper: swap Usage via an autouse monkeypatch fixture instead of a module-level rebind that leaks FakeUsage into later tests. - build_index_payload test: evict the helper module from sys.modules after binding it, so later importers in the same process get a real copy. - drop dead tox `runner` alias (its unit-runner group was removed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
d55768c to
9e04f44
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Make the rig's unit/integration CI job (
.github/workflows/ci-test.yaml,tox -e unit/-e integration) run green onmainby scoping the--fail-on-critical-gapgate to in-tier coverage, plus small cleanups of dead/unrunnable tests.This is checklist item #1 of UN-3636 (Obj 1: Dev test rig backfill & readiness).
Why
--fail-on-critical-gap(passed only on themainpush, not PRs) failed the build on every uncovered critical path intests/critical_paths.yaml— including e2e-only paths that don't run in the unit/integration tier, and paths with no test anywhere. Every group-level failure was already non-gating (optional groups, or exit 5 = no tests collected), so this gap gate was the sole reason the job had never passed onmain. A perma-red job can't become a required PR merge gate (the parent goal, UN-3635).How
in_scopeadded toCriticalPathStatus;evaluate()already computed it).--fail-on-critical-gapnow gates only on in-scope gaps — a declared in-tier covering group that didn't run green (real coverage regressed). Out-of-scope gaps (covered only by an unrun tier, or no declared coverage) are reported + logged but never gate that tier.adapter-register-llm → unit-sdk1. This gives the path teeth — if sdk1 regresses, the path flips to an in-scope gap and fails.unit-tool-registrygroup (component slated for removal; can't even collect — commented-out[build-system]+ an undeclared transitiveceleryimport).test_pandora_account.py(account_servicesremoved), coretest_pubsub_helper.py(LogHelper→LogPublisher), platform-servicetest_auth_middleware.py(platform_service.mainremoved; also made live Postgres calls).unit-platform-serviceis green via its hermetic memory-leak test;unit-corehas no valid tests left and skips as a placeholder.evaluate()andcmd_run()layers.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No. Changes are confined to the test rig (
tests/rig/, the two manifests) and the deletion of already-broken test files. No production/runtime code is touched. The gate change only relaxes failure conditions for out-of-tier coverage (it never makes a previously-green run red); real in-tier coverage regressions still fail.Database Migrations
Env Config
Relevant Docs
tests/README.md,tests/rig/Related Issues or PRs
Dependencies Versions
Notes on Testing
Verified locally on current
main(6b3cfab26):python -m pytest tests/rig/tests→ 57 passed (52 + 5 new),python -m tests.rig validate→ OK (15 groups, 10 paths).python -m tests.rig run unit-rig unit-sdk1 unit-runner unit-prompt-service unit-platform-service --fail-on-critical-gap→ exit 0; load-bearing groups green (sdk1 390, rig 57, prompt-service 15, runner 11, platform-service 9); 2 wired paths covered, 8 remaining gaps out-of-scope warn-only.Out of scope / deferred (separate tickets/PRs under UN-3636): re-tier
unit-connectorslive-connection tests to a credential-gated group with skip-without-creds; wire co-locatedbackend/<app>/tests/intounit-backendand make it non-optional;unit-workersmarker fix.Screenshots
Checklist
I have read and understood the Contribution Guidelines.