chore: centralize test mocking and add named helpers - #63
Conversation
PR SummaryLow Risk Overview Introduces Cleans up unused mocks and simplifies several aggregate/telemetry/opentelemetry tests to use the new helper functions and recorded-event builder. Written by Cursor Bugbot for commit ea13be5. This will update automatically on new commits. Configure here. |
WalkthroughRefactors test suites to replace inline Mox expectations with centralized test helpers and case templates. Adds Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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)
test/support/mock_event_store_helpers.ex (3)
170-177: Consider documenting the default count.The default
count: 6presumably matches the aggregate's retry limit configuration. A brief note in the@docexplaining this coupling would help maintainers understand why this specific default was chosen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/mock_event_store_helpers.ex` around lines 170 - 177, Update the `@doc` for expect_too_many_retry_attempts to explicitly state that the default count is 6 and that this value is chosen to match the aggregate's retry limit configuration (so callers know the coupling); mention that the count can be overridden via the opts parameter (e.g., opts[:count]) and that the function calls expect_append_wrong_expected_version and expect_stream_empty with that count so both mocks use the same retry attempt number.
199-200: Fallback event_type may be misleading.For non-struct data, returning
"Elixir.Commanded.EventStore.RecordedEvent"as the event type is semantically confusing—it suggests the event is aRecordedEventrather than indicating an unknown/untyped event. Consider a clearer fallback:♻️ Suggested alternatives
defp infer_event_type(%{__struct__: struct}), do: to_string(struct) - defp infer_event_type(_), do: "Elixir.Commanded.EventStore.RecordedEvent" + defp infer_event_type(_), do: "UnknownEvent"Or require an explicit
:event_typein opts for non-struct data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/mock_event_store_helpers.ex` around lines 199 - 200, The fallback in infer_event_type currently returns the misleading "Elixir.Commanded.EventStore.RecordedEvent" for non-structs; change infer_event_type to accept an optional opts (e.g., infer_event_type(data, opts \\ [])) and return opts[:event_type] when provided, otherwise use a clear default like "Elixir.UnknownEvent" (or "unknown") instead of "Elixir.Commanded.EventStore.RecordedEvent"; update callers to pass opts where non-struct event_type must be explicit.
71-75: Consider addingcountoption for API consistency.Unlike
expect_stream_empty/2and other helpers, this function lacks acountoption. For consistency and flexibility:♻️ Suggested refactor
- def expect_stream_not_found(stream_uuid) do - expect(MockEventStore, :stream_forward, fn _meta, ^stream_uuid, _from, _batch_size -> + def expect_stream_not_found(stream_uuid, opts \\ []) do + count = Keyword.get(opts, :count, 1) + + expect(MockEventStore, :stream_forward, count, fn _meta, ^stream_uuid, _from, _batch_size -> {:error, :stream_not_found} end) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/mock_event_store_helpers.ex` around lines 71 - 75, The helper expect_stream_not_found/1 is missing a count option for consistency; change it to accept an optional count argument (e.g., def expect_stream_not_found(stream_uuid, count \\ 0)) and use that count in the expect for MockEventStore.stream_forward by matching the _batch_size argument to the provided count (pin the count variable in the anonymous function head), and update any callers to pass a count where needed so behavior matches expect_stream_empty/2 and other helpers.
🤖 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/support/mock_projection_case.ex`:
- Around line 35-37: The stub for MockEventStore.subscribe_to returns {:ok,
self()} but never sends the subscription acknowledgement message to the handler;
update the stub for subscribe_to (MockEventStore.subscribe_to) to generate a
reference (e.g., ref = make_ref()) and send {:subscribed, ref} to the handler
process argument before returning {:ok, self()} so projection handlers receive
the subscription lifecycle message they expect.
---
Nitpick comments:
In `@test/support/mock_event_store_helpers.ex`:
- Around line 170-177: Update the `@doc` for expect_too_many_retry_attempts to
explicitly state that the default count is 6 and that this value is chosen to
match the aggregate's retry limit configuration (so callers know the coupling);
mention that the count can be overridden via the opts parameter (e.g.,
opts[:count]) and that the function calls expect_append_wrong_expected_version
and expect_stream_empty with that count so both mocks use the same retry attempt
number.
- Around line 199-200: The fallback in infer_event_type currently returns the
misleading "Elixir.Commanded.EventStore.RecordedEvent" for non-structs; change
infer_event_type to accept an optional opts (e.g., infer_event_type(data, opts
\\ [])) and return opts[:event_type] when provided, otherwise use a clear
default like "Elixir.UnknownEvent" (or "unknown") instead of
"Elixir.Commanded.EventStore.RecordedEvent"; update callers to pass opts where
non-struct event_type must be explicit.
- Around line 71-75: The helper expect_stream_not_found/1 is missing a count
option for consistency; change it to accept an optional count argument (e.g.,
def expect_stream_not_found(stream_uuid, count \\ 0)) and use that count in the
expect for MockEventStore.stream_forward by matching the _batch_size argument to
the provided count (pin the count variable in the anonymous function head), and
update any callers to pass a count where needed so behavior matches
expect_stream_empty/2 and other helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a35dc7bb-7215-4cd2-afb5-081dbd8b5426
📒 Files selected for processing (10)
test/aggregates/aggregate_concurrency_test.exstest/aggregates/aggregate_telemetry_test.exstest/event_handler_after_start_test.exstest/opentelemetry/aggregate_test.exstest/projections/error_callback_test.exstest/projections/runtime_config_projector_test.exstest/support/mock_event_store_case.extest/support/mock_event_store_helpers.extest/support/mock_projection_case.extest/support/mocks.ex
💤 Files with no reviewable changes (1)
- test/support/mocks.ex
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
2ef05c9 to
b233029
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/support/mock_event_store_helpers.ex (1)
181-243: Consider parameterizing version numbers inexpect_concurrency_retry_succeeds/2.The helper hardcodes versions
1,2,2for the append/stream sequence. While this works for the current test cases, adding optional:expected_versionand:reload_versionparameters (similar toexpect_wrong_expected_version_retry_succeeds/3) would make the helper more flexible for future scenarios.The
:countersusage for stateful mock behavior inexpect_wrong_expected_version_retry_succeeds/3is a clever approach that keeps the expectation self-contained.♻️ Optional: Add version parameters for flexibility
- def expect_concurrency_retry_succeeds(stream_uuid, concurrent_event) do - expect(MockEventStore, :append_to_stream, fn _meta, ^stream_uuid, 1, _events, _opts -> + def expect_concurrency_retry_succeeds(stream_uuid, concurrent_event, opts \\ []) do + expected_version = Keyword.get(opts, :expected_version, 1) + reload_version = Keyword.get(opts, :reload_version, 2) + + expect(MockEventStore, :append_to_stream, fn _meta, ^stream_uuid, ^expected_version, _events, _opts -> {:error, :wrong_expected_version} end) - expect(MockEventStore, :stream_forward, fn _meta, ^stream_uuid, 2, _batch_size -> + expect(MockEventStore, :stream_forward, fn _meta, ^stream_uuid, ^reload_version, _batch_size -> [concurrent_event] end) - expect(MockEventStore, :append_to_stream, fn _meta, ^stream_uuid, 2, _events, _opts -> + expect(MockEventStore, :append_to_stream, fn _meta, ^stream_uuid, ^reload_version, _events, _opts -> :ok end) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/mock_event_store_helpers.ex` around lines 181 - 243, The helper expect_concurrency_retry_succeeds/2 hardcodes versions (1, 2, 2); make it accept optional version parameters (e.g., opts with :expected_version and :reload_version) defaulting to the current values and use those variables in the MockEventStore expect clauses instead of literals so the append_to_stream and stream_forward patterns match the supplied versions; update the function head and docstring to mention the new opts and default values so tests can pass custom expected/reload versions when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/support/mock_event_store_helpers.ex`:
- Around line 181-243: The helper expect_concurrency_retry_succeeds/2 hardcodes
versions (1, 2, 2); make it accept optional version parameters (e.g., opts with
:expected_version and :reload_version) defaulting to the current values and use
those variables in the MockEventStore expect clauses instead of literals so the
append_to_stream and stream_forward patterns match the supplied versions; update
the function head and docstring to mention the new opts and default values so
tests can pass custom expected/reload versions when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef28da4f-1cf6-47aa-bdbb-1cde66e8b9a7
📒 Files selected for processing (11)
test/aggregates/aggregate_concurrency_test.exstest/aggregates/aggregate_telemetry_test.exstest/event_handler_after_start_test.exstest/opentelemetry/aggregate_test.exstest/projections/error_callback_test.exstest/projections/runtime_config_projector_test.exstest/support/mock_event_store_case.extest/support/mock_event_store_helpers.extest/support/mock_projection_case.extest/support/mocks.extest/support/runtime_config_projector.ex
💤 Files with no reviewable changes (1)
- test/support/mocks.ex
🚧 Files skipped from review as they are similar to previous changes (4)
- test/aggregates/aggregate_concurrency_test.exs
- test/support/mock_projection_case.ex
- test/event_handler_after_start_test.exs
- test/support/mock_event_store_case.ex
1 participant
Summary
Reduces test noise by centralizing MockEventStore setup and replacing inline
expect/stubcalls with named helper functions that document intent.Changes
New infrastructure
stub_event_store/1from error_callback_test and runtime_config_projector_testexpect_wrong_expected_version_conflict/1expect_successful_append_with_empty_stream/1expect_open_aggregate/1expect_concurrency_retry_succeeds/2expect_too_many_retry_attempts/2expect_wrong_expected_version_retry_succeeds/3expect_stream_not_found/1,expect_stream_with_events/3stub_subscribe_to_return_handler/0build_recorded_event/4Refactored tests
Cleanup
MockEventStorealias from opentelemetry/aggregate_test.exs