Skip to content

test(utils): raise the post-success hook error from a guardrail in the failure-hook regression - #42646

Merged
yuneng-berri merged 2 commits into
mainfrom
litellm_fix_stale_post_success_hook_test
Sep 23, 2026
Merged

yuneng-berri merged 2 commits into
mainfrom
litellm_fix_stale_post_success_hook_test

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

How it solves it:

User Flow

Before: a contributor opening a PR into main sees the required misc / Run tests check fail on a test they never touched, on every push

  1. They push a branch and open a PR into main from https://github.com/BerriAI/litellm/compare
  2. On the PR's Checks tab, misc / Run tests from test-unit.yml turns red while the other shards go green
  3. They open the job log, for example https://github.com/BerriAI/litellm/actions/runs/35806170176/job/107007408694, and read FAILED tests/test_litellm/test_utils.py::test_wrapper_async_does_not_fire_failure_hook_for_post_success_error - Failed: DID NOT RAISE <class 'RuntimeError'> and 1 failed, 6532 passed, 25 skipped
  4. They open https://github.com/BerriAI/litellm/actions/workflows/test-unit.yml?query=branch%3Amain and see every main run since 944f44d red on that same test, so a re-run cannot help and their PR cannot show all required checks green

After: the same contributor's PR gets a green misc / Run tests check

  1. They push a branch and open a PR into main from https://github.com/BerriAI/litellm/compare
  2. On the PR's Checks tab, misc / Run tests from test-unit.yml goes green with the other shards
  3. They open the job log and read the whole shard passing with zero failures
  4. https://github.com/BerriAI/litellm/actions/workflows/test-unit.yml?query=branch%3Amain shows main green on this shard again from the merge onward

Relevant issues

No linked issue. Follow-up to #42535, which changed the behavior the test expected, and #42603, which brought the misc shard back so the stale test ran again

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

Shared setup: a fresh worktree at the named commit with make bootstrap, then the one test main's misc shard fails on

uv run pytest "tests/test_litellm/test_utils.py::test_wrapper_async_does_not_fire_failure_hook_for_post_success_error" -p no:cacheprovider --no-cov

Before (1953603)

  1. Run the command above at main's tip
  2. Output: E Failed: DID NOT RAISE <class 'RuntimeError'>, then FAILED tests/test_litellm/test_utils.py::test_wrapper_async_does_not_fire_failure_hook_for_post_success_error and 1 failed, 76 warnings in 6.81s
  3. The same failure on main's own misc / Run tests at this commit, https://github.com/BerriAI/litellm/actions/runs/35806170176/job/107007408694: 2 workers [6558 items], then 1 failed, 6532 passed, 25 skipped, 509 warnings, 2 rerun in 391.64s

After (17b9dd3)

  1. Run the same command at this PR's tip
  2. Output: 1 passed, 76 warnings in 0.59s
  3. The neighboring hook tests, uv run pytest tests/test_litellm/test_utils.py -k "success_deployment_hook or failure_hook_for" -p no:cacheprovider --no-cov: 15 passed, 380 deselected (was 1 failed, 14 passed at the merge base)
  4. Mutation check, the wrapper routing a post-success error into the failure hook: the test fails with Left contains one more item: RuntimeError('boom in success hook, model call itself succeeded')
  5. Mutation check, a guardrail's success-hook error isolated like a logger's: the test fails with Failed: DID NOT RAISE <class 'RuntimeError'>
  6. The PR's own misc / Run tests job, https://github.com/BerriAI/litellm/actions/runs/35808610579/job/107015265898: 2 workers [6588 items] then 6563 passed, 25 skipped, 509 warnings in 442.80s (main's run 107007408694 had 1 failed on 6558 items)

Type

✅ Test

Caveats (if any)

Low

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

  • 17b9dd3 passes /live-pr-risk

…e failure-hook regression

Since #42535 a plain logger raising inside async_post_call_success_deployment_hook
is logged and the completed request returns, so the regression added by #36657 for
"a post-success error never reaches async_post_call_failure_deployment_hook" failed
with DID NOT RAISE on every main run once #42603 revived the misc unit shard. The
raising callback is now a CustomGuardrail, the one kind of callback whose
post-success raise still propagates, which keeps the original assertions intact
@devin-ai-integration

devin-ai-integration Bot commented Sep 23, 2026 •

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: 5/5

The test-only change appears safe to merge and no actionable issues remain

Summary

This PR updates a post-success failure-hook regression test to use a guardrail, whose callback errors still propagate, while preserving the assertion that deployment failure hooks are not invoked. The latest change replaces the bare dict annotation with Mapping[str, object]

Reviews (2) · Last reviewed commit: "test(utils): type the guardrail's succes..."

Comment thread tests/test_litellm/test_utils.py Outdated
@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot 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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 17b9dd3. Configure here.

@yuneng-berri
yuneng-berri enabled auto-merge (squash) September 23, 2026 02:09
@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!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants