fix: integration tests #3345
fix: integration tests #3345
Conversation
📝 WalkthroughWalkthroughUpdates the customer–subject hook constructor and wiring to remove billing overrides, refactors and expands entitlement and subscription tests (including new name generator), adjusts Stripe customer upsert conflict keys, changes invoice line diff child handling, tweaks credit helper error detection, and removes multiple migration tests and sqlc testdata. Adds a new test noop override service and a new module dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Pre-merge checks (1 warning, 2 inconclusive)❌ Failed checks (1 warning, 2 inconclusive)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (58)
openmeter/entitlement/adapter/entitlement_test.go (3)
472-472: Also assert IsSoftLimit round-trips.You’re setting IsSoftLimit on create but never verify it when loading or after typed mapping. Add an assertion to ensure the flag is persisted and preserved through ParseFromGenericEntitlement to avoid silent regressions.
425-429: Make pagination deterministic by specifying Limit on the second page.The second query relies on an implicit default limit; this can flake if defaults change. Pass Limit explicitly.
next5Ents, err := repo.entRepo.ListActiveEntitlementsWithExpiredUsagePeriod(ctx, entitlement.ListExpiredEntitlementsParams{ Namespaces: []string{ns}, Highwatermark: time.Date(2025, 5, 1, 0, 0, 0, 0, time.UTC), Cursor: resetableEnts[4].ID, + Limit: 5, })
306-306: Avoid duplicate ResetTime defers; use t.Cleanup once.Two defers reset the clock in the same subtest. Use a single cleanup to keep it tidy.
- clock.SetTime(now) - defer clock.ResetTime() + clock.SetTime(now) + t.Cleanup(clock.ResetTime) ... - clock.SetTime(now) - defer clock.ResetTime() + clock.SetTime(now)Also applies to: 351-353
openmeter/app/stripe/adapter/customer.go (1)
137-144: Enrich warning log context for faster triageConsider adding namespace and Stripe account to the warn log to speed up debugging in multi-tenant setups.
Apply:
- a.logger.WarnContext(ctx, "failed to upsert app stripe customer data", + a.logger.WarnContext(ctx, "failed to upsert app stripe customer data", "error", err, + "namespace", input.AppID.Namespace, "app_id", input.AppID.ID, "customer_id", input.CustomerID.ID, "stripe_customer_id", input.StripeCustomerID, + "stripe_account_id", stripeAppData.StripeAccountID, )openmeter/entitlement/service/scheduling_test.go (13)
32-34: Prefer t.Cleanup over defer for teardown; minor naming nit.Use t.Cleanup to bind resource cleanup to the test lifecycle and avoid surprises in subtests. Also, if feasible, consider correcting setupDependecies → setupDependencies for clarity.
conn, deps := setupDependecies(t) -defer deps.Teardown() +t.Cleanup(deps.Teardown)
66-90: Reduce brittleness: assert on substrings, not full error text.Exact message matches are fragile. Assert that the error contains the key phrase instead.
-assert.EqualError(t, err, "activeTo and activeFrom are not supported in CreateEntitlement") +require.Error(t, err) +assert.ErrorContains(t, err, "not supported in CreateEntitlement") ... -assert.EqualError(t, err, "activeTo and activeFrom are not supported in CreateEntitlement") +require.Error(t, err) +assert.ErrorContains(t, err, "not supported in CreateEntitlement")
95-101: Freeze clock per subtest to avoid global drift leakage.clock.SetTime mutates global drift and can leak across subtests or parallel runs. Prefer FreezeTime + t.Cleanup(UnFreeze) for isolation.
-clock.SetTime(testutils.GetRFC3339Time(t, "2024-01-03T00:00:00Z")) +clock.FreezeTime(testutils.GetRFC3339Time(t, "2024-01-03T00:00:00Z")) +t.Cleanup(clock.UnFreeze)Also, make err local in each subtest to avoid capturing the outer err.
ctx := t.Context() +var err errorApply similarly in other subtests that call clock.SetTime and reuse err.
Also applies to: 136-151
171-174: Use the created feature key instead of a hard-coded literal.Keeps tests resilient to future changes to generated keys.
-FeatureKey: lo.ToPtr("feature1"), +FeatureKey: lo.ToPtr(feat.Key),Also applies to: 180-184
282-285: Use require.ErrorAs for type assertion.Stop early if the type assertion fails; avoids nil deref risk on conflictErr.
-var conflictErr *entitlement.AlreadyExistsError -assert.ErrorAsf(t, err, &conflictErr, "expected error to be of type %T", conflictErr) +var conflictErr *entitlement.AlreadyExistsError +require.ErrorAs(t, err, &conflictErr, "expected error to be of type %T", conflictErr)
316-316: Upgrade to require.NoError for setup step.Failure here invalidates the rest of the assertions.
-assert.Nil(t, err) +require.NoError(t, err)
340-416: Stabilize time anchor in metered test.Use clock.Now() instead of time.Now() to respect the test clock; reduces flakiness.
-Anchor: time.Now(), +Anchor: clock.Now(),
483-484: Assert error semantics without depending on full message.Match on key parts to keep the test resilient across refactors.
-assert.EqualError(t, err, fmt.Sprintf("entitlement not found bogus-id in namespace %s", namespace)) +require.Error(t, err) +assert.ErrorContains(t, err, "entitlement not found") +assert.ErrorContains(t, err, namespace)
559-559: Upgrade to require.NoError for setup step.-assert.Nil(t, err) +require.NoError(t, err)
699-699: Upgrade to require.NoError for setup step.-assert.Nil(t, err) +require.NoError(t, err)
741-741: Use require.NoError for critical steps in “current time” test.These are gating errors; fail fast improves signal.
-assert.Nil(t, err) +require.NoError(t, err)(Apply to all three occurrences.)
Also applies to: 758-758, 761-761
763-763: Grammar nit in comment.-// This assertions could fail if the test is slow +// These assertions could fail if the test is slow
466-466: Standardize time control in scheduling_test.go
Replace each directclock.SetTime(...)/clock.ResetTime()pair in the subtests with a call toclock.FreezeTime(...)at the start of the subtest and registert.Cleanup(clock.Unfreeze)so that each subtest’s clock state is isolated (and can safely run in parallel).Affected lines in openmeter/entitlement/service/scheduling_test.go: 97, 158, 191, 240, 292, 466, 491, 538, 582, 628, 677.
pkg/timeutil/recurrence.go (1)
110-110: Replace TODO with explicit guard and actionable error.Backwards iteration limit “1733” is opaque. Add a documented min-supported time constant and return a typed error when crossing it. Also enrich the “too many iterations” error with anchor/t/iter for diagnostics.
Example patch:
@@ - // TODO: Right now we cannot iterate backwards past 1733, please fix this. + // Guard against known backward-iteration floor (document why in code). + const minSupportedYear = 1733 + if res.Year() < minSupportedYear { + return RecurrenceIterator{}, fmt.Errorf( + "recurrence.NextAfter: backward iteration earlier than %d-01-01 unsupported (anchor=%s t=%s iter=%d)", + minSupportedYear, r.Anchor.UTC().Format(time.RFC3339), t.UTC().Format(time.RFC3339), ic) + } @@ - if ic <= -MAX_SAFE_ITERATIONS { - return RecurrenceIterator{}, fmt.Errorf("recurrence.NextAfter: too many iterations") + if ic <= -MAX_SAFE_ITERATIONS { + return RecurrenceIterator{}, fmt.Errorf("recurrence.NextAfter: too many iterations (anchor=%s t=%s iter=%d)", + r.Anchor.UTC().Format(time.RFC3339), t.UTC().Format(time.RFC3339), ic) }test/subscription/scenario_firstofmonth_test.go (1)
30-31: Reduce reliance on hardcoded framework namespace (if possible).If the harness allows, prefer a unique namespace per test run to avoid cross-test bleed. Otherwise, add a brief rationale here to silence future churn.
openmeter/credit/helper.go (1)
30-51: Good: use ErrorsAs to detect wrapped “no snapshot” error; minor logging improvement.Suggestion: capture the typed error and include its Owner/Time in the debug log for better traces.
- if _, ok := lo.ErrorsAs[*balance.NoSavedBalanceForOwnerError](err); ok { + if noBalErr, ok := lo.ErrorsAs[*balance.NoSavedBalanceForOwnerError](err); ok { - m.Logger.Debug(fmt.Sprintf("no saved balance found for owner %s before %s, calculating from start of time", owner.ID, at)) + m.Logger.Debug(fmt.Sprintf("no saved balance found (owner=%s time=%s, reportedAt=%s), calculating from start of time", + owner.ID, at, noBalErr.Time))test/subscription/scenario_editcancel_test.go (1)
135-136: Prefer generated, collision-resistant subject keys in tests.Use the new NameGenerator to avoid fixed “subject_N” collisions when tests run concurrently.
- SubjectKeys: []string{fmt.Sprintf("subject_%d", i+2)}, + SubjectKeys: []string{testutils.NameGenerator.Generate().Key},openmeter/testutils/namegen.go (3)
11-11: Expose a concurrency-safe generator or make Generate atomic.Tests may call Generate() in parallel. Guard the underlying generator unless it’s documented thread-safe.
@@ -var NameGenerator = newNameGenerator() +var NameGenerator = newNameGenerator()If you accept, apply the struct/method changes below.
3-9: Thread-safety and slug robustness.
- Add a mutex around Generate to prevent races.
- Use slug.Make to build URL-safe keys (handles unicode/punctuation).
@@ -import ( - "strings" - "time" - - "github.com/forscht/namegen" - "github.com/forscht/namegen/dictionaries" -) +import ( + "strings" + "sync" + "time" + + "github.com/forscht/namegen" + "github.com/forscht/namegen/dictionaries" + "github.com/gosimple/slug" +) @@ type nameGenerator struct { *namegen.Generator + mu sync.Mutex } @@ func (g *nameGenerator) Generate() GeneratedName { - name := g.Generator.WithStyle(namegen.Title).WithWordSeparator(" ").Generate() + g.mu.Lock() + defer g.mu.Unlock() + name := g.Generator.WithStyle(namegen.Title).WithWordSeparator(" ").Generate() return GeneratedName{ - Key: strings.ReplaceAll(strings.ToLower(name), " ", "-"), + Key: slug.Make(name), // requires adding github.com/gosimple/slug as a direct dep Name: name, } }Note: add
github.com/gosimple/slug v1.15.0to go.mod require if not already direct.Also applies to: 13-16, 31-38
17-24: Optional: seed control for deterministic runs.Expose a helper to set a fixed seed via env (e.g., NAMEGEN_SEED) to reproduce flaky tests.
func newNameGenerator() *nameGenerator { - return &nameGenerator{ - Generator: namegen.New(). - WithSeed(time.Now().UnixNano()). + seed := time.Now().UnixNano() + if v := strings.TrimSpace(strings.ToLower(strings.TrimSpace(strings.TrimPrefix(strings.TrimPrefix(strings.TrimSpace(""), "NAMEGEN_SEED="), "")))); v != "" { + // keep simple; or parse from os.Getenv("NAMEGEN_SEED") + } + return &nameGenerator{ + Generator: namegen.New(). + WithSeed(seed). WithDictionaries(dictionaries.Adjectives, dictionaries.Animals). WithNumberOfWords(2), } }Alternatively, a simple
NewWithSeed(seed int64)constructor in tests.openmeter/entitlement/service/access_test.go (7)
20-21: Nit: fix typo in helper name for clarityConsider renaming setupDependecies -> setupDependencies across helpers/call sites.
23-27: Assert expected-first (or empty) to improve failure messagesSwap expected/actual in the equality, or just assert empty.
- require.Equal(t, access, entitlement.Access{}) + require.Equal(t, entitlement.Access{}, access) + // or: + // require.Empty(t, access)
83-90: Use contains + boolean helpers; map value “not nil” check is ineffectiveMap lookup returns a value (not a pointer), so NotNil won’t detect absence. Also prefer True() over Equal(..., true).
- require.NotNil(t, access.Entitlements[featureKey]) - require.Equal(t, access.Entitlements[featureKey].Value.HasAccess(), true) + require.Contains(t, access.Entitlements, featureKey) + require.True(t, access.Entitlements[featureKey].Value.HasAccess())
152-155: Prefer True() for boolean assertionsThis tightens failure output and intent.
- require.Equal(t, ent.Value.HasAccess(), true) + require.True(t, ent.Value.HasAccess())
267-270: Prefer True() for boolean assertions (repeat)Consistent with other subtests.
- require.Equal(t, access.Entitlements["test-bool"].Value.HasAccess(), true) - require.Equal(t, access.Entitlements["test-static"].Value.HasAccess(), true) - require.Equal(t, access.Entitlements["test-metered"].Value.HasAccess(), true) + require.True(t, access.Entitlements["test-bool"].Value.HasAccess()) + require.True(t, access.Entitlements["test-static"].Value.HasAccess()) + require.True(t, access.Entitlements["test-metered"].Value.HasAccess())
69-76: Avoid passing both FeatureKey and FeatureID unless the API requires itIf only one is needed, pass a single selector to prevent accidental mismatches in future refactors.
Also applies to: 132-138, 196-204, 237-248, 311-317
252-254: Typo in comment“streming” -> “streaming”.
- // We need to add an event so streming mock finds the meter + // We need to add an event so the streaming mock finds the meteropenmeter/entitlement/metered/balance_test.go (2)
195-205: Avoid shadowing imported package name “entitlement” with local variableRename local entitlement := ... to ent (or e) to prevent confusion and accidental shadowing of the imported package identifier.
Example patch for one occurrence (apply similarly to others):
- entitlement, err := deps.entitlementRepo.CreateEntitlement(ctx, inp) + ent, err := deps.entitlementRepo.CreateEntitlement(ctx, inp) require.NoError(t, err) - entBalance, err := connector.GetEntitlementBalance(ctx, models.NamespacedID{Namespace: namespace, ID: entitlement.ID}, queryTime) + entBalance, err := connector.GetEntitlementBalance(ctx, models.NamespacedID{Namespace: namespace, ID: ent.ID}, queryTime)Also applies to: 255-265, 366-376, 493-505, 556-563
300-305: Comment typo: “vaild” → “valid”- // get last vaild snapshot + // get last valid snapshotAlso applies to: 532-535
openmeter/entitlement/metered/lateevents_test.go (4)
232-236: Use t.Context() for per-test cancellation/trace propagationAligns with other suites and avoids leaked contexts.
- ctx := context.Background() + ctx := t.Context()
251-252: Remove leftover subject bootstrap; it no longer matches the generated subjectAfter switching to randName-based subjects, creating “subject1” is redundant and can confuse future readers.
- // ensure subject exists for SubjectKey used in entitlement - _, _ = deps.dbClient.Subject.Create().SetNamespace(namespace).SetKey("subject1").Save(ctx)
258-284: Avoid shadowing: rename local entitlement variableSame rationale as in balance_test.go.
- entitlement, err := deps.entitlementRepo.CreateEntitlement(ctx, inp) + ent, err := deps.entitlementRepo.CreateEntitlement(ctx, inp) assert.NoError(t, err) - entBalance, err := connector.GetEntitlementBalance(ctx, models.NamespacedID{Namespace: namespace, ID: entitlement.ID}, queryTime) + entBalance, err := connector.GetEntitlementBalance(ctx, models.NamespacedID{Namespace: namespace, ID: ent.ID}, queryTime)
238-246: Prefer require.NoError for setup stepsFail fast on fixture creation to avoid cascading assertions on invalid state.
- feature, err := deps.featureRepo.CreateFeature(ctx, exampleFeature) - assert.NoError(t, err) + feature, err := deps.featureRepo.CreateFeature(ctx, exampleFeature) + require.NoError(t, err)openmeter/testutils/namegen_test.go (1)
10-18: Solid smoke test; consider a tiny robustness check.The assertions are good. Optionally add a second Generate() call and assert keys differ to catch accidental static seeding/caching.
t.Run("Generate", func(t *testing.T) { names := NameGenerator.Generate() t.Logf("Generated name: %v", names) require.NotEmpty(t, names) assert.NotEmpty(t, names.Key) assert.NotEmpty(t, names.Name) + + // Optional: ensure subsequent calls don't collide trivially + names2 := NameGenerator.Generate() + assert.NotEqual(t, names.Key, names2.Key) })openmeter/subscription/workflow/service/addon_test.go (2)
63-64: Minor consistency nit: prefer t.Context() over context.Background() in tests.Not required, but propagating test context helps cancellation on failure/timeouts.
Also applies to: 99-104, 230-233, 285-286, 337-338, 459-462, 513-514, 578-579, 727-729
34-44: Deduplicate runWithDeps helpers.You define similar closures in multiple tests. Consider a shared package-level helper to reduce repetition.
Also applies to: 308-319, 652-661
openmeter/entitlement/metered/utils_test.go (2)
204-204: Prefer time.Equal over string formatting for anchor comparisonComparing formatted strings is brittle and allocates. Use time.Equal which ignores location and compares instants precisely.
- assert.Equal(t, expected.GetValue().Anchor.UTC().Format(time.RFC3339), actual.GetValue().Anchor.UTC().Format(time.RFC3339), "anchors do not match") + assert.True(t, expected.GetValue().Anchor.Equal(actual.GetValue().Anchor), "anchors do not match")
210-214: Optionally set Subject display nameIf the provided name is intended for human display, pass it as DisplayName on subject creation for parity with the customer name.
- _, err := subjectService.Create(t.Context(), subject.CreateInput{ - Namespace: ns, - Key: key, - }) + _, err := subjectService.Create(t.Context(), subject.CreateInput{ + Namespace: ns, + Key: key, + DisplayName: lo.ToPtr(name), + })test/billing/suite.go (1)
221-225: Reuse a single tracer providerAvoid constructing two noop tracer providers; create once and reuse.
+ tracer := noop.NewTracerProvider().Tracer("test_env") ... - Tracer: noop.NewTracerProvider().Tracer("test_env"), + Tracer: tracer, ... - Tracer: noop.NewTracerProvider().Tracer("test_env"), + Tracer: tracer,Also applies to: 239-241
test/entitlement/regression/framework_test.go (2)
236-239: Use test context instead of context.Background()Using t.Context() helps tie operations to the test lifecycle and avoids leaks/timeouts.
- _, err := subjectService.Create(context.Background(), subject.CreateInput{ + _, err := subjectService.Create(t.Context(), subject.CreateInput{ Namespace: ns, Key: key, }) ... - cust, err := customerService.CreateCustomer(context.Background(), customer.CreateCustomerInput{ + cust, err := customerService.CreateCustomer(t.Context(), customer.CreateCustomerInput{ Namespace: ns, CustomerMutate: customer.CustomerMutate{ Key: lo.ToPtr(key), UsageAttribution: customer.CustomerUsageAttribution{ SubjectKeys: []string{key}, }, Name: name, }, })Also applies to: 242-252
236-239: Optionally set Subject display nameConsider passing DisplayName to reflect the provided name for parity with customer.
- _, err := subjectService.Create(t.Context(), subject.CreateInput{ - Namespace: ns, - Key: key, - }) + _, err := subjectService.Create(t.Context(), subject.CreateInput{ + Namespace: ns, + Key: key, + DisplayName: lo.ToPtr(name), + })openmeter/subscription/testutils/service.go (2)
74-76: Avoid shadowing the imported package name "lockr" with a local var.Shadowing hampers readability and can confuse future edits.
Apply this rename and update references:
- lockr, err := lockr.NewLocker(&lockr.LockerConfig{Logger: logger}) + locker, err := lockr.NewLocker(&lockr.LockerConfig{Logger: logger}) require.NoError(t, err) ... - Locker: lockr, + Locker: locker, ... - Lockr: lockr, + Lockr: locker, ... - Lockr: lockr, + Lockr: locker,Also applies to: 109-110, 188-189, 240-241
150-153: Use the test logger consistently instead of slog.Default().Mixing loggers can fragment output and complicate debugging.
- Customer: customerService, - Logger: slog.Default(), + Customer: customerService, + Logger: logger,openmeter/entitlement/service/utils_test.go (1)
171-189: Helper signature and context usage LGTM; set Subject.DisplayName for clarity.Optional: also set DisplayName so test fixtures are easier to read in logs/UI.
- _, err := subjectService.Create(t.Context(), subject.CreateInput{ - Namespace: ns, - Key: key, - }) + _, err := subjectService.Create(t.Context(), subject.CreateInput{ + Namespace: ns, + Key: key, + DisplayName: lo.ToPtr(name), + })openmeter/entitlement/metered/reset_test.go (6)
54-56: Use clock.Now() instead of time.Now() for deterministic tests.You already time-travel via clock; mixing in time.Now() can cause flakes.
- currentUsagePeriod, err := input.UsagePeriod.GetValue().GetPeriodAt(time.Now()) + currentUsagePeriod, err := input.UsagePeriod.GetValue().GetPeriodAt(clock.Now())
63-67: Fix typo in comment.“featute” → “feature”.
- // create featute in db + // create feature in db
666-676: Comment contradicts value for ResetMaxRollover.Value is 0 (no rollover) but comment says “full amount can be rolled over”.
- ResetMaxRollover: 0, // full amount can be rolled over + ResetMaxRollover: 0, // no rollover
764-766: Spelling in test name.“reseting” → “resetting”.
- name: "Should reseting without anchor update keeps the next reset time intact", + name: "Should resetting without anchor update keeps the next reset time intact",
806-808: Spelling in test name.“reseting” → “resetting”.
- name: "Should reseting with anchor update updates the next reset time too", + name: "Should resetting with anchor update updates the next reset time too",
299-321: Prefer require.NoError for setup assertions.In setup sections, failing fast improves signal and avoids misleading follow-on failures.
Example change (apply similarly to other grant creations and critical setup steps):
- g1, err := deps.grantRepo.CreateGrant(ctx, grant.RepoCreateInput{ ... }) - assert.NoError(t, err) + g1, err := deps.grantRepo.CreateGrant(ctx, grant.RepoCreateInput{ ... }) + require.NoError(t, err)Also applies to: 379-392, 559-571, 611-612, 700-705, 711-712, 748-749, 758-761, 583-595, 624-636, 488-494
openmeter/subscription/testutils/billing.go (4)
12-12: Add a short doc comment to flag test-only semantics.Clarify that this is intentionally a no-op stub for tests to avoid accidental production use.
type NoopCustomerOverrideService struct{} +// NoopCustomerOverrideService is a test stub that implements billing.CustomerOverrideService +// and intentionally returns zero values with no side effects. +// Meant for integration/unit tests only.
30-32: List returns a zero-value page; consider returning an explicit empty slice.Nil slices work with len/range, but JSON marshaling and equality checks can differ from empty slices. If callers or tests assert on JSON payloads, consider ensuring Items is [] not nil (only if ListCustomerOverridesResult exposes such a field).
10-11: Optional: also assert value-type conformance.Not required here, but it prevents regressions if receivers ever switch to value methods only.
var _ billing.CustomerOverrideService = (*NoopCustomerOverrideService)(nil) +var _ billing.CustomerOverrideService = (NoopCustomerOverrideService{})
22-24: Document no-op semantics in test utils
In openmeter/subscription/testutils/billing.go, clarify that NoopCustomerOverrideService.GetCustomerOverride deliberately returns a zero-value and nil error to signal “no override” in tests.func (n NoopCustomerOverrideService) GetCustomerOverride(ctx context.Context, input billing.GetCustomerOverrideInput) (billing.CustomerOverrideWithDetails, error) { - return billing.CustomerOverrideWithDetails{}, nil + // No-op for tests: returns zero-value and nil error to represent “absent override.” + return billing.CustomerOverrideWithDetails{}, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
app/common/customer.go(1 hunks)cmd/server/wire_gen.go(1 hunks)go.mod(1 hunks)openmeter/app/stripe/adapter/customer.go(1 hunks)openmeter/billing/adapter/invoicelinediff.go(0 hunks)openmeter/billing/worker/subscription/sync_test.go(1 hunks)openmeter/credit/helper.go(1 hunks)openmeter/entitlement/adapter/entitlement_test.go(1 hunks)openmeter/entitlement/metered/balance_test.go(49 hunks)openmeter/entitlement/metered/grant_owner_adapter_test.go(2 hunks)openmeter/entitlement/metered/lateevents_test.go(1 hunks)openmeter/entitlement/metered/reset_test.go(46 hunks)openmeter/entitlement/metered/utils_test.go(1 hunks)openmeter/entitlement/service/access_test.go(10 hunks)openmeter/entitlement/service/scheduling_test.go(26 hunks)openmeter/entitlement/service/utils_test.go(4 hunks)openmeter/subscription/testutils/billing.go(1 hunks)openmeter/subscription/testutils/service.go(7 hunks)openmeter/subscription/workflow/service/addon_test.go(44 hunks)openmeter/subscription/workflow/service/subscription_test.go(10 hunks)openmeter/testutils/namegen.go(1 hunks)openmeter/testutils/namegen_test.go(1 hunks)pkg/timeutil/recurrence.go(1 hunks)test/app/stripe/app_test.go(1 hunks)test/billing/adapter_test.go(2 hunks)test/billing/suite.go(5 hunks)test/customer/customer.go(2 hunks)test/entitlement/regression/framework_test.go(1 hunks)test/entitlement/regression/scenario_test.go(7 hunks)test/subscription/scenario_editaligned_test.go(0 hunks)test/subscription/scenario_editcancel_test.go(1 hunks)test/subscription/scenario_entinnextphase_test.go(0 hunks)test/subscription/scenario_firstofmonth_test.go(1 hunks)tools/migrate/billing_test.go(0 hunks)tools/migrate/entitlement_test.go(0 hunks)tools/migrate/productcatalog_test.go(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/db/db.go(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/db/helpers.go(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/db/models.go(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/db/queries.sql.go(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/fixture.sql(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/sqlc.yaml(0 hunks)tools/migrate/testdata/sqlcgen/20250605102416/sqlc/queries.sql(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/db/db.go(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/db/models.go(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/db/queries.sql.go(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/fixture.sql(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/sqlc.yaml(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/sqlc/db-schema.sql(0 hunks)tools/migrate/testdata/sqlcgen/20250609172811/sqlc/queries.sql(0 hunks)tools/migrate/testdata/sqlcgen/20250624115812/db/db.go(0 hunks)tools/migrate/testdata/sqlcgen/20250624115812/db/models.go(0 hunks)tools/migrate/testdata/sqlcgen/20250624115812/db/queries.sql.go(0 hunks)tools/migrate/testdata/sqlcgen/20250624115812/sqlc.yaml(0 hunks)tools/migrate/testdata/sqlcgen/20250624115812/sqlc/queries.sql(0 hunks)tools/migrate/testdata/sqlcgen/20250703081943/db/db.go(0 hunks)tools/migrate/testdata/sqlcgen/20250703081943/db/models.go(0 hunks)tools/migrate/testdata/sqlcgen/20250703081943/db/queries.sql.go(0 hunks)tools/migrate/testdata/sqlcgen/20250703081943/sqlc.yaml(0 hunks)tools/migrate/testdata/sqlcgen/20250703081943/sqlc/queries.sql(0 hunks)
💤 Files with no reviewable changes (30)
- test/subscription/scenario_editaligned_test.go
- tools/migrate/testdata/sqlcgen/20250609172811/sqlc.yaml
- openmeter/billing/adapter/invoicelinediff.go
- tools/migrate/testdata/sqlcgen/20250605102416/db/helpers.go
- tools/migrate/productcatalog_test.go
- tools/migrate/testdata/sqlcgen/20250703081943/sqlc.yaml
- test/subscription/scenario_entinnextphase_test.go
- tools/migrate/testdata/sqlcgen/20250703081943/db/db.go
- tools/migrate/testdata/sqlcgen/20250624115812/sqlc.yaml
- tools/migrate/testdata/sqlcgen/20250609172811/db/models.go
- tools/migrate/testdata/sqlcgen/20250609172811/sqlc/queries.sql
- tools/migrate/testdata/sqlcgen/20250624115812/sqlc/queries.sql
- tools/migrate/testdata/sqlcgen/20250605102416/fixture.sql
- tools/migrate/testdata/sqlcgen/20250703081943/sqlc/queries.sql
- tools/migrate/testdata/sqlcgen/20250605102416/sqlc.yaml
- tools/migrate/testdata/sqlcgen/20250609172811/fixture.sql
- tools/migrate/testdata/sqlcgen/20250605102416/sqlc/queries.sql
- tools/migrate/testdata/sqlcgen/20250605102416/db/models.go
- tools/migrate/testdata/sqlcgen/20250605102416/db/db.go
- tools/migrate/billing_test.go
- tools/migrate/entitlement_test.go
- tools/migrate/testdata/sqlcgen/20250624115812/db/queries.sql.go
- tools/migrate/testdata/sqlcgen/20250605102416/db/queries.sql.go
- tools/migrate/testdata/sqlcgen/20250624115812/db/db.go
- tools/migrate/testdata/sqlcgen/20250609172811/db/db.go
- tools/migrate/testdata/sqlcgen/20250703081943/db/queries.sql.go
- tools/migrate/testdata/sqlcgen/20250624115812/db/models.go
- tools/migrate/testdata/sqlcgen/20250703081943/db/models.go
- tools/migrate/testdata/sqlcgen/20250609172811/db/queries.sql.go
- tools/migrate/testdata/sqlcgen/20250609172811/sqlc/db-schema.sql
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/billing/worker/subscription/sync_test.go
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
PR: openmeterio/openmeter#2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
openmeter/entitlement/metered/utils_test.goopenmeter/entitlement/metered/lateevents_test.goopenmeter/entitlement/metered/balance_test.goopenmeter/entitlement/service/scheduling_test.goopenmeter/entitlement/metered/reset_test.goopenmeter/entitlement/service/utils_test.goopenmeter/testutils/namegen.goopenmeter/entitlement/service/access_test.go
🧬 Code graph analysis (21)
openmeter/credit/helper.go (1)
openmeter/credit/balance/repository.go (1)
NoSavedBalanceForOwnerError(19-22)
openmeter/testutils/namegen_test.go (1)
openmeter/testutils/namegen.go (1)
NameGenerator(11-11)
openmeter/entitlement/adapter/entitlement_test.go (1)
openmeter/ent/db/entitlement/where.go (1)
IsSoftLimit(140-142)
openmeter/entitlement/metered/grant_owner_adapter_test.go (1)
openmeter/testutils/namegen.go (1)
NameGenerator(11-11)
openmeter/subscription/workflow/service/subscription_test.go (4)
openmeter/ent/db/subscription.go (2)
Subscription(22-60)Subscription(140-157)openmeter/subscription/workflow/service.go (1)
CreateSubscriptionWorkflowInput(23-29)openmeter/subscription/testutils/db.go (1)
DBDeps(15-19)openmeter/ent/db/ent.go (1)
QueryContext(69-69)
openmeter/entitlement/metered/utils_test.go (2)
openmeter/subject/service.go (1)
CreateInput(43-49)openmeter/customer/customer.go (2)
CreateCustomerInput(213-216)CustomerMutate(72-82)
openmeter/entitlement/metered/lateevents_test.go (1)
openmeter/testutils/namegen.go (1)
NameGenerator(11-11)
test/billing/adapter_test.go (1)
openmeter/billing/invoiceline.go (1)
LineChildren(716-716)
openmeter/subscription/testutils/billing.go (2)
openmeter/billing/service.go (1)
CustomerOverrideService(35-42)openmeter/billing/customeroverride.go (7)
UpsertCustomerOverrideInput(132-141)CustomerOverrideWithDetails(241-248)DeleteCustomerOverrideInput(199-201)GetCustomerOverrideInput(167-170)GetCustomerAppInput(177-180)ListCustomerOverridesInput(256-275)ListCustomerOverridesResult(334-334)
openmeter/app/stripe/adapter/customer.go (2)
openmeter/ent/db/appcustomer/appcustomer.go (3)
FieldNamespace(18-18)FieldAppID(26-26)FieldCustomerID(28-28)openmeter/ent/db/ent.go (1)
IsConstraintError(367-373)
cmd/server/wire_gen.go (1)
app/common/customer.go (1)
NewCustomerSubjectServiceHook(61-86)
openmeter/entitlement/metered/balance_test.go (5)
pkg/convert/ptr.go (1)
ToPointer(10-12)openmeter/testutils/time.go (1)
GetRFC3339Time(8-15)openmeter/testutils/namegen.go (1)
NameGenerator(11-11)openmeter/ent/db/subject/where.go (3)
Key(74-76)Namespace(69-71)ID(14-16)openmeter/ent/db/balancesnapshot/where.go (4)
Namespace(59-61)ID(14-16)Balance(84-86)OwnerID(79-81)
openmeter/entitlement/service/scheduling_test.go (7)
openmeter/meter/service.go (1)
CreateMeterInput(107-117)openmeter/productcatalog/feature/connector.go (1)
CreateFeatureInputs(17-24)openmeter/entitlement/connector.go (1)
Connector(52-77)openmeter/entitlement/entitlement.go (2)
CreateEntitlementInputs(78-103)EntitlementType(305-305)pkg/clock/clock.go (1)
SetTime(23-27)openmeter/testutils/time.go (1)
GetRFC3339Time(8-15)openmeter/testutils/namegen.go (1)
NameGenerator(11-11)
test/billing/suite.go (5)
openmeter/customer/service.go (1)
Service(10-15)openmeter/customer/service/service.go (2)
Service(13-19)New(42-53)openmeter/subject/service/hooks/customersubject.go (2)
NewCustomerSubjectHook(81-96)CustomerSubjectHookConfig(98-98)openmeter/customer/service/hooks/subjectcustomer.go (1)
SubjectCustomerHookConfig(115-123)openmeter/customer/service/hooks/subjectvalidator.go (2)
NewSubjectValidatorHook(56-65)SubjectValidatorHookConfig(67-70)
test/entitlement/regression/framework_test.go (2)
openmeter/subject/service.go (1)
CreateInput(43-49)openmeter/customer/customer.go (2)
CreateCustomerInput(213-216)CustomerMutate(72-82)
openmeter/subscription/workflow/service/addon_test.go (2)
openmeter/subscription/testutils/service.go (1)
SubscriptionDependencies(46-64)openmeter/subscription/testutils/helpers.go (2)
CreatePlanWithAddon(24-63)CreateSubscriptionFromPlan(65-81)
openmeter/subscription/testutils/service.go (5)
openmeter/subscription/testutils/customer.go (2)
NewCustomerService(58-72)NewSubjectService(42-56)openmeter/subject/service/hooks/customersubject.go (2)
NewCustomerSubjectHook(81-96)CustomerSubjectHookConfig(98-98)openmeter/customer/service/hooks/subjectcustomer.go (1)
SubjectCustomerHookConfig(115-123)openmeter/subscription/testutils/billing.go (1)
NoopCustomerOverrideService(12-12)openmeter/customer/service/hooks/subjectvalidator.go (2)
NewSubjectValidatorHook(56-65)SubjectValidatorHookConfig(67-70)
openmeter/entitlement/metered/reset_test.go (5)
pkg/convert/ptr.go (1)
ToPointer(10-12)openmeter/testutils/time.go (1)
GetRFC3339Time(8-15)openmeter/testutils/namegen.go (1)
NameGenerator(11-11)pkg/models/id.go (1)
NamespacedID(7-10)openmeter/entitlement/metered/connector.go (1)
ResetEntitlementUsageParams(27-31)
openmeter/entitlement/service/utils_test.go (2)
openmeter/subject/service.go (1)
CreateInput(43-49)openmeter/customer/customer.go (2)
CreateCustomerInput(213-216)CustomerMutate(72-82)
openmeter/testutils/namegen.go (1)
pkg/clock/clock.go (1)
Now(14-21)
openmeter/entitlement/service/access_test.go (7)
pkg/framework/transaction/transaction.go (1)
Run(32-50)openmeter/testutils/time.go (1)
GetRFC3339Time(8-15)pkg/clock/clock.go (2)
SetTime(23-27)Now(14-21)openmeter/meter/service.go (1)
CreateMeterInput(107-117)openmeter/testutils/namegen.go (1)
NameGenerator(11-11)openmeter/productcatalog/feature/connector.go (1)
CreateFeatureInputs(17-24)openmeter/entitlement/entitlement.go (1)
CreateEntitlementInputs(78-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (35)
openmeter/billing/worker/subscription/sync_test.go (1)
4503-4503: Correct month-end alignment in expectationChanging period end to 2025-03-31T00:00:00Z matches EOM alignment for the next in-advance monthly slice and fixes the off-by-days expectation. Looks good.
openmeter/entitlement/adapter/entitlement_test.go (1)
411-423: Verify ordering guarantees for pagination assertions.The test asserts a strict item order across pages. Ensure ListActiveEntitlementsWithExpiredUsagePeriod has a deterministic ORDER BY (e.g., CreatedAt ASC, then ID ASC) to prevent flaky tests.
openmeter/app/stripe/adapter/customer.go (1)
132-135: Good fix: conflict target narrowed to the composite owner keyConfirmed that migrations define a unique index on (
namespace,app_id,customer_id) and a separate unique index on (app_id,stripe_customer_id), so the ON CONFLICT on (namespace, app_id, customer_id) will correctly defer duplicates onstripe_customer_idto its unique constraint.openmeter/entitlement/service/scheduling_test.go (1)
35-58: Good one-time meter/feature setup to reuse across subtests.Pre-creating the meter and feature reduces duplication and flakiness. Looks solid.
go.mod (1)
24-24: Approve test-only dependency namegen v1.0.1 Version v1.0.1 is available on the Go proxy and is licensed under MIT, which complies with policy.test/app/stripe/app_test.go (1)
30-32: LGTM: explicit per-suite namespace improves isolation.Setting namespace in the suite struct is clear and localized. No action needed.
test/customer/customer.go (2)
21-21: LGTM: direct subject import is consistent.
768-771: Idempotent DeleteCustomer semantics confirmed. No tests assert NotFound on a second delete—every repeat deletion uses require.NoError or equivalent.openmeter/entitlement/metered/grant_owner_adapter_test.go (2)
39-43: Switching to randomized subject names reduces cross-test collisions. LGTM.
649-653: Same here—good move to generate names for isolation.test/billing/adapter_test.go (1)
288-289: Clear semantics for “empty means delete” are well covered.You validate both representations (empty LineChildren literal and NewLineChildren([])) and assert zero children after upsert. Good coverage of the API contract.
Also applies to: 316-317
test/entitlement/regression/scenario_test.go (3)
45-46: Adapting createCustomerAndSubject to accept key + name is consistent across call sites.Also applies to: 196-197, 300-301, 403-404, 441-442, 479-480, 590-591
1001-1002: Anchor expectation updated to retain BillingAnchor.Aligns with removal of normalization-on-change. Make sure any docs/comments referencing “advance anchor by 1 cycle” are updated elsewhere.
1269-1279: No action required: go.mod uses Go 1.24.1
go.mod specifiesgo 1.24.1, which satisfies the Go ≥ 1.21 requirement for testing.T.Context.cmd/server/wire_gen.go (1)
366-375: Wiring updated to new NewCustomerSubjectServiceHook signature.Matches app/common/customer.go (no CustomerOverride). Since this file is generated, ensure wire generation ran cleanly (no local edits).
openmeter/subscription/workflow/service/subscription_test.go (2)
1001-1001: Correcting expected BillingAnchor (no auto-advance) makes the test match current behavior.
1269-1269: Confirm Go toolchain version meets Context requirement
go.mod declaresgo 1.24.1, which is ≥ 1.21 and includestesting.T.Context()support. No changes needed.openmeter/subscription/workflow/service/addon_test.go (2)
34-44: runWithDeps helper keeps tests hermetic (time freeze + DB setup/teardown). LGTM.
48-62: Refactor to pass SubscriptionDependencies directly simplifies scaffolding.Call sites updated accordingly (CreatePlanWithAddon/CreateSubscriptionFromPlan). Nice cleanup.
Also applies to: 83-98, 214-228, 269-283, 321-336, 369-384, 496-511, 551-576, 665-725
app/common/customer.go (1)
72-79: Removal of CustomerOverride wiring is clean; constructor now only depends on Customer service.Hook registration path remains intact. LGTM.
openmeter/entitlement/metered/utils_test.go (1)
207-229: Helper refactor LGTMExplicit key/name propagation and using t.Context() improve clarity and test control.
test/billing/suite.go (3)
20-20: Imports update LGTMAdding noop tracer and subject service imports is appropriate for the new hooks.
Also applies to: 42-46
73-73: Struct extension LGTMExposing SubjectService on BaseSuite is consistent with the new wiring.
145-152: Subject service wiring LGTMAdapter + service construction with error checks looks correct.
test/entitlement/regression/framework_test.go (1)
234-256: Helper refactor LGTMSignature and key/name propagation are consistent with the subject/customer schemas.
openmeter/subscription/testutils/service.go (1)
122-161: Hook wiring looks correct and complete.Subject/customer hooks and validators are registered against the intended services with consistent tracer usage. Nice.
openmeter/entitlement/service/utils_test.go (3)
75-76: New dependency field is appropriate.Adding meter.ManageService to deps aligns with the refactor toward managed meters.
122-123: Correct injection point for MeterService.Passing MeterService (not adapter) into the registry is the right direction.
91-94: No background routines in mock adapter: NewManage(nil) in utils_test.go invokes the mockadapter implementation (openmeter/meter/mockadapter/adapter.go), which merely wraps the provided slice and does not spawn any goroutines or rely on global state.openmeter/entitlement/metered/reset_test.go (2)
967-971: Good: reuse harness and reset streaming connector per sub-test.This reduces setup time while containing cross-test interference.
60-66: No action needed:setupConnectoris defined inopenmeter/entitlement/metered/utils_test.go.openmeter/subscription/testutils/billing.go (4)
10-10: Interface conformance check is solid.The compile-time assertion guarantees pointer receivers satisfy billing.CustomerOverrideService.
18-20: Delete is a true no-op; LGTM.
14-16: No-op CustomerOverrideService stub returns zero-value and none of its callers in subscription tests dereference its return, so no changes required.
26-28: Ignore nil‐check suggestion. NoopCustomerOverrideService is only used in subscription tests (which already assert non-nil), and the real billingService implementation never returns a nil app with no error.Likely an incorrect or invalid review comment.
| currentUsagePeriod, err := input.UsagePeriod.GetValue().GetPeriodAt(time.Now()) | ||
| assert.NoError(t, err) | ||
| require.NoError(t, err) | ||
| input.CurrentUsagePeriod = ¤tUsagePeriod |
There was a problem hiding this comment.
Use clock.Now() instead of time.Now() for deterministic tests
These helpers rely on the frozen test clock; using time.Now() can cause flakiness when clock.SetTime is active.
- currentUsagePeriod, err := input.UsagePeriod.GetValue().GetPeriodAt(time.Now())
+ currentUsagePeriod, err := input.UsagePeriod.GetValue().GetPeriodAt(clock.Now())Apply in both getEntitlement closures in this file.
Also applies to: 614-616
🤖 Prompt for AI Agents
In openmeter/entitlement/metered/balance_test.go around lines 61-63 (and also at
lines 614-616), the test uses time.Now() when computing UsagePeriod, which is
non-deterministic and conflicts with the frozen test clock; replace time.Now()
with clock.Now() (the package test clock used by the helpers) in both
getEntitlement closures so the calls read
input.UsagePeriod.GetValue().GetPeriodAt(clock.Now()), ensuring deterministic
tests when clock.SetTime is used.
| t.Run("Should return empty access if no entitlements are found", func(t *testing.T) { | ||
| access, err := conn.GetAccess(t.Context(), "test", "test") | ||
| require.NoError(t, err) | ||
| require.Equal(t, access, entitlement.Access{}) | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Prevent cross-test contamination in streaming mock
All subtests share a single deps instance and reuse the meter slug "meter1". The mock streaming connector doesn’t take namespace, so events can bleed between subtests. Reset it at the start of each subtest or use unique meter keys per subtest.
t.Run("Should return empty access if no entitlements are found", func(t *testing.T) {
+ deps.streamingConnector.Reset()
access, err := conn.GetAccess(t.Context(), "test", "test") t.Run("Should return access for a single entitlement", func(t *testing.T) {
+ deps.streamingConnector.Reset()
namespace := "ns1" t.Run("Should return access for multiple entitlements of multiple types", func(t *testing.T) {
+ deps.streamingConnector.Reset()
namespace := "ns3" t.Run("Should return access for multiple entitlements (> than max concurrency)", func(t *testing.T) {
+ deps.streamingConnector.Reset()
namespace := "ns4"Also applies to: 29-37, 158-166, 272-280
Reset streamingConnector in each subtest
Call deps.streamingConnector.Reset() at the start of every t.Run block (e.g. lines 23–27, 29–37, 158–166, 272–280) to isolate mock state between tests.
🤖 Prompt for AI Agents
In openmeter/entitlement/service/access_test.go around lines 23 to 27 (and
similarly at 29–37, 158–166, 272–280), the test subtests share mock state
causing inter-test interference; add a call to deps.streamingConnector.Reset()
at the very start of each t.Run block to reset the mock state before running the
subtest so each test is isolated and deterministic.
| // Hooks | ||
|
|
||
| // Subject hooks | ||
|
|
||
| subjectCustomerHook, err := subjecthooks.NewCustomerSubjectHook(subjecthooks.CustomerSubjectHookConfig{ | ||
| Subject: subjectService, | ||
| Logger: slog.Default(), | ||
| Tracer: noop.NewTracerProvider().Tracer("test_env"), | ||
| }) | ||
| require.NoError(t, err) | ||
| customerService.RegisterHooks(subjectCustomerHook) | ||
|
|
||
| subjectEntitlementValidatorHook, err := subjecthooks.NewEntitlementValidatorHook(subjecthooks.EntitlementValidatorHookConfig{ | ||
| EntitlementService: entitlementRegistry.Entitlement, | ||
| }) | ||
| require.NoError(t, err) | ||
| subjectService.RegisterHooks(subjectEntitlementValidatorHook) | ||
|
|
||
| // customer hooks | ||
| customerSubjectHook, err := customerservicehooks.NewSubjectCustomerHook(customerservicehooks.SubjectCustomerHookConfig{ | ||
| Customer: customerService, | ||
| CustomerOverride: billingService, | ||
| Logger: slog.Default(), | ||
| Tracer: noop.NewTracerProvider().Tracer("test_env"), | ||
| }) | ||
| require.NoError(t, err) | ||
| subjectService.RegisterHooks(customerSubjectHook) | ||
|
|
||
| customerSubjectValidatorHook, err := customerservicehooks.NewSubjectValidatorHook(customerservicehooks.SubjectValidatorHookConfig{ | ||
| Customer: customerService, | ||
| Logger: slog.Default(), | ||
| }) | ||
| require.NoError(t, err) | ||
| subjectService.RegisterHooks(customerSubjectValidatorHook) | ||
|
|
||
| entitlementValidatorHook, err := customerservicehooks.NewEntitlementValidatorHook(customerservicehooks.EntitlementValidatorHookConfig{ | ||
| EntitlementService: entitlementRegistry.Entitlement, | ||
| }) | ||
| require.NoError(t, err) | ||
| customerService.RegisterHooks(entitlementValidatorHook) | ||
| } |
There was a problem hiding this comment.
Hooks registered to the wrong services (compile/runtime mismatch)
Subject hooks must be registered on SubjectService; customer hooks on CustomerService. Current registrations are swapped and will not type-check (or will be no-ops if untyped).
Apply this swap:
- customerService.RegisterHooks(subjectCustomerHook)
+ s.SubjectService.RegisterHooks(subjectCustomerHook)
- s.SubjectService.RegisterHooks(customerSubjectHook)
+ s.CustomerService.RegisterHooks(customerSubjectHook)
- s.SubjectService.RegisterHooks(customerSubjectValidatorHook)
+ s.CustomerService.RegisterHooks(customerSubjectValidatorHook)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/billing/suite.go around lines 217-257, several hook registrations are
wired to the wrong services; swap the RegisterHooks targets so subject-related
hooks are registered on subjectService and customer-related hooks on
customerService. Concretely: change the subjectCustomerHook registration to
subjectService.RegisterHooks(...), change customerSubjectHook and
customerSubjectValidatorHook registrations to
customerService.RegisterHooks(...), leaving the entitlement-related
registrations as they are.
2 participants
Overview
Fix integration tests.
Summary by CodeRabbit
New Features
Bug Fixes
Tests