Skip to content

fix: testcase timing#4074

Merged
turip merged 1 commit into
mainfrom
fix/testcase-timing
Apr 2, 2026
Merged

fix: testcase timing#4074
turip merged 1 commit into
mainfrom
fix/testcase-timing

Conversation

@turip
Copy link
Copy Markdown
Member

@turip turip commented Apr 2, 2026

Overview

fix timing issue

Notes for reviewer

Summary by CodeRabbit

  • Tests
    • Updated internal test infrastructure for billing truncation assertions.

No user-facing changes in this release.

@turip turip requested a review from a team as a code owner April 2, 2026 15:59
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 2, 2026

📝 Walkthrough

Walkthrough

Test file modifications replace clock.SetTime() calls with clock.FreezeTime() and add defer clock.UnFreeze() cleanup for clock state management in truncation tests. One subsequent freeze operation lacks the accompanying unfreeze defer.

Changes

Cohort / File(s) Summary
Clock Control Test Updates
openmeter/billing/charges/service/truncation_test.go
Switches from SetTime() to FreezeTime() for clock management in TestCreateTruncatesFlatFeeIntentAndProrationInputs and TestUsageBasedAdvanceTruncatesPersistedCalculationTimestamps. Adds defer clock.UnFreeze() after initial freeze sites, but a second FreezeTime() call in the latter test lacks a corresponding unfreeze defer.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix: testcase timing' is vague and doesn't clearly convey what specific timing issue is being fixed or which test cases are affected. Consider using a more descriptive title like 'fix: use FreezeTime instead of SetTime in truncation tests' to clearly indicate the change being made.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/testcase-timing

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@turip turip added the release-note/bug-fix Release note: Bug Fixes label Apr 2, 2026
@turip turip enabled auto-merge (squash) April 2, 2026 16:00
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
openmeter/billing/charges/service/truncation_test.go (1)

153-153: Consider adding a brief comment for clarity.

This works correctly because the defer clock.UnFreeze() at line 112 will clean up all frozen state when the test exits (since the clock uses a single boolean flag, not reference-counting). However, a quick reader might wonder why there's no defer here.

Totally optional—maybe just a one-liner comment like // UnFreeze handled by defer above to make the intent obvious at a glance?

✨ Optional: Add clarifying comment
-	clock.FreezeTime(datetime.MustParseTimeInLocation(s.T(), "2026-02-01T00:02:00.900Z", time.UTC).AsTime())
+	clock.FreezeTime(datetime.MustParseTimeInLocation(s.T(), "2026-02-01T00:02:00.900Z", time.UTC).AsTime()) // UnFreeze handled by existing defer
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/service/truncation_test.go` at line 153, Add a
one-line clarifying comment next to the clock.FreezeTime(...) call in
truncation_test.go indicating that cleanup is handled by the earlier defer
clock.UnFreeze() (e.g., "// UnFreeze handled by defer above"), so readers
understand why there is no local defer; reference the clock.FreezeTime call and
the existing defer clock.UnFreeze to guide placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@openmeter/billing/charges/service/truncation_test.go`:
- Line 153: Add a one-line clarifying comment next to the clock.FreezeTime(...)
call in truncation_test.go indicating that cleanup is handled by the earlier
defer clock.UnFreeze() (e.g., "// UnFreeze handled by defer above"), so readers
understand why there is no local defer; reference the clock.FreezeTime call and
the existing defer clock.UnFreeze to guide placement.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 20047896-5c42-4718-9b58-4cbf1cc549bb

📥 Commits

Reviewing files that changed from the base of the PR and between 494829a and fb8cfb9.

📒 Files selected for processing (1)
  • openmeter/billing/charges/service/truncation_test.go

@turip turip merged commit d72ff62 into main Apr 2, 2026
25 of 28 checks passed
@turip turip deleted the fix/testcase-timing branch April 2, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/bug-fix Release note: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants