Skip to content

Commit a7ef0d8

Browse files
committed
Fix Task.async_stream
1 parent 5da5180 commit a7ef0d8

File tree

6 files changed

+194
-7
lines changed

6 files changed

+194
-7
lines changed

config/test.exs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,4 +11,5 @@ config :logger, level: :warning
1111

1212
config :diff,
1313
package_store_impl: Diff.Package.StoreMock,
14-
storage_impl: Diff.StorageMock
14+
storage_impl: Diff.StorageMock,
15+
hex_impl: Diff.HexMock

lib/diff/hex/behaviour.ex

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
defmodule Diff.Hex.Behaviour do
2+
@callback diff(package :: String.t(), from :: String.t(), to :: String.t()) ::
3+
{:ok, Enumerable.t()} | :error
4+
end
5+

lib/diff/hex/hex.ex

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
defmodule Diff.Hex do
2+
@behaviour Diff.Hex.Behaviour
3+
24
@config %{
35
:hex_core.default_config()
46
| http_adapter: {Diff.Hex.Adapter, %{}},

lib/diff_web/live/diff_live_view.ex

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,9 @@ defmodule DiffWeb.DiffLiveView do
151151
end
152152

153153
def handle_info({:generate_diff, package, from, to}, socket) do
154-
case Diff.Hex.diff(package, from, to) do
154+
hex_impl = Application.get_env(:diff, :hex_impl, Diff.Hex)
155+
156+
case hex_impl.diff(package, from, to) do
155157
{:ok, stream} ->
156158
case process_stream_to_diffs(package, from, to, stream) do
157159
{:ok, metadata, diff_ids} ->
@@ -243,11 +245,12 @@ defmodule DiffWeb.DiffLiveView do
243245
}
244246

