Skip to content

Harden test suite reliability for Anvil and WebSocket tests#17

Merged
dominicletz merged 2 commits into
mainfrom
fix/test-suite-reliability
Jul 14, 2026
Merged

Harden test suite reliability for Anvil and WebSocket tests#17
dominicletz merged 2 commits into
mainfrom
fix/test-suite-reliability

Conversation

@dominicletz

Copy link
Copy Markdown
Member

Summary

  • Retry RemoteChain.RPC.rpc/3 when NodeProxy returns {:error, :disconnect} instead of crashing with a CaseClauseError
  • Bootstrap Anvil in tests via HTTP anvil_mine and restore selective ChainList.refresh_chains/1 cache updates
  • Make PeerReadyEts tolerant of a missing ETS table during test service restarts
  • Stabilize WebSocket E2E and billing tests with PubSub.publish, device isolation, and cleaner test resets

Test plan

  • mix test — 187 passed, 6 skipped
  • mix lint — passes (Credo + Dialyzer)

…iably.

Retry RPC on NodeProxy disconnect, bootstrap Anvil via HTTP mining, restore selective chain-list cache refresh, and make PeerReadyEts safe when the ETS table is torn down during test service restarts.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several robustness improvements, including retry logic for RPC disconnects, dynamic initialization and checks for the PeerReady ETS table, and refactored chain list caching. It also updates test helpers and integration tests to ensure reliable execution. The review feedback highlights a potential race condition in the concurrent creation of the ETS table, suggests a more defensive fallback clause in the RPC retry logic to avoid CaseClauseError crashes, and recommends using the idiomatic is_nil/1 check in Elixir.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +17 to +25
def ensure do
case :ets.info(@table) do
:undefined ->
:ets.new(@table, [:named_table, :set, :public, read_concurrency: true])

_ ->
:ok
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In a concurrent environment, multiple processes calling ensure/0 simultaneously (e.g., during concurrent connection setups) can cause a race condition where both see :undefined and attempt to create the named ETS table @table. This will result in an ArgumentError crash. Rescuing the ArgumentError ensures that concurrent calls to ensure/0 handle this race condition gracefully.

  def ensure do
    case :ets.info(@table) do
      :undefined ->
        try do
          :ets.new(@table, [:named_table, :set, :public, read_concurrency: true])
          :ok
        rescue
          ArgumentError -> :ok
        end

      _ ->
        :ok
    end
  end

Comment thread lib/remote_chain/rpc.ex Outdated
Comment on lines +71 to +76
{:error, :disconnect} when retries_left > 0 ->
Process.sleep(200)
rpc_with_retry(chain, method, params, retries_left - 1)

{:error, :disconnect} ->
{:error, :disconnect}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a generic {:error, reason} fallback clause instead of a specific {:error, :disconnect} clause when retries_left <= 0 makes the code more robust and defensive. If RemoteChain.NodeProxy.rpc/3 ever returns other error tuples in the future, this prevents a CaseClauseError crash and propagates the error gracefully.

      {:error, :disconnect} when retries_left > 0 ->
        Process.sleep(200)
        rpc_with_retry(chain, method, params, retries_left - 1)

      {:error, reason} ->
        {:error, reason}

Comment thread lib/remote_chain/chain_list.ex Outdated
Enum.each(chains_by_id, fn {id, chain} ->
key = cache_key(id)

if not only_cached? or Globals.get(key) != nil do

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In Elixir, it is more idiomatic to use the is_nil/1 guard or not is_nil/1 instead of comparing directly with nil using != or == operators.

      if not only_cached? or not is_nil(Globals.get(key)) do

Rescue concurrent PeerReadyEts table creation, propagate any NodeProxy error tuple from RPC retry, and use is_nil/1 in chain list cache updates.
@dominicletz
dominicletz merged commit 0938070 into main Jul 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant