Skip to content

Commit c80e7f7

Browse files
JusterZhuJusterZhu
committed
fix: address copilot review comments for PR #399
- Fix effectiveConcurrency to use _options.MaxConcurrency as primary value (method param maxConcurrency now acts as override for backward compat) - Fix OS-dependent /tmp path to use Path.GetTempPath() in test - Add ConcurrencyTracker to verify Serial mode peak concurrency <= 1 - Strengthen resume test assertion to exact file length (50 bytes) - Expose GetOption via TestableBootstrap subclass for value assertions - Replace weak Assert.NotNull with Assert.Equal value checks in Bootstrap tests
1 parent c971e40 commit c80e7f7

3 files changed

Lines changed: 99 additions & 31 deletions

File tree

‎src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,13 @@ public async Task<DownloadReport> ExecuteAsync(
4848

4949
Directory.CreateDirectory(destDir);
5050

51-
// Resolve effective concurrency: Serial mode forces 1
51+
// Resolve effective concurrency: Serial mode forces 1.
52+
// Uses _options.MaxConcurrency as primary value; the method parameter
53+
// maxConcurrency acts as an override (default 3).
54+
var baseConcurrency = maxConcurrency > 0 ? maxConcurrency : _options.MaxConcurrency;
5255
var effectiveConcurrency = _options.DiffMode == DiffMode.Serial
5356
? 1
54-
: DownloadOrchestratorOptions.SanitizeMaxConcurrency(Math.Max(1, maxConcurrency));
57+
: DownloadOrchestratorOptions.SanitizeMaxConcurrency(Math.Max(1, baseConcurrency));
5558

5659
GeneralTracer.Info($"DefaultDownloadOrchestrator.ExecuteAsync: concurrency={effectiveConcurrency}, " +
5760
$"resume={_options.EnableResume}, verifyChecksum={_options.VerifyChecksum}, diffMode={_options.DiffMode}");

‎tests/CoreTest/Configuration/GlobalConfigInfoWiringTests.cs‎

Lines changed: 49 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -86,75 +86,98 @@ public void GlobalConfigInfo_AllDownloadProperties_HaveReasonableDefaults()
8686

8787
#region Bootstrap Option → GetOption Roundtrip
8888

89+
/// <summary>Test subclass that exposes the protected GetOption method.</summary>
90+
private sealed class TestableBootstrap : GeneralUpdateBootstrap
91+
{
92+
public T PublicGetOption<T>(UpdateOption<T>? option) => GetOption(option);
93+
}
94+
8995
[Fact]
9096
public void Bootstrap_GetOption_MaxConcurrency_ReturnsSetValue()
9197
{
92-
var b = MakeBootstrap().Option(UpdateOptions.MaxConcurrency, 8);
93-
Assert.NotNull(b);
98+
var b = new TestableBootstrap();
99+
b.Option(UpdateOptions.MaxConcurrency, 8);
100+
Assert.Equal(8, b.PublicGetOption(UpdateOptions.MaxConcurrency));
94101
}
95102

96103
[Fact]
97104
public void Bootstrap_GetOption_EnableResume_ReturnsSetValue()
98105
{
99-
var b = MakeBootstrap().Option(UpdateOptions.EnableResume, false);
100-
Assert.NotNull(b);
106+
var b = new TestableBootstrap();
107+
b.Option(UpdateOptions.EnableResume, false);
108+
Assert.False(b.PublicGetOption(UpdateOptions.EnableResume));
101109
}
102110

103111
[Fact]
104112
public void Bootstrap_GetOption_RetryCount_ReturnsSetValue()
105113
{
106-
var b = MakeBootstrap().Option(UpdateOptions.RetryCount, 10);
107-
Assert.NotNull(b);
114+
var b = new TestableBootstrap();
115+
b.Option(UpdateOptions.RetryCount, 10);
116+
Assert.Equal(10, b.PublicGetOption(UpdateOptions.RetryCount));
108117
}
109118

110119
[Fact]
111120
public void Bootstrap_GetOption_RetryInterval_ReturnsSetValue()
112121
{
113-
var b = MakeBootstrap().Option(UpdateOptions.RetryInterval, TimeSpan.FromSeconds(5));
114-
Assert.NotNull(b);
122+
var b = new TestableBootstrap();
123+
b.Option(UpdateOptions.RetryInterval, TimeSpan.FromSeconds(5));
124+
Assert.Equal(TimeSpan.FromSeconds(5), b.PublicGetOption(UpdateOptions.RetryInterval));
115125
}
116126

117127
[Fact]
118128
public void Bootstrap_GetOption_VerifyChecksum_ReturnsSetValue()
119129
{
120-
var b = MakeBootstrap().Option(UpdateOptions.VerifyChecksum, false);
121-
Assert.NotNull(b);
130+
var b = new TestableBootstrap();
131+
b.Option(UpdateOptions.VerifyChecksum, false);
132+
Assert.False(b.PublicGetOption(UpdateOptions.VerifyChecksum));
122133
}
123134

124135
[Fact]
125136
public void Bootstrap_GetOption_BackupEnabled_ReturnsSetValue()
126137
{
127-
var b = MakeBootstrap().Option(UpdateOptions.BackupEnabled, false);
128-
Assert.NotNull(b);
138+
var b = new TestableBootstrap();
139+
b.Option(UpdateOptions.BackupEnabled, false);
140+
Assert.False(b.PublicGetOption(UpdateOptions.BackupEnabled));
129141
}
130142

131143
[Fact]
132144
public void Bootstrap_GetOption_PatchEnabled_ReturnsSetValue()
133145
{
134-
var b = MakeBootstrap().Option(UpdateOptions.PatchEnabled, true);
135-
Assert.NotNull(b);
146+
var b = new TestableBootstrap();
147+
b.Option(UpdateOptions.PatchEnabled, true);
148+
Assert.True(b.PublicGetOption(UpdateOptions.PatchEnabled));
136149
}
137150

138151
[Fact]
139152
public void Bootstrap_GetOption_DiffMode_ReturnsSetValue()
140153
{
141-
var b = MakeBootstrap().Option(UpdateOptions.DiffMode, DiffMode.Parallel);
142-
Assert.NotNull(b);
154+
var b = new TestableBootstrap();
155+
b.Option(UpdateOptions.DiffMode, DiffMode.Parallel);
156+
Assert.Equal(DiffMode.Parallel, b.PublicGetOption(UpdateOptions.DiffMode));
143157
}
144158

145159
[Fact]
146160
public void Bootstrap_AllEightOptions_SetWithoutError()
147161
{
148-
var b = MakeBootstrap()
149-
.Option(UpdateOptions.MaxConcurrency, 6)
150-
.Option(UpdateOptions.EnableResume, false)
151-
.Option(UpdateOptions.RetryCount, 5)
152-
.Option(UpdateOptions.RetryInterval, TimeSpan.FromSeconds(3))
153-
.Option(UpdateOptions.VerifyChecksum, false)
154-
.Option(UpdateOptions.BackupEnabled, false)
155-
.Option(UpdateOptions.PatchEnabled, true)
156-
.Option(UpdateOptions.DiffMode, DiffMode.Parallel);
157-
Assert.NotNull(b);
162+
var b = new TestableBootstrap();
163+
b.Option(UpdateOptions.MaxConcurrency, 6)
164+
.Option(UpdateOptions.EnableResume, false)
165+
.Option(UpdateOptions.RetryCount, 5)
166+
.Option(UpdateOptions.RetryInterval, TimeSpan.FromSeconds(3))
167+
.Option(UpdateOptions.VerifyChecksum, false)
168+
.Option(UpdateOptions.BackupEnabled, false)
169+
.Option(UpdateOptions.PatchEnabled, true)
170+
.Option(UpdateOptions.DiffMode, DiffMode.Parallel);
171+
172+
// Verify each option was stored correctly
173+
Assert.Equal(6, b.PublicGetOption(UpdateOptions.MaxConcurrency));
174+
Assert.False(b.PublicGetOption(UpdateOptions.EnableResume));
175+
Assert.Equal(5, b.PublicGetOption(UpdateOptions.RetryCount));
176+
Assert.Equal(TimeSpan.FromSeconds(3), b.PublicGetOption(UpdateOptions.RetryInterval));
177+
Assert.False(b.PublicGetOption(UpdateOptions.VerifyChecksum));
178+
Assert.False(b.PublicGetOption(UpdateOptions.BackupEnabled));
179+
Assert.True(b.PublicGetOption(UpdateOptions.PatchEnabled));
180+
Assert.Equal(DiffMode.Parallel, b.PublicGetOption(UpdateOptions.DiffMode));
158181
}
159182

160183
#endregion

‎tests/CoreTest/Download/OrchestratorOptionsBehaviourTests.cs‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ public void Constructor_ReadsMaxConcurrencyFromOptions()
3737
[Fact]
3838
public async Task ExecuteAsync_SerialMode_ForcesConcurrencyToOne()
3939
{
40-
using var httpClient = new HttpClient(new FakeSuccessHandler());
40+
var fakeHandler = new FakeSuccessHandler();
41+
var tracker = new ConcurrencyTracker(fakeHandler.PublicSendAsync);
42+
using var httpClient = new HttpClient(tracker);
4143
var opts = new DownloadOrchestratorOptions
4244
{
4345
DiffMode = DiffMode.Serial,
@@ -60,6 +62,9 @@ public async Task ExecuteAsync_SerialMode_ForcesConcurrencyToOne()
6062
var orch = new DefaultDownloadOrchestrator(httpClient, opts);
6163
var report = await orch.ExecuteAsync(plan, destDir, token: CancellationToken.None);
6264
Assert.Equal(2, report.SuccessCount);
65+
// Serial mode must never exceed 1 concurrent request
66+
Assert.True(tracker.PeakConcurrency <= 1,
67+
$"Expected peak concurrency <= 1 for Serial mode, got {tracker.PeakConcurrency}");
6368
}
6469
finally
6570
{
@@ -81,6 +86,42 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
8186
Content = new ByteArrayContent(new byte[100]),
8287
});
8388
}
89+
90+
/// <summary>Public wrapper for testing.</summary>
91+
public Task<HttpResponseMessage> PublicSendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
92+
=> SendAsync(request, cancellationToken);
93+
}
94+
95+
/// <summary>Wraps an inner handler and tracks peak concurrent requests.</summary>
96+
private sealed class ConcurrencyTracker : HttpMessageHandler
97+
{
98+
private readonly Func<HttpRequestMessage, CancellationToken, Task<HttpResponseMessage>> _sendAsync;
99+
private int _current;
100+
public int PeakConcurrency;
101+
102+
public ConcurrencyTracker(Func<HttpRequestMessage, CancellationToken, Task<HttpResponseMessage>> sendAsync)
103+
=> _sendAsync = sendAsync;
104+
105+
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
106+
{
107+
var current = Interlocked.Increment(ref _current);
108+
InterlockedAddMax(ref PeakConcurrency, current);
109+
try
110+
{
111+
return await _sendAsync(request, cancellationToken);
112+
}
113+
finally
114+
{
115+
Interlocked.Decrement(ref _current);
116+
}
117+
}
118+
119+
private static void InterlockedAddMax(ref int target, int value)
120+
{
121+
int snapshot;
122+
do { snapshot = Volatile.Read(ref target); }
123+
while (value > snapshot && Interlocked.CompareExchange(ref target, value, snapshot) != snapshot);
124+
}
84125
}
85126

