Skip to content

fix: account for ETS matching maps when comparing shapes#2824

Merged
icehaunter merged 1 commit into
mainfrom
ilia/pr/uvytnzvmtqxo
Jun 11, 2025
Merged

fix: account for ETS matching maps when comparing shapes#2824
icehaunter merged 1 commit into
mainfrom
ilia/pr/uvytnzvmtqxo

Conversation

@icehaunter

Copy link
Copy Markdown
Contributor

No description provided.

@icehaunter
icehaunter requested review from magnetised and msfstef and removed request for msfstef June 11, 2025 11:00
@codecov

codecov Bot commented Jun 11, 2025

Copy link
Copy Markdown

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
2257 2 2255 967
View the top 2 failed test(s) by shortest run time
Elixir.Electric.ShapeCacheTest::test clean_all_shapes/1 calls unsafe_cleanup! on storage
Stack Traces | 0.411s run time
24) test clean_all_shapes/1 calls unsafe_cleanup! on storage (Electric.ShapeCacheTest)
     test/electric/shape_cache_test.exs:996
     Assertion failed, no matching message after 400ms
     The following variables were pinned:
       shape_handle = "20871201-1749642164206413"
     Showing 10 of 10 messages in the mailbox
     code: assert_receive {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
     mailbox:
       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :get_all_stored_shapes}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :for_shape, "20871201-1749642164206413"}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :initialise, "20871201-1749642164206413"}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :set_shape_definition, "20871201-1749642164206413", Shape.new!("public.items" [OID 1])}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :get_current_position, "20871201-1749642164206413"}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :snapshot_started?, "20871201-1749642164206413"}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :make_new_snapshot!, "20871201-1749642164206413", [["test"]]}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :set_pg_snapshot, "20871201-1749642164206413", %{xmin: 10, xmax: 11, xip_list: ~c"\n", filter_txns?: true}}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Support.TestStorage, :mark_snapshot_as_started, "20871201-1749642164206413"}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle}
       value:   {Electric.Shapes.Monitor, :remove, "20871201-1749642164206413"}
     stacktrace:
       test/electric/shape_cache_test.exs:1017: (test)
Elixir.Electric.ShapeCacheTest::test after restart invalidates shapes that we fail to restore
Stack Traces | 0.428s run time
20) test after restart invalidates shapes that we fail to restore (Electric.ShapeCacheTest)
     test/electric/shape_cache_test.exs:1122
     Assertion failed, no matching message after 400ms
     The following variables were pinned:
       shape_handle1 = "20871201-1749642673852528"
     Showing 2 of 2 messages in the mailbox
     code: assert_receive {Electric.Shapes.Monitor, :cleanup, ^shape_handle1}
     mailbox:
       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle1}
       value:   {Electric.Shapes.Monitor, :remove, "20871201-1749642673852528"}

       pattern: {Electric.Shapes.Monitor, :cleanup, ^shape_handle1}
       value:   {Electric.Shapes.Monitor, :remove, "20871201-1749642673852528"}
     stacktrace:
       test/electric/shape_cache_test.exs:1143: (test)
View the full list of 1 ❄️ flaky tests
test/integration.test.ts > HTTP Sync > should get initial data and then receive updates

Flake rate in main: 16.67% (Passed 20 times, Failed 4 times)

Stack Traces | 5.01s run time
Error: Test timed out in 5000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@msfstef msfstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, nice catch ! 🚀

Fixes #2820

%Expr{} = expr -> Electric.Shapes.Shape.Comparable.comparable(expr)
statement when is_binary(statement) -> statement
end)
{:shape, {shape.root_table_id, shape.root_table}, shape.root_pk,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: this is the only thing I was slightly worried about - we need to make sure this tuple is kept up to date with any more fields/info we end up adding to the shape. Maybe a docstring with a little bit of an explanation might help here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@icehaunter
icehaunter force-pushed the ilia/pr/uvytnzvmtqxo branch from 38b4dad to 7b8e560 Compare June 11, 2025 11:41
@icehaunter
icehaunter merged commit 68ba7ed into main Jun 11, 2025
24 of 29 checks passed
@icehaunter
icehaunter deleted the ilia/pr/uvytnzvmtqxo branch June 11, 2025 11:55
end)
{:shape, {shape.root_table_id, shape.root_table}, shape.root_pk,
Comparable.comparable(shape.where), shape.selected_columns,
Enum.flat_map(shape.flags, fn {k, v} -> if(v, do: [k], else: []) end) |> Enum.sort(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not in scope for this PR but it would be better to turn shape.flags into a keyword list that gets sorted and normalized whenever it is updated, instead of doing this mapping and sorting every time we do any lookups for the shape.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we don't open an issue for doing it, we'll never do it :)

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.

CaseClauseError: no case clause matching: ["96961314-1749566689686071", "51148140-1749566689764757"]

4 participants