UN-3636 [MISC] Make the rig unit/integration CI job green on main#2115
Open
chandrasekharan-zipstack wants to merge 33 commits into
Open
UN-3636 [MISC] Make the rig unit/integration CI job green on main#2115chandrasekharan-zipstack wants to merge 33 commits into
chandrasekharan-zipstack wants to merge 33 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCritical-path status tracking now records scope, the rig and manifest configuration were updated, and ChangesCritical-path scope gating
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: bool = False to CriticalPathStatus and threads it through evaluate(); logic is correct and the safe default prevents false build blocks on regression. |
| tests/rig/cli.py | Core gap-gating refactor, infra-env injection helpers, TestcontainersRuntime wiring, _pytest_base_cmd extraction, and --update-baseline guard for report combine. Logic is sound and consistent with the new semantics. |
| backend/conftest.py | New pytest_collection_modifyitems hook auto-marks every Django TestCase/TransactionTestCase/django_db test as integration. Correct hook placement; detects both decorator and class-inheritance based DB access. |
| .github/workflows/ci-test.yaml | Tiers split into parallel matrix; path filtering moved to job-level dorny/paths-filter; report job is the single baseline writer; --fail-on-critical-gap now runs on all PRs (safe with new in-scope semantics). |
| tests/groups.yaml | Consolidates unit-backend to paths: ["."] with markers: "not integration", adds integration-backend and integration-connectors, removes unit-tool-registry and unit-runner; YAML anchor correctly ties both backend groups to the same env block. |
| backend/adapter_processor_v2/tests/test_adapter_api.py | New integration test exercises the full POST /api/v1/adapter/ path with only the SDK context-window call mocked; auto-marked as integration by conftest hook since it inherits TestCase. |
| tests/rig/tests/test_cli.py | New tests cover in-scope vs out-of-scope gap scenarios end-to-end through cmd_run, plus _db_env_from_postgres_url and _inject_infra_env unit tests. All new tests have clear failure scenarios. |
| backend/dashboard_metrics/tests/test_tasks.py | Switches from a UUID org_id string to a real Organization FK; uses _base_manager for assertions to bypass the org-scoped default manager. Correct fix for a silent test error. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
trigger["push/PR triggered"] --> changes["changes job\n(dorny/paths-filter)"]
changes -->|relevant=true| matrix["test matrix\n(unit | integration)"]
changes -->|relevant=false| skip["jobs skipped\n(conclusion: skipped)"]
matrix --> unit["unit leg\ntox -e unit\n--fail-on-critical-gap"]
matrix --> integ["integration leg\ntox -e integration\n--fail-on-critical-gap"]
unit --> gap_unit{gap type?}
integ --> gap_integ{gap type?}
gap_unit -->|in-scope gap| fail_unit["❌ tier fails"]
gap_unit -->|out-of-scope gap| warn_unit["⚠️ warn-only"]
gap_integ -->|in-scope gap| fail_integ["❌ tier fails"]
gap_integ -->|out-of-scope gap| warn_integ["⚠️ warn-only"]
unit --> upload_unit["upload reports-unit artifact"]
integ --> upload_integ["upload reports-integration artifact"]
upload_unit --> report["report job\n(always, unless tiers skipped)"]
upload_integ --> report
report --> combine["tox -e rig -- report combine\n[--update-baseline on green main]"]
combine -->|main + all green| save_baseline["cache/save previous-summary.json"]
combine --> fail_check{"needs.test.result == success?"}
fail_check -->|no| exit1["exit 1"]
fail_check -->|yes| exit0["exit 0"]
%%{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
trigger["push/PR triggered"] --> changes["changes job\n(dorny/paths-filter)"]
changes -->|relevant=true| matrix["test matrix\n(unit | integration)"]
changes -->|relevant=false| skip["jobs skipped\n(conclusion: skipped)"]
matrix --> unit["unit leg\ntox -e unit\n--fail-on-critical-gap"]
matrix --> integ["integration leg\ntox -e integration\n--fail-on-critical-gap"]
unit --> gap_unit{gap type?}
integ --> gap_integ{gap type?}
gap_unit -->|in-scope gap| fail_unit["❌ tier fails"]
gap_unit -->|out-of-scope gap| warn_unit["⚠️ warn-only"]
gap_integ -->|in-scope gap| fail_integ["❌ tier fails"]
gap_integ -->|out-of-scope gap| warn_integ["⚠️ warn-only"]
unit --> upload_unit["upload reports-unit artifact"]
integ --> upload_integ["upload reports-integration artifact"]
upload_unit --> report["report job\n(always, unless tiers skipped)"]
upload_integ --> report
report --> combine["tox -e rig -- report combine\n[--update-baseline on green main]"]
combine -->|main + all green| save_baseline["cache/save previous-summary.json"]
combine --> fail_check{"needs.test.result == success?"}
fail_check -->|no| exit1["exit 1"]
fail_check -->|yes| exit0["exit 0"]
Reviews (19): Last reviewed commit: "ci: tighten workflow comments; drop comm..." | Re-trigger Greptile
chandrasekharan-zipstack
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
chandrasekharan-zipstack
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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
… 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
chandrasekharan-zipstack
force-pushed
the
fix/test-rig-green-v2
branch
from
d55768c to
9e04f44
Compare
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 `@tox.ini`:
- Line 12: Update the stale alias comment in tox config so it no longer mentions
the removed runner env. Adjust the comment near the existing service envs to
reflect only the envs that still exist, using the same nearby tox env names as
the reference point so the documentation matches the current configuration.
🪄 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: dfe7f820-870a-4a7a-b39f-ba9b196dc4cf
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
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.pyplatform-service/tests/test_auth_middleware.pypyproject.tomltests/critical_paths.yamltests/groups.yamltests/rig/cli.pytests/rig/critical_paths.pytests/rig/tests/test_cli.pytests/rig/tests/test_critical_paths.pytox.iniunstract/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.pyunstract/core/tests/account_services/test_pandora_account.pyunstract/core/tests/test_pubsub_helper.py
💤 Files with no reviewable changes (4)
- unstract/core/tests/account_services/test_pandora_account.py
- platform-service/tests/test_auth_middleware.py
- unstract/core/tests/test_pubsub_helper.py
- pyproject.toml
✅ Files skipped from review due to trivial changes (3)
- backend/utils/file_storage/helpers/prompt_studio_file_helper.py
- unstract/connectors/tests/filesystems/test_google_drive_fs.py
- unstract/connectors/tests/filesystems/test_box_fs.py
🚧 Files skipped from review as they are similar to previous changes (15)
- unstract/connectors/tests/databases/test_mariadb.py
- unstract/connectors/tests/databases/test_mysql_db.py
- unstract/connectors/tests/filesystems/test_miniofs.py
- unstract/connectors/tests/databases/test_postgresql_db.py
- unstract/connectors/tests/filesystems/test_sharepoint_fs.py
- unstract/connectors/tests/databases/test_snowflake_db.py
- tests/rig/tests/test_critical_paths.py
- unstract/connectors/tests/databases/test_mssql_db.py
- tests/rig/tests/test_cli.py
- backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py
- unstract/connectors/tests/databases/test_redshift_db.py
- tests/critical_paths.yaml
- unstract/connectors/tests/filesystems/test_zs_dropbox_fs.py
- backend/usage_v2/tests/test_helper.py
- backend/dashboard_metrics/tests/test_tasks.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@tox.ini`:
- Line 12: Update the stale alias comment in tox config so it no longer mentions
the removed runner env. Adjust the comment near the existing service envs to
reflect only the envs that still exist, using the same nearby tox env names as
the reference point so the documentation matches the current configuration.
🪄 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: dfe7f820-870a-4a7a-b39f-ba9b196dc4cf
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
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.pyplatform-service/tests/test_auth_middleware.pypyproject.tomltests/critical_paths.yamltests/groups.yamltests/rig/cli.pytests/rig/critical_paths.pytests/rig/tests/test_cli.pytests/rig/tests/test_critical_paths.pytox.iniunstract/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.pyunstract/core/tests/account_services/test_pandora_account.pyunstract/core/tests/test_pubsub_helper.py
💤 Files with no reviewable changes (4)
- unstract/core/tests/account_services/test_pandora_account.py
- platform-service/tests/test_auth_middleware.py
- unstract/core/tests/test_pubsub_helper.py
- pyproject.toml
✅ Files skipped from review due to trivial changes (3)
- backend/utils/file_storage/helpers/prompt_studio_file_helper.py
- unstract/connectors/tests/filesystems/test_google_drive_fs.py
- unstract/connectors/tests/filesystems/test_box_fs.py
🚧 Files skipped from review as they are similar to previous changes (15)
- unstract/connectors/tests/databases/test_mariadb.py
- unstract/connectors/tests/databases/test_mysql_db.py
- unstract/connectors/tests/filesystems/test_miniofs.py
- unstract/connectors/tests/databases/test_postgresql_db.py
- unstract/connectors/tests/filesystems/test_sharepoint_fs.py
- unstract/connectors/tests/databases/test_snowflake_db.py
- tests/rig/tests/test_critical_paths.py
- unstract/connectors/tests/databases/test_mssql_db.py
- tests/rig/tests/test_cli.py
- backend/prompt_studio/prompt_studio_core_v2/tests/test_build_index_payload.py
- unstract/connectors/tests/databases/test_redshift_db.py
- tests/critical_paths.yaml
- unstract/connectors/tests/filesystems/test_zs_dropbox_fs.py
- backend/usage_v2/tests/test_helper.py
- backend/dashboard_metrics/tests/test_tasks.py
🛑 Comments failed to post (1)
tox.ini (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale alias comment.
runnerwas removed in this change, so this comment now documents a tox env that no longer exists.Suggested diff
-# Pre-existing service envs (runner, sdk1) remain as thin +# Pre-existing service envs (sdk1) remain as thin📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Pre-existing service envs (sdk1) remain as thin🤖 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 `@tox.ini` at line 12, Update the stale alias comment in tox config so it no longer mentions the removed runner env. Adjust the comment near the existing service envs to reflect only the envs that still exist, using the same nearby tox env names as the reference point so the documentation matches the current configuration.
… out of unit Make `requires_services` actually provision instead of being cosmetic. The rig now brings up testcontainers infra (Postgres/MinIO) for any runnable group that declares `requires_services`, and injects connection env into the group's pytest subprocess (Postgres URL -> discrete DB_* vars; MinIO endpoint/creds). Previously django_db tests fell back to the compose hostname `backend-db-1`, unreachable from host-side pytest, so unit-backend had to be `optional`. Reclassify infra-dependent tests by the rig's own tier taxonomy (unit = no external services, integration = real infra but not the full platform): - backend: split unit-backend into pure `unit-backend` (gates unit tier, no infra) and `integration-backend` (django_db tests: dashboard_metrics + prompt_studio_registry_v2; provisioned Postgres; gates integration tier). - connectors: marker-based split (tests are interleaved within files). Credential + MinIO tests marked `@pytest.mark.integration`; `unit-connectors` runs `-m "not integration"`, new `integration-connectors` runs `-m "integration"`. test_minio actually runs against provisioned MinIO; external-credential tests skip. Skip-guard test_http_fs (was hitting a live URL unguarded in CI). Both groups are non-optional and gate their tiers. Unit tier stays infra-free. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/rig/cli.py`:
- Around line 611-631: The backend env override logic in cli.py only applies
Postgres and MinIO, so Redis-backed groups still use the default localhost Redis
and miss the provisioned endpoint. Update the same env-building block that
handles endpoints.infra.postgres_url and endpoints.infra.minio_endpoint to also
check for the Redis service requirement and, when present, rewrite the Redis
connection env from the provisioned Redis endpoint instead of leaving the
tests/groups.yaml default in place. Use the existing endpoint pattern and the
group.requires_services check so integration-backend and other Redis-backed
groups pick up the testcontainer Redis consistently.
In `@unstract/connectors/tests/filesystems/test_http_fs.py`:
- Around line 1-15: Mark the HttpFS test module as integration-tier so it is
selected by the integration test job and excluded from unit runs. Update the
TestHttpFS module/class in the HTTP filesystem test file to use the integration
marker already used elsewhere in the test suite, while keeping the existing
HTTP_FS_TEST_URL skip guard for the live-server dependency.
🪄 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: bac488a0-d4b9-4736-96ba-2a98e93b06c7
📒 Files selected for processing (17)
tests/groups.yamltests/rig/cli.pytests/rig/runtime.pytests/rig/tests/test_cli.pyunstract/connectors/pyproject.tomlunstract/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_http_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 skipped from review as they are similar to previous changes (10)
- unstract/connectors/tests/filesystems/test_miniofs.py
- unstract/connectors/tests/filesystems/test_sharepoint_fs.py
- unstract/connectors/tests/filesystems/test_box_fs.py
- unstract/connectors/tests/filesystems/test_google_drive_fs.py
- unstract/connectors/tests/databases/test_mysql_db.py
- unstract/connectors/tests/databases/test_mssql_db.py
- unstract/connectors/tests/databases/test_snowflake_db.py
- unstract/connectors/tests/databases/test_postgresql_db.py
- unstract/connectors/tests/filesystems/test_zs_dropbox_fs.py
- unstract/connectors/tests/databases/test_redshift_db.py
…ation, cut rig complexity - integration-backend declares requires_services: [postgres, redis] but the rig only injected Postgres/MinIO env, so Redis-backed tests bypassed the testcontainer and hit localhost:6379. Inject REDIS_HOST/PORT + CELERY_BROKER_BASE_URL from the provisioned endpoint (CodeRabbit). - test_http_fs was skip-guarded but unmarked, so the connector marker split (-m "not integration") could still run it in the unit tier. Mark the module integration (CodeRabbit). - Extract _inject_infra_env and _pytest_base_cmd to bring both functions under SonarCloud's cognitive-complexity threshold. NOSONAR the test's DB_PASSWORD placeholder (not a real credential). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
The MinIO endpoint is a throwaway testcontainer with no TLS, so http is expected. Suppress the SonarCloud insecure-protocol hotspot that otherwise blocks the quality gate on new code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
The module stubs adapter_processor_v2.models to import PromptStudioHelper without the full Django app, but only provided AdapterInstance. The helper also imports UserDefaultAdapter, so the import failed and all 4 tests in the module self-skipped via the _IMPORT_ERROR guard. Add the missing stub so the tests actually execute. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
Removed from the e2e test overlay; the platform brought up for e2e no longer provisions a standalone prompt-service. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
…r tests - test_miniofs: drop the permanently-skipped test_s3 (hardcoded AWS S3 smoke). MinIO/S3 is covered by test_minio (integration), the TestAccessFilteredS3 unit tests, and the connectorkit registry check. - database + filesystem tests: replace print()-in-loop debug with assertions (or drop redundant prints) so integration runs don't spam the logs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
…tion tests unit-backend was a hand-kept file allowlist that had to grow with every new test dir. Collect the whole backend tree instead and let markers decide: tests needing live infra carry `@pytest.mark.integration` and are excluded from the unit tier via `-m "not integration"`. - register the `integration` marker in backend/pyproject.toml - mark the two DB-bound suites (dashboard_metrics, prompt_studio_registry_v2) that were previously grouped by path only - add a conftest marking the endpoint_v2 destination-connector subtree integration (uses django.test.TestCase -> needs Postgres). Kept out of the gating integration-backend group for now: 3 postgres destination tests are pre-existing failures and need a skip-guard/fix before they can gate. - remove the dead SharePoint `test_json_schema_has_is_personal` skip: the schema never exposes `is_personal`, so the test only ever asserted-then- skipped; drop it rather than carry a permanent skip. Verified: unit tier 116 passed / 50 deselected; integration-backend collects its marked suites; rig self-tests 54 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5HQX5CSoMR6RzHtXcfwJt
…gration API test Replace the scattered integration-marking (per-file pytestmark, per-app endpoint_v2 conftest) with a single backend/conftest.py hook that auto-marks any Django TestCase/TransactionTestCase or django_db item as integration — tests declare their DB need by how they're written, not a hand-kept marker. Cover the adapter-register-llm critical path honestly: unit-sdk1 only exercises the SDK provider classes, not the HTTP endpoint, so map it to a new integration-backend APITestCase that POSTs /adapter/ (SDK context-window call mocked, everything else real). Trim comments that would go stale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ation tier Follow-up completing 6d61a56, which staged only the adapter API test and the deleted per-dir conftest, leaving the rest of the batch uncommitted. - backend/conftest.py: central pytest_collection_modifyitems hook auto-marks every Django TestCase/TransactionTestCase/django_db test as `integration`, so unit-backend (-m "not integration") and integration-backend (-m integration) are exact complements. Drops now-redundant per-app pytestmark in dashboard_metrics and prompt_studio_registry_v2. - critical_paths.yaml: adapter-register-llm now covered_by integration-backend (real API test), reverting the earlier unit-sdk1 placeholder mapping. - groups.yaml: integration-backend gains the adapter API test and the destination-connectors DB-writer tests (BE orchestration over the connector lib — superset of the connector-lib DB tests). Adds WORKFLOW_EXECUTION_DIR_PREFIX to the shared backend test env (ExecutionFileHandler builds paths from it). - destination-connector postgres test: read DB_SCHEMA (was hardcoded "test"), use a lowercase table name (connector lowercases on read-back), drop the error-record case (hits a latent product edge: data=None serialized as the string 'None' into a jsonb column). - Delete 7 connector-lib DB tests (databases/test_*_db.py) — superseded by the backend DB-writer tests; keep test_sql_safety.py, filesystems, connectorkit. - rig cli.py / critical_paths.py: comment + docstring cleanups. Verified (testcontainers Postgres/Redis): unit-backend 116 passed, integration-backend 24 passed / 26 skipped (external-DB engines skip w/o creds), unit-connectors 53 passed, rig validate OK (15 groups, 9 paths). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chandrasekharan-zipstack
changed the title
chandrasekharan-zipstack
changed the title
chandrasekharan-zipstack
requested review from
hari-kuriakose,
muhammad-ali-e and
ritwik-g
…connector_v2 tests - groups.yaml: integration-backend now selects paths ["."] with -m integration — the exact complement of unit-backend over the same tree. New backend DB tests auto-join the group with no manifest edit. Verified equivalent: collect-only shows the same 50/166 tests (116 deselected); full run against testcontainers Postgres/Redis gives the same 24 passed / 26 skipped as the hand-listed paths on CI. - Delete backend/connector_v2/tests (connector_tests.py, conftest.py, fixture): written for the pre-v2 schema — the fixture loads removed models (account.org, account.user, project) so loaddata errors immediately, and the filename never matched pytest's test_*.py pattern, so these tests have not been collected anywhere. Same dead-test cleanup as the rest of this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
…backend-marker-paths
The previous version stubbed cross-app imports in sys.modules (only when absent) — written for a bare no-Django env. Under the rig, pytest-django boots Django and the real modules are already imported, so the stubs were skipped and the tests called reset_mock() on real classes (AttributeError). Same control-flow assertions, now via mock.patch.multiple on the imported module: no sys.modules mutation, no import-order dependence, safe in a shared pytest session. Stays in the unit tier (no DB touched). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
…ss flask check - test_build_index_payload.py: import the real prompt_studio_helper (Django is loaded by the rig env) instead of installing ~30 stub modules in sys.modules and restoring them post-import. The clobber/restore dance was import-order dependent and silently skipped all tests when the stub surface drifted. Tests and assertions unchanged; patches now managed via ExitStack. - test_legacy_executor_scaffold.py: test_no_flask_import now runs the check in a fresh subprocess. The in-place importlib.reload swapped class identities under sibling tests, and scanning sys.modules in-process fails if any other test imported flask. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
…compat Trigger-level paths-ignore means docs/frontend/docker-only PRs never start the workflow, so no check run is created and a required 'test' status check would block those PRs forever. A 'changes' job now always runs and gates 'test' via needs + if; a skipped job still reports a check run (skipped = passing for required checks), while any executed test failure still fails. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TGcZHkM4jbUmrx4YNYewZK
…guard test - python_files = [test_*.py, *_test.py, *_tests.py] in backend, workers, sdk1, connectors — one superset everywhere instead of three different configs. sdk1 loses the unused bare tests.py pattern (no such files). No currently existing file changes collection status (verified: zero *_tests.py in repo). - Remove test_no_flask_import: flask is not a workers dependency, so a flask import in the exceptions module would already fail every test that imports it; the Flask source it guarded against (prompt-service) is decommissioned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Accepting the bare `tests.py` Django convention surfaces two real backend suites (tenant_account_v2, account_v2 — 28 tests) that were written but never collected. All pass in integration-backend (24→52 passed, skips unchanged); unit-backend collection is untouched. Bare `test.py` is deliberately NOT accepted: backend/backend/settings/test.py is a Django settings module and would be imported as a test module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
The tiers share no state (separate runners, disjoint groups), so they run as matrix legs; wall clock drops from tier-sum to max(tiers). Everything needing both tiers' results moves to a new `report` job fed by per-tier artifacts: report aggregation, coverage union (tier .coverage files are suffixed so `coverage combine` can merge them), PR comment, and the main-branch baseline. Baseline writes were the one real conflict: parallel per-tier `run --update-baseline` would race the cache save (first save wins, the other tier's coverage is lost). Tier jobs now restore-only; the rig's `report combine` gains `--update-baseline` as the single writer, refusing the merge when any non-optional group is red or the baseline is corrupt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Lets `report` serve as the single required branch-protection check: a matrix job's result is success only when every leg passes, so either tier failing turns report red too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Trim verbose comments and drop PR-narrative phrasing that would go stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Critical-path regressions already gate PRs; the gap check was main-only to avoid failing unrelated PRs on pre-existing debt. With main enforced gap-free and PR CI running the merge ref, a gap failure on a PR can only be one the PR introduces — so catch it before merge, not after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
`branches: [main]` filters by base branch, so stacked PRs (targeting a feature branch) got no test runs at all until the stack reached main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UVfw7aAocC3KCJaEyZ5GU
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
ritwik-g
reviewed
Jul 7, 2026
ritwik-g
left a comment
ritwik-g
left a comment
Contributor
There was a problem hiding this comment.
Focused review of the rig/CI changes. All items below were verified against the branch; I've skipped everything the bots already resolved. Nothing here blocks the core approach (which is sound) — one is an operational merge step, one is a latent gate hole this PR widens, and two are coverage-loss judgment calls on deletions.
| # whatever results exist from both. | ||
| fail-fast: false | ||
| matrix: | ||
| tier: [unit, integration] |
Contributor
There was a problem hiding this comment.
The required status-check name changes when this merges — needs a branch-protection update in lockstep. On main the sole job was test, so GitHub emits a check run literally named test (almost certainly what branch protection currently requires). Converting test to a matrix makes GitHub emit test (unit) and test (integration) instead — the context test is never reported again. If the required check is still test, it will sit at "Expected — waiting for status" and block every PR (the exact failure the header comment at L11-13 warns about).
Since you mentioned you can either add a new required job or keep test: note that "keep test" no longer maps to a single context — you'd have to require the two matrix legs test (unit) / test (integration). The cleaner option (and what this file is designed around, see L210-215) is to require the report job as the single gate. Either way it's a repo-settings change that has to land with the merge; flagging it so open PRs don't get stranded.
| needs: test | ||
| # `always()` so a red tier still gets a PR report; skip only when the | ||
| # tiers were themselves skipped (irrelevant paths / draft). | ||
| if: always() && needs.test.result != 'skipped' |
Contributor
There was a problem hiding this comment.
A failed changes job silently reports the required check as green with zero tests run. test (L40) has needs: changes with an if that has no always()/failure(), so if changes itself fails (checkout / paths-filter / runner flake), test is skipped → report is skipped (this guard) → and a skipped required check counts as success (the mechanism relied on for docs-only PRs). The "Fail if any tier failed" failsafe at L210-215 only runs when report runs, so this path bypasses it entirely.
Low probability (changes is just checkout + dorny/paths-filter), but if changes isn't itself a required check, a green report could mask an untested merge. Options: make changes required too, or gate the skip explicitly, e.g. needs.changes.result == 'success' && needs.changes.outputs.relevant == 'false', so a broken gate turns report red instead of green-skip.
|
|
||
|
|
||
| def _green_group_names(results: list[GroupResult]) -> set[str]: | ||
| return {r.name for r in results if r.status in ("pass", "empty")} |
Contributor
There was a problem hiding this comment.
"empty" (0 tests collected, pytest exit 5) counts as coverage-green, which can produce false critical-path coverage. This set is the sole input to cp.evaluate(groups_run_green=...), and evaluate() marks a path covered if any covered_by group is in it — so a covering group that collected zero tests still satisfies covered_by.
This PR newly attaches a real path to that hole: adapter-register-llm → integration-backend, where integration-backend is marker-only (paths: ["."], -m integration) and its membership comes entirely from the backend/conftest.py auto-marker. If that marker ever stops matching (a refactor drops django_db/TestCase, the test moves, conftest logic changes), -m integration collects nothing → empty → green → adapter-register-llm reported ✅ covered with zero tests, and that gets baked into the main baseline. The regression gate is then blind to exactly the coverage loss it exists to catch. (Note the evaluate() docstring defines groups_run_green as "ran AND passed" — which empty violates.)
Latent — needs a second regression to trigger — but cheap to close: keep "empty" non-build-failing for optional/placeholder groups, but exclude it from the set passed as groups_run_green to evaluate() (require status == "pass" for coverage attestation). Optionally, have validate assert that any group in a path's covered_by collects >0 tests when it runs.
|
|
||
| return context | ||
|
|
||
| def test_execute_query_forbidden_billing(self): |
Contributor
There was a problem hiding this comment.
This deletion (and test_mariadb.py) isn't fully "superseded by the backend DB-writer tests." Unlike the other 5 deleted files, test_bigquery_db.py and test_mariadb.py are pure mock-based unit tests (no live DB, no skip guard) — they ran green in CI on main and were the only coverage of BigQuery's Forbidden/NotFound → exception mapping (+ the empty-detail branch in _format_exception_detail) and MariaDB's SSL-param build + 1045/2003 message mapping. The backend replacements only exercise Postgres (testcontainers) and skipTest without BigQuery/MariaDB creds, so that error-handling logic is now untested anywhere.
The other 5 (postgresql/mysql/mssql/redshift/snowflake) are skipUnless(creds) live-DB smoke tests that already skipped in CI — deleting those is fine. Suggest restoring just these two mock-only files into unit-connectors.
| f"✅ Successfully inserted test data into PostgreSQL table: {self.test_table_name}" | ||
| ) | ||
|
|
||
| def test_insert_into_db_with_error_postgresql(self) -> None: |
Contributor
There was a problem hiding this comment.
Deleting this test removes the only CI-runnable coverage of a real error-path bug. The commit drops it citing "a latent product edge: data=None serialized as the string 'None' into a jsonb column" — but that's a genuine shipping bug on a live path: failed files write an error record via insert_into_db(error=…), and get_sql_values_for_query falls through to f"{value}", writing the literal 'None' into the data_v2 jsonb column. The 7 sibling error tests (bigquery/mssql/mariadb/redshift/oracle/mysql/snowflake) all skipTest() without creds, so this Postgres case was the only error-path test that actually executed in CI (against the provisioned testcontainers Postgres).
The bug is pre-existing, but deleting the test with no code fix and no ticket papers it over. Suggest filing a ticket and either fixing it (write SQL/JSON NULL for None) + keeping the test, or xfail-ing it linked to the ticket rather than silently deleting the only runnable coverage of that path.
ritwik-g
approved these changes
Jul 7, 2026
ritwik-g
left a comment
ritwik-g
left a comment
Contributor
There was a problem hiding this comment.
Approving. This touches no production runtime code, and the core approach — gate scoping to in-tier coverage, marker-based unit/integration split via the central conftest auto-marker, testcontainers provisioning for the integration tier, and the parallel matrix CI + single-writer baseline — is sound and verified green.
One merge-time step (not a code change): the required status check must be repointed when this lands. test becomes a matrix, so the old test context stops reporting — require the report job (or both test (unit) / test (integration)) or PRs will hang on a missing check. Handled at merge.
Non-blocking follow-ups from the inline comments:
- Restore the two mock-only connector tests (
test_bigquery_db.py,test_mariadb.py) — they aren't actually superseded by the Postgres-only backend tests. - Track the
empty-counts-as-coverage-green gate hole (latent) and thedata=None→'None'jsonb error-path bug that the removed Postgres test covered.
None of these block the merge; they can be handled as fast-follows.
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
3 participants
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) a reliable, green, parallel check — the groundwork for making it a required PR merge gate (UN-3635). Checklist item #1 of UN-3636.Why
The job had never passed on
main:--fail-on-critical-gapfailed the build on every uncovered critical path, including paths only coverable by tiers that didn't run. Beyond that, backend test selection was hand-listed, several test files were dead or never collected, and the two tiers ran serially.How
Gating
--fail-on-critical-gapnow runs on PRs and main: main is gap-free and PRs test the merge ref, so a PR gap failure can only be self-introduced.Test selection & hygiene
backend/conftest.pyauto-marks DB-bound tests (TestCase/TransactionTestCase/django_db) asintegration;unit-backendandintegration-backendare exact complements overpaths: ["."]— no hand-kept lists.python_filesstandardized across backend/workers/sdk1/connectors:test_*.py,*_test.py,*_tests.py, plus Django's per-apptests.py— which surfaced 28 real, passing tests (tenant_account_v2,account_v2) that were never being collected.sys.modules(broke under whole-tree collection with Django loaded) tomock.patchon real modules:api_v2/tests/test_deployment_helper.py,prompt_studio_core_v2/tests/test_build_index_payload.py.adapter-register-llm(adapter_processor_v2/tests/test_adapter_api.py) — drivesPOST /api/v1/adapter/end-to-end.test_pandora_account.py+test_pubsub_helper.py, platform-servicetest_auth_middleware.py,connector_v2tests (v1-era fixtures, never collected), the redundant no-flask guard test, and the uncollectableunit-tool-registrygroup.tests/README.md: contributor guide — filename patterns, tier inference, seeding/mocking rules, local run commands.CI workflow
reportjob aggregates reports, unions coverage, posts the PR comment, and propagates tier failures — making it suitable as the single required check.report combine --update-baseline(new rig flag) is the sole writer, on green main builds only.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, CI workflow, and test files — no production/runtime code. Gate changes only relax out-of-tier failure conditions; 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
test (unit)+test (integration)in parallel,reportaggregating.unit-backend118 passed;integration-backend52 passed, 26 skipped (skips = destination-connector engine tests gated on external credentials); rig self-tests 59 passed;rig validateOK (15 groups, 9 critical paths).Deferred (separate PRs under UN-3636):
endpoint_v2/tests/destination-connectors/engine suites (skip-without-creds today).unit-workersmarker fix (collects 0 tests).Screenshots
Checklist
I have read and understood the Contribution Guidelines.