-
Notifications
You must be signed in to change notification settings - Fork 7
fix: fixed panic during tests #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: fixed panic during tests #145
Conversation
BulkCircuit: circuitbreaker.Config{ | ||
RequestVolumeThreshold: 101, // disable circuit breaker | ||
Timeout: time.Hour, | ||
CustomSuffix: cfg.Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, you are right, i missed this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkharms already updated test configs, check please
📝 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Name
in-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.NewTestingEnv
across your tests; extract theConfig.Name = fmt.Sprintf("%s-%s", …)
plusNewTestingEnv
into 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.
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