Skip to content

Commit

Permalink
chore: Fix more Elixir test flakes (#2192)
Browse files Browse the repository at this point in the history
I ran tests individually and in groups with many repetitions until
failure to find flaky spots, and also went over the CI logs to see tests
that failed, and made some changes to fix that.

Main test changes:
- Unique per-test publication when using `:with_publication` using test
name based hash
- Use per test `:with_shared_db` for `TransactionCase` - otherwise
connections are not closed in time and we run into too many clients
issues
- Longer time to get `:ready` message when spinning up full stack - on
CI you can see that sometimes the logs indicate that we're _almost_
ready but it times out with the default

Actual code changes:
- Stop trapping exits in the `Shapes.Consumer`, we don't handle it in
`handle_info` and I think it was leftover from when I was doing
termination cleanups. I think it's ok to allow cleanups to occur on
restore since if it receives an exit there is not guarantee that the
storage process is alive to clean things up.
- Found this while testing `ShapeCache`, where we have tests restarting
it and seeing if they are restored correctly and sometimes errors are
logged with the consumer faliing to do a cleanup - it led me to try and
kill the consumers directly and all signals were getting transformed to
`:EXIT` messages, with no handler
- Minor dialyzer fix
  • Loading branch information
msfstef authored Dec 19, 2024
1 parent b5106ec commit 8987142
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 26 deletions.
5 changes: 5 additions & 0 deletions .changeset/breezy-boats-tease.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@core/sync-service": patch
---

Do not trap exits in `Electric.Shapes.Consumer` - not handled.
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ defmodule Electric.ShapeCache.FileStorage do
@impl Electric.ShapeCache.Storage
def unsafe_cleanup!(%FS{} = opts) do
{:ok, _} = File.rm_rf(opts.data_dir)
:ok
end

defp shape_definition_path(%{data_dir: data_dir} = _opts) do
Expand Down
2 changes: 0 additions & 2 deletions packages/sync-service/lib/electric/shapes/consumer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ defmodule Electric.Shapes.Consumer do
Logger.metadata(metadata)
Electric.Telemetry.Sentry.set_tags_context(metadata)

Process.flag(:trap_exit, true)

:ok = ShapeCache.Storage.initialise(storage)

# Store the shape definition to ensure we can restore it
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
defmodule Electric.Postgres.ConfigurationTest do
use ExUnit.Case, async: true
import ExUnit.CaptureLog
import Support.DbSetup

alias Electric.Replication.PublicationManager.RelationFilter
alias Electric.Replication.Eval
alias Electric.Postgres.Configuration

@pg_15 150_000

setup {Support.DbSetup, :with_unique_db}
setup {Support.DbSetup, :with_publication}
setup {Support.DbSetup, :with_pg_version}
setup :with_unique_db
setup :with_publication
setup :with_pg_version

setup %{db_conn: conn} do
Postgrex.query!(
Expand Down
17 changes: 8 additions & 9 deletions packages/sync-service/test/electric/shape_cache_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@ defmodule Electric.ShapeCacheTest do
%{name: "value", type: "text", type_id: {25, 1}}
])

defmodule TempPubManager do
def add_shape(_, opts) do
send(opts[:test_pid], {:called, :prepare_tables_fn})
end

def refresh_publication(_), do: :ok
end

setup :verify_on_exit!

setup do
Expand Down Expand Up @@ -123,14 +131,6 @@ defmodule Electric.ShapeCacheTest do
test "triggers table prep and snapshot creation only once", ctx do
test_pid = self()

defmodule TempPubManager do
def add_shape(_, opts) do
send(opts[:test_pid], {:called, :prepare_tables_fn})
end

def refresh_publication(_), do: :ok
end

%{shape_cache_opts: opts} =
with_shape_cache(Map.merge(ctx, %{pool: nil, inspector: @stub_inspector}),
run_with_conn_fn: &run_with_conn_noop/2,
Expand Down Expand Up @@ -880,7 +880,6 @@ defmodule Electric.ShapeCacheTest do
stop_shape_cache(context)
# Wait 1 millisecond to ensure shape handles are not generated the same
Process.sleep(1)
with_cub_db_storage(context)

with_shape_cache(Map.put(context, :inspector, @stub_inspector),
create_snapshot_fn: fn parent, shape_handle, _shape, _, storage, _, _ ->
Expand Down
6 changes: 3 additions & 3 deletions packages/sync-service/test/support/component_setup.ex
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,9 @@ defmodule Support.ComponentSetup do
restart: :temporary
)

assert_receive {:stack_status, ^ref, :ready}

# Process.sleep(100)
# allow a reasonable time for full stack setup to account for
# potential CI slowness, including PG
assert_receive {:stack_status, ^ref, :ready}, 1000

%{
stack_id: stack_id,
Expand Down
20 changes: 12 additions & 8 deletions packages/sync-service/test/support/db_setup.ex
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,7 @@ defmodule Support.DbSetup do

full_db_name = to_string(ctx.test)

db_name_hash =
full_db_name
|> :erlang.phash2(64 ** 5)
|> :binary.encode_unsigned()
|> Base.encode64()
|> String.replace_trailing("==", "")
db_name_hash = small_hash(full_db_name)

# Truncate the database name to 63 characters, use hash to guarantee uniqueness
db_name = "#{db_name_hash} ~ #{String.slice(full_db_name, 0..50)}"
Expand Down Expand Up @@ -55,8 +50,9 @@ defmodule Support.DbSetup do
end

def with_publication(ctx) do
Postgrex.query!(ctx.pool, "CREATE PUBLICATION electric_test_publication", [])
{:ok, %{publication_name: "electric_test_publication"}}
publication_name = "electric_test_publication_#{small_hash(ctx.test)}"
Postgrex.query!(ctx.pool, "CREATE PUBLICATION \"#{publication_name}\"", [])
{:ok, %{publication_name: publication_name}}
end

def with_pg_version(ctx) do
Expand Down Expand Up @@ -123,6 +119,14 @@ defmodule Support.DbSetup do
defp database_settings(%{database_settings: settings}), do: settings
defp database_settings(_), do: []

defp small_hash(value),
do:
to_string(value)
|> :erlang.phash2(64 ** 5)
|> :binary.encode_unsigned()
|> Base.encode64()
|> String.replace_trailing("==", "")

defp start_db_pool(connection_opts) do
start_opts = Electric.Utils.deobfuscate_password(connection_opts) ++ @postgrex_start_opts
Postgrex.start_link(start_opts)
Expand Down
2 changes: 1 addition & 1 deletion packages/sync-service/test/support/transaction_case.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ defmodule Support.TransactionCase do
use ExUnit.CaseTemplate
import Support.DbSetup

setup_all :with_shared_db
setup :with_shared_db
setup :in_transaction
end

0 comments on commit 8987142

Please sign in to comment.