Skip to content

Commit

Permalink
✅ Fix flaky tests (#113)
Browse files Browse the repository at this point in the history
For a while now, I've been seeing spurious test failures, but hadn't been able to figure out the root cause.

It turns out that clearing the entire `InMemoryCache` before tests was causing test interference. This is obvious in retrospect.

There's also a spurious failure in CI with the integration tests where a second instance tries to start with the same SDK key as another. I'm not sure if I've fixed that one with these changes.

Changes:
- Clear only a specific cache key from the InMemoryCache, and only when multiple tests will use the same key.
- Mark the integration tests as `async: false` to be sure they run in isolation.
- Eliminate log noise from the tests.
  • Loading branch information
randycoulman authored Sep 22, 2023
1 parent a0dfa81 commit c79a7cd
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 16 deletions.
10 changes: 5 additions & 5 deletions lib/config_cat/in_memory_cache.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ defmodule ConfigCat.InMemoryCache do
GenServer.call(__MODULE__, {:set, cache_key, value})
end

@spec clear :: :ok
def clear do
GenServer.call(__MODULE__, :clear)
@spec clear(ConfigCache.key()) :: :ok
def clear(cache_key) do
GenServer.call(__MODULE__, {:clear, cache_key})
end

@impl GenServer
Expand All @@ -33,8 +33,8 @@ defmodule ConfigCat.InMemoryCache do
end

@impl GenServer
def handle_call(:clear, _from, _state) do
{:reply, :ok, %{}}
def handle_call({:clear, cache_key}, _from, state) do
{:reply, :ok, Map.delete(state, cache_key)}
end

@impl GenServer
Expand Down
16 changes: 8 additions & 8 deletions test/config_cat/in_memory_cache_test.exs
Original file line number Diff line number Diff line change
@@ -1,34 +1,34 @@
defmodule ConfigCat.InMemoryCacheTest do
use ExUnit.Case, async: true

alias ConfigCat.InMemoryCache, as: Cache
alias ConfigCat.InMemoryCache

@cache_key "CACHE_KEY"

setup do
Cache.clear()
InMemoryCache.clear(@cache_key)

:ok
end

test "cache is initially empty" do
assert Cache.get(@cache_key) == {:error, :not_found}
assert InMemoryCache.get(@cache_key) == {:error, :not_found}
end

test "returns cached value" do
entry = "serialized-cache-entry"

:ok = Cache.set(@cache_key, entry)
:ok = InMemoryCache.set(@cache_key, entry)

assert {:ok, ^entry} = Cache.get(@cache_key)
assert {:ok, ^entry} = InMemoryCache.get(@cache_key)
end

test "cache is empty after clearing" do
entry = "serialized-cache-entry"

:ok = Cache.set(@cache_key, entry)
:ok = InMemoryCache.set(@cache_key, entry)

assert :ok = Cache.clear()
assert Cache.get(@cache_key) == {:error, :not_found}
assert :ok = InMemoryCache.clear(@cache_key)
assert InMemoryCache.get(@cache_key) == {:error, :not_found}
end
end
3 changes: 3 additions & 0 deletions test/config_cat_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ defmodule ConfigCatTest do
} = ConfigCat.get_value_details("testStringKey", "", user, client: client)
end

@tag capture_log: true
test "get_all_value_details/2 returns evaluation details for all keys", %{client: client} do
all_details = ConfigCat.get_all_value_details(client: client)
details_by_key = fn key -> Enum.find(all_details, &(&1.key == key)) end
Expand All @@ -133,6 +134,8 @@ defmodule ConfigCatTest do
end

describe "when the configuration has not been fetched" do
@describetag capture_log: true

setup do
{:ok, client} = start_client()

Expand Down
11 changes: 9 additions & 2 deletions test/integration_test.exs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
defmodule ConfigCat.IntegrationTest do
use ExUnit.Case, async: true
# Must be async: false to avoid a collision with other tests.
# Now that we only allow a single ConfigCat instance to use the same SDK key,
# one of the async tests would fail due to the existing running instance.
use ExUnit.Case, async: false

alias ConfigCat.Cache
alias ConfigCat.CachePolicy
alias ConfigCat.InMemoryCache

Expand All @@ -16,6 +20,7 @@ defmodule ConfigCat.IntegrationTest do
|> assert_sdk_key_required()
end

@tag capture_log: true
test "raises error when starting another instance with the same SDK key" do
{:ok, _} = start_config_cat(@sdk_key, name: :original)

Expand Down Expand Up @@ -108,7 +113,9 @@ defmodule ConfigCat.IntegrationTest do
end

defp start_config_cat(sdk_key, options \\ []) do
InMemoryCache.clear()
sdk_key
|> Cache.generate_key()
|> InMemoryCache.clear()

name = UUID.uuid4() |> String.to_atom()
default_options = [name: name, sdk_key: sdk_key]
Expand Down
1 change: 0 additions & 1 deletion test/support/cache_policy_case.ex
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ defmodule ConfigCat.CachePolicyCase do

defp start_cache(instance_id) do
cache_key = UUID.uuid4()
InMemoryCache.clear()

{:ok, _pid} =
start_supervised(
Expand Down

0 comments on commit c79a7cd

Please sign in to comment.