Skip to content

test(integration): allow skipped nodes and drop the shard cap - #42687

Merged
yuneng-berri merged 1 commit into
mainfrom
litellm_integration_allow_skips
Sep 23, 2026
Merged

yuneng-berri merged 1 commit into
mainfrom
litellm_integration_allow_skips

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • One pytest.skip turns a CircleCI integration-<suite> shard red
  • The upcoming MCP tests skip on known product bugs, so skips must be allowed
  • The 11-minute whole-shard cap kills long shards mid-run

How it solves it:

  • execution.json gains a skipped list and complete accepts passed or skipped nodes
  • run.py accepts passed + skipped == manifest and prints skip reasons with -rs
  • run_integration.sh runs the shard without the timeout wrapper; per-test --timeout=90 stays

User Flow

Before: a contributor adds an integration test that skips on a known bug and the shard fails

  1. They add a test that calls pytest.skip("<symptom>") and register it in contracts.json
  2. They push and CircleCI runs integration-extensions
  3. pytest reports 1 skipped, exits 0, but execution.json says "complete": false and the job exits 1
  4. The shard is red with no failing test to look at

After: the same push is green and the skip is listed as evidence

  1. They add the same skipping test and register it in contracts.json
  2. They push and CircleCI runs integration-extensions
  3. pytest reports 1 skipped with its reason under -rs, execution.json lists the node under skipped with "complete": true, the job exits 0
  4. The shard is green and the skipped list is the open bug list

A failed test, a setup or teardown error, a manifest node that was not collected, a collected node not in the manifest, or a node with no report still fail the shard

Relevant issues

Affected release

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*, make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

Screenshots / Proof of Fix

This is a test harness change, so the proof is the harness itself run against a scratch file. Shared setup: a scratch file tests/integration/sdk/test_scratch_skip.py with one passing test and one pytest.skip("scratch: simulated product bug"), both nodes temporarily registered in contracts.json. Neither the file nor the manifest edit is committed. The direct pytest command is

INTEGRATION_RESULTS_DIR=/tmp/skip LITELLM_LOCAL_MODEL_COST_MAP=True PYTHONPATH=$PWD:$PWD/tests:$PWD/tests/e2e \
  uv run --no-sync python -m pytest tests/integration/sdk/test_scratch_skip.py -vv -rs -p no:pytest-retry -p no:rerunfailures --strict-markers; echo exit=$?

Before (3190d93)

one pass and one skip

  1. Run the pytest command above
  2. 1 passed, 1 skipped, pytest exit 0, then the session hook flips it: exit=1, execution.json has "complete": false and no record of the skipped node

After (3739587)

one pass and one skip

  1. Run the pytest command above
  2. Output
tests/integration/sdk/test_scratch_skip.py::test_scratch_passes PASSED   [ 50%]
tests/integration/sdk/test_scratch_skip.py::test_scratch_skips SKIPPED   [100%]
SKIPPED [1] tests/integration/sdk/test_scratch_skip.py:11: scratch: simulated product bug
========================= 1 passed, 1 skipped in 0.04s =========================
exit=0
  1. cat /tmp/skip/execution.json
"passed": ["tests/integration/sdk/test_scratch_skip.py::test_scratch_passes"],
"skipped": ["tests/integration/sdk/test_scratch_skip.py::test_scratch_skips"],
"complete": true,
"exitstatus": 0
  1. Through the runner: uv run --no-sync python tests/integration/run.py sdk --results /tmp/skip2; echo exit=$? gives 3 passed, 1 skipped and exit=0 (the two real sdk http2 nodes plus the scratch pair)

pass changed to a failing assert

  1. Change assert True to assert False and rerun the pytest command
  2. 1 failed, 1 skipped, exit=1, execution.json has "complete": false, "passed": [], and the skip still listed

skip raised from a fixture in setup

  1. Move the pytest.skip into a fixture used by the second test and rerun
  2. SKIPPED [1] tests/integration/sdk/test_scratch_skip.py:14: scratch: simulated product bug in setup, exit=0, "complete": true, the node appears in skipped exactly once

