disable more event bus subscribers in test#5009
Merged
IvanIvanoff merged 6 commits intomasterfrom Feb 19, 2026
Merged
Conversation
Add a NoopSubscriber that subscribers to all topics as otherwise when KafkaExporterSubscriber is disabled, a warning that events without subscribers is written for many tests.
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces boolean subscriber flags with list-based Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant EventBus
participant Subscriber
participant TestRunner
Config->>EventBus: load `extra_subscribers` and `disabled_subscribers`
EventBus->>EventBus: build subscriber list (base ++ extra) and compute startup subscribers
EventBus->>EventBus: register topics for all subscribers
EventBus->>Subscriber: subscribe at startup (for allowed subscribers)
Note right of EventBus: runtime subscribe_subscriber/ unsubscribe_subscriber helpers
TestRunner->>EventBus: subscribe_subscriber(Subscriber)
EventBus->>Subscriber: EventBus.subscribe({Subscriber, Subscriber.topics()})
TestRunner->>EventBus: unsubscribe_subscriber(Subscriber)
EventBus->>Subscriber: EventBus.unsubscribe(Subscriber)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
lib/sanbase/event_bus/noop_subscriber.ex (2)
1-7: Consider relocating totest/support/given its test-only nature.The
@moduledocexplicitly states this module is only for tests, but placing it underlib/means it gets compiled into the production application. Modules intended exclusively for tests typically belong intest/support/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/event_bus/noop_subscriber.ex` around lines 1 - 7, Sanbase.EventBus.NoopSubscriber is test-only but lives under lib/, so move its source out of production code into the test tree (e.g., test/support/event_bus/noop_subscriber.ex) and remove it from lib/, then update any tests/setup that reference it (or ensure test/support is compiled by ExUnit) so the module remains available in tests but is not compiled into the production release.
10-23: Add@docand@specto public functions.
topics/0,start_link/1, andprocess/1are all public but lack documentation and typespecs.📝 Proposed addition
+ `@doc` "Subscribes to all EventBus topics." + `@spec` topics() :: [String.t()] def topics(), do: [".*"] + `@doc` "Starts the NoopSubscriber GenServer." + `@spec` start_link(keyword()) :: GenServer.on_start() def start_link(opts \\ []) do GenServer.start_link(__MODULE__, opts, name: Keyword.get(opts, :name, __MODULE__)) end def init(opts) do {:ok, opts} end + `@doc` "Marks the event as completed without any side effects." + `@spec` process({atom(), any()}) :: :ok def process({_topic, _id} = event_shadow) do spawn(fn -> EventBus.mark_as_completed({__MODULE__, event_shadow}) end) :ok endAs per coding guidelines: "Add typespecs (
@spec) to all public functions" and "Add@docdocumentation to all public functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/event_bus/noop_subscriber.ex` around lines 10 - 23, Add `@doc` and `@spec` annotations for the public functions topics/0, start_link/1, and process/1: document that topics/0 returns a list of topic patterns (e.g., [String.t()]) and add `@spec` topics() :: [String.t()]; document start_link/1 (accepts opts :: keyword() with optional :name) and add `@spec` start_link(keyword()) :: GenServer.on_start(); document process/1 (accepts an event tuple like {topic :: String.t(), id :: any()}) and add `@spec` process({String.t(), any()}) :: :ok, plus brief `@doc` text for each describing purpose/usage and that process/1 spawns EventBus.mark_as_completed({__MODULE__, event_shadow}).lib/sanbase/event_bus/event_bus.ex (1)
79-85: Add@docand@specto new public functions.Both
subscribe_subscriber/1andunsubscribe_subscriber/1are public API but lack documentation and typespecs, which are required by the project guidelines.📝 Proposed addition
+ `@doc` """ + Subscribes a subscriber module to its declared topics. + The subscriber module must implement a `topics/0` function. + """ + `@spec` subscribe_subscriber(module()) :: :ok def subscribe_subscriber(subscriber) do EventBus.subscribe({subscriber, subscriber.topics()}) end + `@doc` """ + Unsubscribes a subscriber module from all topics. + """ + `@spec` unsubscribe_subscriber(module()) :: :ok def unsubscribe_subscriber(subscriber) do EventBus.unsubscribe(subscriber) endAs per coding guidelines: "Add typespecs (
@spec) to all public functions" and "Add@docdocumentation to all public functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/event_bus/event_bus.ex` around lines 79 - 85, Add `@doc` and `@spec` for the two new public functions: document that subscribe_subscriber/1 calls EventBus.subscribe with the subscriber and its topics and that unsubscribe_subscriber/1 calls EventBus.unsubscribe for the subscriber; add `@specs` declaring the subscriber parameter as module() (or a more specific behaviour type if available) and a return type (e.g., term() or :ok | {:error, term()} depending on EventBus API) so both subscribe_subscriber/1 and unsubscribe_subscriber/1 have proper docs and typespecs referencing EventBus.subscribe/1 and EventBus.unsubscribe/1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanbase/metric_registry/metric_registry_test.exs`:
- Around line 2-13: The test module uses setup_all which on exit calls
Sanbase.EventBus.drain_topics/2 (which must only be invoked from non-async
tests), so update the module declaration to run synchronously by adding async:
false to the test case invocation (i.e., change the use Sanbase.DataCase line to
include async: false); keep the existing setup_all and on_exit logic
(Sanbase.EventBus.subscribe_subscriber and Sanbase.EventBus.drain_topics/2)
intact.
---
Nitpick comments:
In `@lib/sanbase/event_bus/event_bus.ex`:
- Around line 79-85: Add `@doc` and `@spec` for the two new public functions:
document that subscribe_subscriber/1 calls EventBus.subscribe with the
subscriber and its topics and that unsubscribe_subscriber/1 calls
EventBus.unsubscribe for the subscriber; add `@specs` declaring the subscriber
parameter as module() (or a more specific behaviour type if available) and a
return type (e.g., term() or :ok | {:error, term()} depending on EventBus API)
so both subscribe_subscriber/1 and unsubscribe_subscriber/1 have proper docs and
typespecs referencing EventBus.subscribe/1 and EventBus.unsubscribe/1.
In `@lib/sanbase/event_bus/noop_subscriber.ex`:
- Around line 1-7: Sanbase.EventBus.NoopSubscriber is test-only but lives under
lib/, so move its source out of production code into the test tree (e.g.,
test/support/event_bus/noop_subscriber.ex) and remove it from lib/, then update
any tests/setup that reference it (or ensure test/support is compiled by ExUnit)
so the module remains available in tests but is not compiled into the production
release.
- Around line 10-23: Add `@doc` and `@spec` annotations for the public functions
topics/0, start_link/1, and process/1: document that topics/0 returns a list of
topic patterns (e.g., [String.t()]) and add `@spec` topics() :: [String.t()];
document start_link/1 (accepts opts :: keyword() with optional :name) and add
`@spec` start_link(keyword()) :: GenServer.on_start(); document process/1 (accepts
an event tuple like {topic :: String.t(), id :: any()}) and add `@spec`
process({String.t(), any()}) :: :ok, plus brief `@doc` text for each describing
purpose/usage and that process/1 spawns EventBus.mark_as_completed({__MODULE__,
event_shadow}).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/sanbase_web/graphql/billing/subscribe_api_test.exs (1)
2-2:⚠️ Potential issue | 🟡 MinorSame
asyncconcern asemails_test.exs— add explicitasync: false.Line 2 uses
use SanbaseWeb.ConnCasewithoutasync: false. The test body useswith_mock/with_mocks(global module patching) and theon_exitcallsdrain_topics, both of which requireasync: false. Add the annotation to make the guarantee explicit and consistent with the two other test files in this PR that already set it:- use SanbaseWeb.ConnCase + use SanbaseWeb.ConnCase, async: falseAlso applies to: 19-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/graphql/billing/subscribe_api_test.exs` at line 2, The test module uses global mocks (with_mock/with_mocks) and on_exit cleanup (drain_topics) so update the test declaration to disable async by passing async: false to the ConnCase usage; specifically change the use SanbaseWeb.ConnCase invocation to include async: false so the test module runs synchronously and avoids race conditions with global module patching and on_exit cleanup.test/sanbase/email/emails_test.exs (1)
2-2:⚠️ Potential issue | 🟠 MajorAdd
async: falseto ConnCase — required fordrain_topicsandsetup_with_mocksThe test module calls
Sanbase.EventBus.drain_topics/2in itssetup_allon_exit callback and usessetup_with_mocksfor global mocking. Both requireasync: falseto be safe:
drain_topicscarries an inline comment: "Invoke this only from async: false tests!" and operates on shared ETS table state.setup_with_mockspatches modules globally, which is incompatible with concurrent test execution.Without
async: false, concurrent test modules can race when cleaning up subscribers, causing one module'sunsubscribe_subscriberto remove the subscriber while other modules' tests are still in flight.Suggested fix:
- use SanbaseWeb.ConnCase + use SanbaseWeb.ConnCase, async: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase/email/emails_test.exs` at line 2, The test module must run synchronously to safely call Sanbase.EventBus.drain_topics/2 in its setup_all on_exit and to use setup_with_mocks; update the module's ConnCase invocation from use SanbaseWeb.ConnCase to use SanbaseWeb.ConnCase, async: false so the tests are not run concurrently and shared ETS/global mocks are safe (this affects the test file that calls drain_topics/2 and setup_with_mocks).
🧹 Nitpick comments (1)
test/sanbase_web/api_call_limit/api_call_limit_api_test.exs (1)
19-23: Optional: extract repeatedsetup_allboilerplate into a shared helper.The same 8-line
setup_all+on_exitblock appears verbatim in all four test files changed by this PR (api_call_limit_api_test.exs,emails_test.exs,subscribe_api_test.exs,stripe_webhook_test.exs). Extracting it would make future changes (e.g., adjusting the drain timeout) a single-site update:# In a shared test support module, e.g. test/support/event_bus_helpers.ex +defmodule Sanbase.Test.EventBusHelpers do + def subscribe_billing_subscriber do + subscriber = Sanbase.EventBus.BillingEventSubscriber + Sanbase.EventBus.subscribe_subscriber(subscriber) + + ExUnit.Callbacks.on_exit(fn -> + Sanbase.EventBus.drain_topics(subscriber.topics(), 10_000) + Sanbase.EventBus.unsubscribe_subscriber(subscriber) + end) + end +endThen each test module reduces to a single
setup_allcall:- setup_all do - subscriber = Sanbase.EventBus.BillingEventSubscriber - Sanbase.EventBus.subscribe_subscriber(subscriber) - on_exit(fn -> - Sanbase.EventBus.drain_topics(subscriber.topics(), 10_000) - Sanbase.EventBus.unsubscribe_subscriber(subscriber) - end) - end + setup_all do: Sanbase.Test.EventBusHelpers.subscribe_billing_subscriber()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/api_call_limit/api_call_limit_api_test.exs` around lines 19 - 23, The repeated setup_all + on_exit block (which sets and deletes Application env via Application.delete_env(SanbaseWeb.Graphql.AbsintheBeforeSend, :api_call_exporting_enabled) and manages the Sanbase.EventBus subscriber with Sanbase.EventBus.drain_topics/2 and Sanbase.EventBus.unsubscribe_subscriber/1) should be extracted into a shared test helper function (e.g., TestHelpers.setup_eventbus_and_env or ApiTestSupport.setup_eventbus_and_env) that returns the same setup_all callback; replace each module's inline setup_all with a single call to setup_all(TestHelpers.setup_eventbus_and_env()) so future changes (like timeout) are made in one place and tests use the helper instead of duplicating the 8-line block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/sanbase_web/graphql/billing/subscribe_api_test.exs`:
- Line 2: The test module uses global mocks (with_mock/with_mocks) and on_exit
cleanup (drain_topics) so update the test declaration to disable async by
passing async: false to the ConnCase usage; specifically change the use
SanbaseWeb.ConnCase invocation to include async: false so the test module runs
synchronously and avoids race conditions with global module patching and on_exit
cleanup.
In `@test/sanbase/email/emails_test.exs`:
- Line 2: The test module must run synchronously to safely call
Sanbase.EventBus.drain_topics/2 in its setup_all on_exit and to use
setup_with_mocks; update the module's ConnCase invocation from use
SanbaseWeb.ConnCase to use SanbaseWeb.ConnCase, async: false so the tests are
not run concurrently and shared ETS/global mocks are safe (this affects the test
file that calls drain_topics/2 and setup_with_mocks).
---
Nitpick comments:
In `@test/sanbase_web/api_call_limit/api_call_limit_api_test.exs`:
- Around line 19-23: The repeated setup_all + on_exit block (which sets and
deletes Application env via
Application.delete_env(SanbaseWeb.Graphql.AbsintheBeforeSend,
:api_call_exporting_enabled) and manages the Sanbase.EventBus subscriber with
Sanbase.EventBus.drain_topics/2 and Sanbase.EventBus.unsubscribe_subscriber/1)
should be extracted into a shared test helper function (e.g.,
TestHelpers.setup_eventbus_and_env or ApiTestSupport.setup_eventbus_and_env)
that returns the same setup_all callback; replace each module's inline setup_all
with a single call to setup_all(TestHelpers.setup_eventbus_and_env()) so future
changes (like timeout) are made in one place and tests use the helper instead of
duplicating the 8-line block.
IvanIvanoff
force-pushed
the
disable-more-event-bus-subscribers-in-test
branch
from
9a9c4ab to
a9043a9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Ticket
Checklist:
Summary by CodeRabbit
New Features
Refactor