fix: fixed panic during tests#145
Conversation
romanchechyotkin
changed the title
| BulkCircuit: circuitbreaker.Config{ | ||
| RequestVolumeThreshold: 101, // disable circuit breaker | ||
| Timeout: time.Hour, | ||
| CustomSuffix: cfg.Name, |
There was a problem hiding this comment.
Could you please verify that this will fix the issue?
I see that we have several suites (for example TestBasicIntegration with name Basic) with multiple tests inside those suites. Each test calls setup.NewTestingEnv with this CustomSuffix so we still can face panics.
Or am I missing something?
There was a problem hiding this comment.
yea, you are right, i missed this point
There was a problem hiding this comment.
@romanchechyotkin sorry, I was chilling on a vacation. Feel free to ping other maintainers.
Honestly, I think your approach is too verbose. I would be fine with something like this (where new CB is created):
breaker := circuitbreaker.New(
fmt.Sprintf("%s-shard-%d-%s", breakerPrefix, i, uuid.NewString()),
config,
)Of course, this is a workaround and the real problem is that CB is a global instance (and compiled only once for integration tests).
romanchechyotkin
changed the title
📝 WalkthroughWalkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (7)
network/circuitbreaker/circuitbreaker.go (1)
55-56: Clarify intent and scope of CustomSuffixNit: consider naming it NameSuffix and documenting it as primarily for tests to avoid metric cardinality surprises in prod if used widely.
tests/suites/single.go (1)
151-152: Avoid cumulative name growth across tests in the same suite instanceBeforeTest mutates
s.Config.Namein-place. In testify, the same suite instance runs all tests; names may accumulate like “Basic-TestA-TestB-…”.Base it on the original name captured at suite construction.
Apply:
type Single struct { Base Env *setup.TestingEnv + origName string } func NewSingle(cfg *setup.TestingEnvConfig) *Single { return &Single{ - Base: *NewBase(cfg), + Base: *NewBase(cfg), + origName: cfg.Name, } } func (s *Single) BeforeTest(suiteName, testName string) { s.Base.BeforeTest(suiteName, testName) - s.Config.Name = fmt.Sprintf("%s-%s", s.Config.Name, testName) + s.Config.Name = fmt.Sprintf("%s-%s", s.origName, testName) s.Env = setup.NewTestingEnv(s.Config) }tests/integration_tests/single_test.go (1)
545-549: Set per-subtest env name deterministicallyLooks good. Minor: you can move
cfg.Name = ...beforet.Parallel()for clarity, but functionally fine.proxy/bulk/ingestor_test.go (3)
87-90: Prefer t.Name() to hardcoded strings for suffixesThis avoids drift if the test name changes.
- BulkCircuit: circuitbreaker.Config{ - CustomSuffix: "TestProcessDocuments", - }, + BulkCircuit: circuitbreaker.Config{ + CustomSuffix: t.Name(), + },
493-496: Prefer b.Name() in benchmarks for consistencyKeeps suffixes aligned with the benchmark’s actual name.
- BulkCircuit: circuitbreaker.Config{ - CustomSuffix: "BenchProcessDocuments", - }, + BulkCircuit: circuitbreaker.Config{ + CustomSuffix: b.Name(), + },
573-579: Prefer t.Name() for this test tooReduce maintenance when renaming tests.
- BulkCircuit: circuitbreaker.Config{ - CustomSuffix: "TestProcessDocumentType", - }, + BulkCircuit: circuitbreaker.Config{ + CustomSuffix: t.Name(), + },tests/integration_tests/integration_test.go (1)
63-64: Encapsulate per-test env setupThere are 38 calls to
setup.NewTestingEnvacross your tests; extract theConfig.Name = fmt.Sprintf("%s-%s", …)plusNewTestingEnvinto a small helper (e.g. a suite method) to eliminate boilerplate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
network/circuitbreaker/circuitbreaker.go(2 hunks)proxy/bulk/ingestor_test.go(4 hunks)tests/integration_tests/integration_test.go(33 hunks)tests/integration_tests/replicas_test.go(1 hunks)tests/integration_tests/single_test.go(1 hunks)tests/setup/env.go(1 hunks)tests/suites/single.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
proxy/bulk/ingestor_test.go (3)
network/circuitbreaker/circuitbreaker.go (1)
Config(27-57)proxyapi/ingestor.go (1)
NewIngestor(110-168)proxy/bulk/ingestor.go (3)
NewIngestor(94-117)IngestorConfig(52-72)MappingProvider(47-50)
tests/integration_tests/single_test.go (1)
tests/suites/single.go (1)
SingleEnvs(160-170)
🔇 Additional comments (3)
tests/integration_tests/replicas_test.go (1)
40-41: Good: include t.Name() in env name to avoid collisionsThis aligns with the suffixing approach and prevents cross-test panics.
tests/setup/env.go (1)
351-354: All test circuitbreakers use unique CustomSuffix
tests/setup/env.go uses cfg.Name; proxy/bulk/ingestor_test.go uses distinct literal suffixes.network/circuitbreaker/circuitbreaker.go (1)
69-71: Ensure atomic circuit creation Appending a suffix still allows two goroutines to race between GetCircuit and MustCreateCircuit. Wrap creation in an atomic path: if cep21/circuit v3 exposes a non-panicking CreateCircuit(name, cfg) → (Circuit, error), use that with a fallback to GetCircuit on “AlreadyExists”; otherwise, serialize MustCreateCircuit calls via sync/singleflight or a per-name mutex. Verify the exact API and error types.
romanchechyotkin
force-pushed
the
37-circuit-breaker
branch
from
e4d7990 to
734f8fc
Compare
2 participants
Description
Added custom suffix for circuit breaker in order not to catch panic via existing name
Fixes #37
If you have used LLM/AI assistance please provide model name and full prompt:
Summary by CodeRabbit