86127
[Fact]
@@ -270,8 +311,9 @@ public async Task HttpDownloadExecutor_WithRangeResponse_AppendsCorrectly()
270311
var result = await executor.ExecuteAsync(
271312
"http://example.com/file.bin", destPath, token: CancellationToken.None);
272313
Assert.True(result.Success);
314+
// Partial content (30 bytes) appended to existing (20 bytes) = 50 total
273315
var fileInfo = new FileInfo(destPath);
274-
Assert.True(fileInfo.Length >= 20, $"Expected file >= 20 bytes, got {fileInfo.Length}");
316+
Assert.Equal(50, fileInfo.Length);
275317
}
276318
finally
277319
{
@@ -323,7 +365,7 @@ public async Task ExecuteAsync_EmptyPlan_ReturnsEmptyReport()
323365
{
324366
using var httpClient = new HttpClient();
325367
var orch = new DefaultDownloadOrchestrator(httpClient);
326-
var report = await orch.ExecuteAsync(DownloadPlan.Empty, "/tmp", token: CancellationToken.None);
368+
var report = await orch.ExecuteAsync(DownloadPlan.Empty, Path.GetTempPath(), token: CancellationToken.None);
327369
Assert.Equal(0, report.SuccessCount);
328370
Assert.Equal(0, report.FailedCount);
329371
}

0 commit comments

Comments
 (0)