Profile CLI Switch#199
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Code ReviewBranch: OverviewThis PR introduces a public, provider-agnostic The core implementation is excellent. One must-fix blocker needs to be addressed before merging. Bugs / CorrectnessNo issues found.
SecurityNo issues found.
Convention AdherenceMUST-FIX —
|
| File | What it covers |
|---|---|
OoklaSpeedtestSettingsTests.Profiles.cs |
Field-for-field equality for all 5 profiles against architecture doc values |
OoklaSpeedtestSettingsTests.cs |
Parameterless ctor chains to Medium; invalid profile throws; with preserves shape; no stored Profile property |
NetPaceConsoleTests.Profile.cs |
CLI case-insensitivity; unknown value rejected; --profile is authoritative; default is Medium; override semantics (--downloadsize/--uploadsize mutate cap only); --no-download short-circuit |
ProfileTests.cs |
Namespace, source file path, no extension methods, enum member definitions, default value is Medium |
ProfileXmlDocTests.cs |
Mega's XML doc contains undocumented, 5000/6000/7000, and architecture doc reference |
CIR behaviour scenarios map 1:1 to test methods. No gaps identified.
Spec / Test-Plan Alignment
No mismatches found.
All seven CIR behaviour scenarios have explicit test coverage. The data-model values in tests agree exactly with docs/architecture/download-upload-size-controls.md §5 tables. The architectural constraint (Profile knows no provider; one inline switch; no factory methods) is verified by reflection tests.
Other Observations
OoklaSpeedtestSettingsAccessordesign: The singleton accessor pattern for the test seam is well-chosen — it decouples CLI parsing verification from writer internals and avoids coupling writers to Ookla-specific settings. The per-test instantiation inBuildServicesWithSettingsAccessor()prevents cross-test state leakage.- Mega caveat documentation: The three-way documentation (XML doc, USER_GUIDE warning box, architecture doc §5.3) for Mega's bonus-payload dependency is thorough. Users and NuGet consumers both see it at the right moment.
- Help snapshot: The
--profileentry inExpectations/NetPaceConsoleTests.Should_Display_Help.verified.txtcorrectly showsMediumas the default and the valid values list. Ordering is sensible (between--datetimeformatand--downloadsize).
Summary
| Category | Status | Notes |
|---|---|---|
| Bugs | ✅ None | |
| Security | ✅ None | |
| Conventions | ❌ 1 MUST-FIX | Remove 12 ad-hoc entries from .claude/settings.json allow list |
| Test coverage | ✅ Excellent | 23 tests, full scenario coverage |
| Spec alignment | ✅ Clean | All CIR behaviours tested |
Verdict: CONDITIONAL APPROVAL — ready to merge once the .claude/settings.json allow-list entries added during this PR are removed.
|
@claude Can you investigate the build failure here please: https://github.com/FrankRay78/NetPace/actions/runs/25920768575/job/76188887015?pr=199 It runs fine on my local machine and the code review above didn't identify anything. |
|
Claude finished @FrankRay78's task in 3m 25s —— View job Build Failure Investigation — Fixed
Root CauseThe test host was crashing with an unhandled Call stack: What happens: Spectre.Console's Why it passes locally on Windows: Pure race condition. Windows thread scheduling and process-exit ordering happen to avoid the race; Linux CI runners expose it reliably. Fix —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — PR #199 Profile CLI Switch
Overview
Introduces a Profile enum (Tiny | Small | Medium | Large | Mega) on NetPace.Core, surfaced via a --profile CLI switch. Bundles per-request shape (download sizes, iterations, parallelism) and a total-byte cap into one knob. The new Medium default cuts traffic from ~370 MiB to ~125 MiB per run. As noted in the PR description, this is a pre-1.0 breaking change: four int sizeMb overloads are removed from ISpeedTestService.
Bugs / Correctness
No correctness issues found. Key logic is solid:
- Profile → settings mapping uses an exhaustive
switchexpression with a_ => throwarm — no silent defaults. int.MaxValuesentinel for "no override supplied" is consistently applied inBuildOoklaSettings.with-expression on nested records creates copies, not mutations — correct.
Security
No vulnerabilities. System.CommandLine's enum parser rejects out-of-range values at parse time, and the OoklaSpeedtestSettings constructor provides a defensive second guard layer.
API Surface / Breaking Changes
Breaking change is intentional and documented. The removal of the four sizeMb overloads is flagged in the PR title, the CIR, and the spec contracts. The migration path (use DownloadTestSettings.DownloadSizeMb on the settings record) is clear. No inconsistencies detected between what the spec contracts describe and what was actually implemented.
C# Style / Conventions
Minor — BuildOoklaSettings in Program.cs carries a <summary> tag but is missing <param> and <returns>:
/// <param name="settings">Parsed CLI settings including profile and override caps.</param>
/// <returns>OoklaSpeedtestSettings configured for the selected profile with any CLI overrides applied.</returns>Everything else is clean: PascalCase throughout, nullable reference types enabled, async/await on all network paths, global alias using Profile = NetPace.Core.Profile correctly resolves the Spectre.Console name clash.
Test Coverage
Coverage is strong across four test files:
OoklaSpeedtestSettingsTests.Profiles.cs— five facts, one per profile, asserting every settings field.NetPaceConsoleTests.Profile.cs— CLI binding, case-insensitive parsing, unknown value rejection, override logic.ProfileTests.cs— namespace, enum membership, no stray extension methods.ProfileXmlDocTests.cs— verifies Mega's caveat appears in published XML docs.
Minor gap — Contract C-OS-3 specifies that new OoklaSpeedtestSettings((Profile)999) throws ArgumentOutOfRangeException, but there is no [Fact] for it. The System.CommandLine parser prevents this at the CLI layer, so runtime risk is negligible, but the contract is untested. Suggested addition:
[Fact]
public void Constructor_InvalidProfileValue_ThrowsArgumentOutOfRangeException()
{
// Given / When
Action act = () => new OoklaSpeedtestSettings((Profile)999);
// Then
var ex = Assert.Throws<ArgumentOutOfRangeException>(act);
Assert.Equal("profile", ex.ParamName);
}Spec / Test-Plan Traceability
Traceability is excellent. Test method names map cleanly to spec user stories and contract identifiers:
| Test name | Covers |
|---|---|
NoProfileFlag_DefaultsToMedium |
US2 — omitted --profile defaults to Medium |
Profile_CaseInsensitiveEnumParsing_BindsToTiny |
US4 — case-insensitive parsing |
DownloadSizeOverride_PreservesProfileShape_OnTiny |
US5 — override only the byte cap |
Mega_DownloadSizes_ContainsBonusPayloads |
FR-021 — Mega includes 5000/6000/7000 MiB sizes |
Other
DI Singleton note — OoklaSpeedtestSettingsAccessor is registered as Singleton and mutated during the command action. This is safe for the current single-invocation-per-process model, but a brief inline comment would protect the next person from making the container reusable without accounting for this:
// Singleton is safe because each process invocation runs one command action and exits.
// If this container is ever reused across invocations, convert to Scoped.Summary
| Category | Severity | Count |
|---|---|---|
| Correctness bugs | — | 0 |
| Security issues | — | 0 |
| Style / conventions | Nit | 1 (missing XML param/returns tags) |
| Test gaps | Minor | 1 (ArgumentOutOfRangeException for invalid Profile int cast) |
| Design notes | Nit | 1 (DI Singleton comment) |
Recommendation: approve. The feature is well-specified, cleanly implemented, and comprehensively tested. The two nits and one minor test gap are non-blocking.
FrankRay78
force-pushed
the
003-profile-cli-switch
branch
from
56a6324 to
b4c9c96
Compare
|
@claude Something has gone wrong with my rebase, and it's causing the build to fail. It's like I'm missing the fix to the Spectre.Console breaking change - but I don't see how that's happened. |
|
Claude finished @FrankRay78's task in 1m 41s —— View job Rebase Build Failure — Fixed
Root Cause
The test project's global usings ( FixOne-line change in // Before (ambiguous after rebase):
public Profile Profile => AnsiConsole.Console.Profile;
// After (matches CompositeAnsiConsole.cs pattern):
public Spectre.Console.Profile Profile => AnsiConsole.Console.Profile;Build result: Commit: |
FrankRay78
force-pushed
the
003-profile-cli-switch
branch
from
a450fb2 to
578fdad
Compare
2 participants
Summary
Introduces a public, provider-agnostic
Profileenum (Tiny,Small,Medium,Large,Mega) onNetPace.Core, surfaced as a single--profileCLI switch and as two new public constructors onOoklaSpeedtestSettings. The profile bundles per-request shape (DownloadSizes, iterations, parallel tasks) and a total-byte cap into one knob, shifting the default-run traffic from ~370 MiB to ~125 MiB (Medium = the new default, ≥ 65 % reduction) so users on constrained data plans can stay within budget without further tuning. Breaking (pre-1.0): the fourint sizeMbper-call overloads onISpeedTestServiceare deleted; the cap now lives onDownloadTestSettings.DownloadSizeMb/UploadTestSettings.UploadSizeMb.Refs #174
Spec
specs/003-profile-cli-switch
Changed Files
New Artifacts