feat: wire UpdateOptions configuration to runtime behavior#399
Merged
Conversation
Closes #398 - Add DownloadOrchestratorOptions config class to bundle download settings - Extend GlobalConfigInfo with MaxConcurrency, EnableResume, RetryCount, RetryInterval, VerifyChecksum, BackupEnabled, DiffMode properties - Wire GeneralUpdateBootstrap.ApplyRuntimeOptions to read all 8 options - Wire MaxConcurrency through DownloadOrchestratorOptions -> orchestrator - Wire EnableResume through HttpDownloadExecutor (skip Range header when false) - Wire RetryCount + RetryInterval through DefaultRetryPolicy - Wire VerifyChecksum to conditionally skip SHA256 verification - Wire BackupEnabled to conditionally skip backup in ClientUpdateStrategy - Wire DiffMode to control Serial/Parallel download mode in orchestrator - Wire PatchEnabled assignment from UpdateOptions to GlobalConfigInfo - Add 56 unit tests across 3 test files covering all wired options
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires previously-defined UpdateOptions values into the C# runtime update workflow so configuration (concurrency, resume, retry, checksum verification, backup, diff mode, etc.) actually affects download/update behavior. It introduces a consolidated DownloadOrchestratorOptions config object and adds unit tests to validate option defaults/mapping and behavior.
Changes:
- Add
DownloadOrchestratorOptionsand thread download-related configuration fromUpdateOptions→GlobalConfigInfo→ orchestrator/executor/policy. - Update
DefaultDownloadOrchestrator/HttpDownloadExecutorto respect retry/resume/checksum/diff-mode settings and plumb timeouts. - Add new tests covering defaults, mapping, and behavior toggles (checksum skipping, resume behavior, diff mode, retry).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CoreTest/Download/OrchestratorOptionsBehaviourTests.cs | New behavior-focused tests for orchestrator/executor option handling. |
| tests/CoreTest/Download/DownloadOrchestratorOptionsTests.cs | New unit tests for DownloadOrchestratorOptions defaults, clamping, and mapping. |
| tests/CoreTest/Configuration/GlobalConfigInfoWiringTests.cs | New tests intended to validate UpdateOptions → bootstrap → GlobalConfigInfo wiring. |
| src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs | Uses BackupEnabled to conditionally run backup; constructs orchestrator with mapped options. |
| src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs | Accepts DownloadOrchestratorOptions; wires resume/checksum/retry/diff-mode behavior. |
| src/c#/GeneralUpdate.Core/Download/Models/DownloadOrchestratorOptions.cs | New consolidated configuration object with From(GlobalConfigInfo) mapping and clamping. |
| src/c#/GeneralUpdate.Core/Download/Executors/HttpDownloadExecutor.cs | Adds enableResume toggle and suppresses Range logic when disabled. |
| src/c#/GeneralUpdate.Core/Configuration/GlobalConfigInfo.cs | Adds new runtime properties for download/update behavior (concurrency, retry, checksum, diff mode, backup). |
| src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs | Extends ApplyRuntimeOptions() to populate all newly wired properties from UpdateOptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+54
| // Resolve effective concurrency: Serial mode forces 1 | ||
| var effectiveConcurrency = _options.DiffMode == DiffMode.Serial | ||
| ? 1 | ||
| : DownloadOrchestratorOptions.SanitizeMaxConcurrency(Math.Max(1, maxConcurrency)); |
Comment on lines
+326
to
+328
| var report = await orch.ExecuteAsync(DownloadPlan.Empty, "/tmp", token: CancellationToken.None); | ||
| Assert.Equal(0, report.SuccessCount); | ||
| Assert.Equal(0, report.FailedCount); |
Comment on lines
+37
to
+44
| [Fact] | ||
| public async Task ExecuteAsync_SerialMode_ForcesConcurrencyToOne() | ||
| { | ||
| using var httpClient = new HttpClient(new FakeSuccessHandler()); | ||
| var opts = new DownloadOrchestratorOptions | ||
| { | ||
| DiffMode = DiffMode.Serial, | ||
| MaxConcurrency = 10, |
Comment on lines
+273
to
+274
| var fileInfo = new FileInfo(destPath); | ||
| Assert.True(fileInfo.Length >= 20, $"Expected file >= 20 bytes, got {fileInfo.Length}"); |
Comment on lines
+93
to
+107
| Assert.NotNull(b); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Bootstrap_GetOption_EnableResume_ReturnsSetValue() | ||
| { | ||
| var b = MakeBootstrap().Option(UpdateOptions.EnableResume, false); | ||
| Assert.NotNull(b); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Bootstrap_GetOption_RetryCount_ReturnsSetValue() | ||
| { | ||
| var b = MakeBootstrap().Option(UpdateOptions.RetryCount, 10); | ||
| Assert.NotNull(b); |
- 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wires all 8
UpdateOptionsconfiguration properties that were defined but never connected to runtime behavior.Closes #398
What changed
New file
DownloadOrchestratorOptions— bundles MaxConcurrency, EnableResume, RetryCount, RetryInterval, VerifyChecksum, DiffMode, DownloadTimeout into a single config classModified source files (6)
GlobalConfigInfo.csGeneralUpdateBootstrap.csApplyRuntimeOptions()now reads all 8 options from UpdateOptionsDefaultDownloadOrchestrator.csDownloadOrchestratorOptions; conditional SHA256 skip; Serial mode forces concurrency=1; passes options to policy/executorHttpDownloadExecutor.csenableResumeparameter; skips Range header when falseClientUpdateStrategy.csDownloadOrchestratorOptions.From(_configInfo); conditionalBackup()New test files (3, 56 tests)
DownloadOrchestratorOptionsTests.csOrchestratorOptionsBehaviourTests.csGlobalConfigInfoWiringTests.csTest results
Data flow