245247
# Process stream elements in parallel with indices
248+
indexed_stream = Stream.with_index(stream)
249+
246250
results =
247-
stream
248-
|> Stream.with_index()
249-
|> Task.async_stream(
251+
Task.Supervisor.async_stream(
250252
Diff.Tasks,
253+
indexed_stream,
251254
fn {element, index} ->
252255
process_stream_element(package, from, to, element, index)
253256
end,
@@ -406,9 +409,9 @@ defmodule DiffWeb.DiffLiveView do
406409
defp build_url(app, from, to), do: "/diff/#{app}/#{from}..#{to}"
407410

408411
defp load_diffs_in_parallel(package, from, to, diff_ids) do
409-
diff_ids
410-
|> Task.async_stream(
412+
Task.Supervisor.async_stream(
411413
Diff.Tasks,
414+
diff_ids,
412415
fn diff_id ->
413416
{diff_id, DiffWeb.LiveView.load_diff_content(package, from, to, diff_id)}
414417
end,

test/diff_web/live/diff_live_view_test.exs

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ defmodule DiffWeb.DiffLiveViewTest do
22
use DiffWeb.ConnCase
33
import Phoenix.LiveViewTest
44
import Mox
5+
import ExUnit.CaptureLog
56

67
setup :verify_on_exit!
78

@@ -65,6 +66,180 @@ defmodule DiffWeb.DiffLiveViewTest do
6566
end
6667
end
6768

69+
describe "DiffLiveView diff generation" do
70+
test "successfully generates and processes diffs in parallel", %{conn: conn} do
71+
# Setup mock stream data that simulates parallel processing
72+
mock_stream = [
73+
{:ok,
74+
{"diff --git a/lib/app.ex b/lib/app.ex\n--- a/lib/app.ex\n+++ b/lib/app.ex\n@@ -1,3 +1,4 @@\n+# New line\n defmodule App do",
75+
"/tmp/from", "/tmp/to"}},
76+
{:ok,
77+
{"diff --git a/lib/config.ex b/lib/config.ex\n--- a/lib/config.ex\n+++ b/lib/config.ex\n@@ -5,2 +5,3 @@\n- old_config: true\n+ new_config: false\n+ extra_config: :value",
78+
"/tmp/from", "/tmp/to"}},
79+
{:too_large, "very_large_file.txt"}
80+
]
81+
82+
# Mock Diff.Hex to return our test stream
83+
Diff.HexMock
84+
|> expect(:diff, fn "phoenix", "1.4.5", "1.4.9" ->
85+
{:ok, mock_stream}
86+
end)
87+
88+
# Mock storage operations for parallel processing
89+
Diff.StorageMock
90+
|> stub(:get_metadata, fn "phoenix", "1.4.5", "1.4.9" ->
91+
{:error, :not_found}
92+
end)
93+
|> expect(:put_diff, 3, fn "phoenix", "1.4.5", "1.4.9", diff_id, data ->
94+
# Verify diff data structure
95+
assert diff_id =~ ~r/diff-\d+/
96+
decoded = Jason.decode!(data)
97+
assert is_map(decoded)
98+
:ok
99+
end)
100+
|> expect(:put_metadata, fn "phoenix", "1.4.5", "1.4.9", metadata ->
101+
# Verify aggregated metadata from parallel processing
102+
assert metadata.total_diffs == 3
103+
assert metadata.files_changed == 3
104+
assert metadata.total_additions > 0
105+
assert metadata.total_deletions > 0
106+
:ok
107+
end)
108+
|> stub(:list_diffs, fn "phoenix", "1.4.5", "1.4.9" ->
109+
{:ok, ["diff-0", "diff-1", "diff-2"]}
110+
end)
111+
|> stub(:get_diff, fn "phoenix", "1.4.5", "1.4.9", _diff_id ->
112+
{:ok,
113+
Jason.encode!(%{
114+
"diff" => "test diff",
115+
"path_from" => "/tmp/from",
116+
"path_to" => "/tmp/to"
117+
})}
118+
end)
119+
120+
capture_log(fn ->
121+
{:ok, view, _html} = live(conn, "/diff/phoenix/1.4.5..1.4.9")
122+
123+
# Wait for generation and loading to complete
124+
:timer.sleep(200)
125+
final_html = render(view)
126+
127+
# Should show the metadata from parallel processing
128+
assert final_html =~ "3 files changed"
129+
# additions
130+
assert final_html =~ "+3"
131+
# deletions
132+
assert final_html =~ "-1"
133+
end)
134+
end
135+
136+
test "handles errors in parallel diff processing", %{conn: conn} do
137+
# Mock stream with some errors
138+
mock_stream = [
139+
{:ok, {"diff content", "/tmp/from", "/tmp/to"}},
140+
{:error, {:git_diff, "git command failed"}},
141+
{:too_large, "large_file.bin"}
142+
]
143+
144+
Diff.HexMock
145+
|> expect(:diff, fn "phoenix", "1.4.5", "1.4.9" ->
146+
{:ok, mock_stream}
147+
end)
148+
149+
# Mock storage - only successful elements should be stored
150+
Diff.StorageMock
151+
|> stub(:get_metadata, fn "phoenix", "1.4.5", "1.4.9" ->
152+
{:error, :not_found}
153+
end)
154+
|> expect(:put_diff, 2, fn "phoenix", "1.4.5", "1.4.9", _diff_id, _data ->
155+
:ok
156+
end)
157+
|> expect(:put_metadata, fn "phoenix", "1.4.5", "1.4.9", metadata ->
158+
# Only 2 diffs should be stored (error one skipped)
159+
assert metadata.total_diffs == 2
160+
assert metadata.files_changed == 2
161+
:ok
162+
end)
163+
|> stub(:list_diffs, fn "phoenix", "1.4.5", "1.4.9" ->
164+
# Skip error element
165+
{:ok, ["diff-0", "diff-2"]}
166+
end)
167+
|> stub(:get_diff, fn "phoenix", "1.4.5", "1.4.9", _diff_id ->
168+
{:ok,
169+
Jason.encode!(%{
170+
"diff" => "test diff",
171+
"path_from" => "/tmp/from",
172+
"path_to" => "/tmp/to"
173+
})}
174+
end)
175+
176+
capture_log(fn ->
177+
{:ok, view, _html} = live(conn, "/diff/phoenix/1.4.5..1.4.9")
178+
179+
:timer.sleep(200)
180+
final_html = render(view)
181+
182+
# Should still succeed with partial results
183+
# Only successful files
184+
assert final_html =~ "2 files changed"
185+
end)
186+
end
187+
188+
test "handles hex diff failure", %{conn: conn} do
189+
Diff.HexMock
190+
|> expect(:diff, fn "phoenix", "1.4.5", "1.4.9" ->
191+
:error
192+
end)
193+
194+
Diff.StorageMock
195+
|> stub(:get_metadata, fn "phoenix", "1.4.5", "1.4.9" ->
196+
{:error, :not_found}
197+
end)
198+
199+
{:ok, view, _html} = live(conn, "/diff/phoenix/1.4.5..1.4.9")
200+
201+
:timer.sleep(100)
202+
final_html = render(view)
203+
204+
assert final_html =~ "Failed to generate diff"
205+
end
206+
207+
test "handles storage failure during parallel processing", %{conn: conn} do
208+
mock_stream = [
209+
{:ok, {"diff content", "/tmp/from", "/tmp/to"}}
210+
]
211+
212+
Diff.HexMock
213+
|> expect(:diff, fn "phoenix", "1.4.5", "1.4.9" ->
214+
{:ok, mock_stream}
215+
end)
216+
217+
Diff.StorageMock
218+
|> stub(:get_metadata, fn "phoenix", "1.4.5", "1.4.9" ->
219+
{:error, :not_found}
220+
end)
221+
|> expect(:put_diff, fn "phoenix", "1.4.5", "1.4.9", _diff_id, _data ->
222+
{:error, :storage_failed}
223+
end)
224+
|> expect(:put_metadata, fn "phoenix", "1.4.5", "1.4.9", metadata ->
225+
# Should still try to store metadata even with failed individual diffs
226+
# No successful diffs
227+
assert metadata.total_diffs == 0
228+
:ok
229+
end)
230+
231+
capture_log(fn ->
232+
{:ok, view, _html} = live(conn, "/diff/phoenix/1.4.5..1.4.9")
233+
234+
:timer.sleep(100)
235+
final_html = render(view)
236+
237+
# Should handle storage failures gracefully
238+
refute final_html =~ "Failed to generate diff"
239+
end)
240+
end
241+
end
242+
68243
describe "DiffLiveView diffs list" do
69244
test "shows list of diffs", %{conn: conn} do
70245
{:ok, _view, html} =

test/test_helper.exs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
ExUnit.start()
22
Mox.defmock(Diff.StorageMock, for: Diff.Storage)
33
Mox.defmock(Diff.Package.StoreMock, for: Diff.Package.Store)
4+
Mox.defmock(Diff.HexMock, for: Diff.Hex.Behaviour)

0 commit comments

Comments
 (0)