Skip to content

test: add additional regression tests for restore_metrics telemetry sync (#13843)#2793

Merged
VascoSch92 merged 3 commits intoOpenHands:mainfrom
Jinhaooo:fix/restore-metrics-telemetry-sync-v2
Apr 10, 2026
Merged

test: add additional regression tests for restore_metrics telemetry sync (#13843)#2793
VascoSch92 merged 3 commits intoOpenHands:mainfrom
Jinhaooo:fix/restore-metrics-telemetry-sync-v2

Conversation

@Jinhaooo
Copy link
Copy Markdown
Contributor

@Jinhaooo Jinhaooo commented Apr 10, 2026

Summary

The fix for restore_metrics() telemetry desync was merged in #2736. This PR adds additional regression tests that complement the existing test_issue_2459_restore_metrics_syncs_telemetry test with edge-case coverage:

  • Cost propagation: costs added via telemetry after restore land in the restored Metrics
  • Stale metrics isolation: the original (pre-restore) Metrics object does not receive new costs
  • Telemetry=None safety: restore_metrics() does not crash when telemetry has not been initialized
  • End-to-end ConversationStats: ConversationStats restores metrics, then new costs are tracked correctly

Test plan

  • All 4 new tests pass: pytest tests/sdk/llm/test_restore_metrics_telemetry_sync.py
  • Existing tests unaffected: pytest tests/sdk/llm/test_llm.py

Related issue: OpenHands/OpenHands#13843

🤖 Generated with Claude Code

@VascoSch92
Copy link
Copy Markdown
Contributor

Hey @Jinhaooo

Thanks for your contribution!

We actually had this bug in the SDK already, and I merged a fix this morning just before seeing your PR (see #2736 and #2459).

However, if you'd like to rebase on main, we would still love to merge the tests you wrote! :-)

@Jinhaooo
Copy link
Copy Markdown
Contributor Author

Jinhaooo commented Apr 10, 2026 via email

@VascoSch92
Copy link
Copy Markdown
Contributor

VascoSch92 commented Apr 10, 2026

Hi! Thanks for the heads-up! Since this is my first-ever PR, I'd be more than happy to see my tests merged into the codebase. :-) Vasco Schiavo @.> 于2026年4月10日周五 16:44写道:

VascoSch92 left a comment (OpenHands/software-agent-sdk#2793) <#2793 (comment)> Hey @Jinhaooo https://github.com/Jinhaooo Thanks for your contribution! We actually had this bug in the SDK already, and I merged a fix this morning just before seeing your PR (see #2736 <#2736> and #2459 <#2459>). However, if you'd like to rebase on main, we would still love to merge the tests you wrote! :-) — Reply to this email directly, view it on GitHub <#2793 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/BIW2UI5SLSN4OJ7VPO3H73L4VCYARAVCNFSM6AAAAACXTQWHOGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMRSGMYDMMZVG4 . You are receiving this because you were mentioned.Message ID: @.
>

Cool... So please merge your tests with the tests I added :-).

You can ping me for a review

Add edge-case tests complementing test_issue_2459_restore_metrics_syncs_telemetry:
- cost propagation through telemetry after restore
- stale metrics isolation (original object not updated)
- telemetry=None safety guard
- end-to-end ConversationStats integration

Related issue: OpenHands/OpenHands#13843

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Jinhaooo Jinhaooo force-pushed the fix/restore-metrics-telemetry-sync-v2 branch from 23303b7 to 477486c Compare April 10, 2026 11:10
@Jinhaooo Jinhaooo changed the title fix: sync telemetry.metrics in restore_metrics() to fix post-resume cost tracking (#13843) test: add additional regression tests for restore_metrics telemetry sync (#13843) Apr 10, 2026
@Jinhaooo
Copy link
Copy Markdown
Contributor Author

Rebased on main and removed the duplicate test. Four new test cases remain that cover scenarios not in the existing test. Ready for review @VascoSch92!

@VascoSch92
Copy link
Copy Markdown
Contributor

Just one last thing and then we are good to merge. Please add these tests under my test: test_issue_2459_restore_metrics_syncs_telemetry().

Move the 4 additional regression tests next to
test_issue_2459_restore_metrics_syncs_telemetry and remove the
separate test_restore_metrics_telemetry_sync.py file.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Jinhaooo
Copy link
Copy Markdown
Contributor Author

Done! @VascoSch92

@VascoSch92 VascoSch92 merged commit 56cd2ab into OpenHands:main Apr 10, 2026
21 checks passed
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