manifest node missing from collection

  1. Add a phantom node tests/integration/sdk/test_scratch_skip.py::test_scratch_missing to contracts.json and run uv run --no-sync python tests/integration/run.py sdk --results /tmp/skip3; echo exit=$?
  2. 3 passed, 1 skipped, then Executed integration nodes differ from the canonical manifest on stderr and exit=1

Type

✅ Test
🚄 Infrastructure

Caveats (if any)

Low

  • report.skipped is also true for xfail outcomes, so an xfail would count as skipped too; xfail handling is out of scope here
  • Without the shard cap, a hung shard now runs until CircleCI's own job timeout instead of 11 minutes; per-test --timeout=90 and the timeout markers still bound each test

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

Link to Devin session: https://app.devin.ai/sessions/85077876371742d6b1f45a0c3ba34fa2
Open in Devin Desktop: https://app.devin.ai/desktop/session/85077876371742d6b1f45a0c3ba34fa2?variant=devin

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
devin-ai-integration Bot requested a review from a team September 23, 2026 05:49
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@greptile-apps

greptile-apps Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The implementation is close, but the explicit test-integrity requirements must be satisfied before merging

Findings

  1. P2 Skips Can Mask Coverage ▶
  2. P2 Regression Coverage Missing ▶

Summary

This PR changes integration qualification so manifest nodes may either pass or skip, prints skip reasons, and removes the non-browser shard-level timeout

  • Adds skipped nodes to execution.json and includes them in completeness checks
  • Updates the runner and contributor documentation for passed-or-skipped qualification
  • Leaves the broader skip policy and its new behavior without a committed regression test

Reviews (1) · Last reviewed commit: "test(integration): allow skipped nodes a..."

Comment on lines +84 to +89
skipped: Final = tuple(report.nodeid for report in reports if report.skipped)
complete: Final = (
exitstatus == 0
and bool(collected)
and sorted(collected) == sorted(passed)
and all(report.passed for report in reports)
and sorted(collected) == sorted(passed + skipped)
and not any(report.failed for report in reports)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Skips Can Mask Coverage Accepting every skip lets an entirely skipped manifest qualify, violating the repository rule against weakening modified tests. This must be resolved before merging

Rule Used: What: Flag any modifications to existing tests and verify they don't weaken test coverage or mask regressions. Why: Developers may alter tests to make failing code pass rather than fix the actual bug, hiding regressions. Good: ``` // Test updated t... (source)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Intentional. Skipped tests mark known product bugs, so the skip list is the bug list. Reasons print via -rs and land in execution.json

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That context makes the behavior intentional: skipped nodes are explicitly retained as the known-product-bug inventory, while failed, uncollected, or unreported nodes still make the shard fail. I’m withdrawing the coverage concern.

One small clarification: execution.json records skipped node IDs; the skip reasons themselves are emitted by pytest’s -rs output rather than serialized into that JSON.

Comment on lines +84 to +89
skipped: Final = tuple(report.nodeid for report in reports if report.skipped)
complete: Final = (
exitstatus == 0
and bool(collected)
and sorted(collected) == sorted(passed)
and all(report.passed for report in reports)
and sorted(collected) == sorted(passed + skipped)
and not any(report.failed for report in reports)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Regression Coverage Missing This qualification change lacks committed tests for call and setup skips or teardown failures, violating the bug-fix regression-test requirement. This must be resolved before merging

Context Used: AGENTS.md (source)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a runner policy change, not a product fix. Pass, skip, failure and teardown-error cases were checked by hand; the next MCP PR rewrites this runner

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That context is fair. This is a test-runner policy change rather than a product bug fix, and the PR documents hands-on validation for call skips, setup skips, failures, and teardown errors. Given that the runner is expected to be rewritten in the upcoming MCP PR, I won't require additional committed regression tests here and withdraw this finding.

@codecov

codecov Bot commented Sep 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yuneng-berri yuneng-berri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified locally that skip exits 0 while failures and teardown errors still exit 1. Windows red is a uv sync timeout

@yuneng-berri
yuneng-berri merged commit 24a05b2 into main Sep 23, 2026
132 of 143 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_integration_allow_skips branch September 23, 2026 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant