Skip to content

Mark SpeedTestResult and LatencyTestResult properties as required #242

Description

@FrankRay78

Problem

SpeedTestResult and LatencyTestResult let every property default silently. A construction site that forgets one compiles cleanly and ships a result that looks real but is not.

This is not hypothetical. During PR #222 review, VariableSpeedTester was found returning a non-zero speed alongside RequestsSucceeded = 0 and RequestsFailed = 0 — zero requests attempted, yet 0.25 Mbps reported. It had been missed when the request counts were introduced, and it silently poisoned 11 committed snapshots before a human spotted the impossible numbers in a CSV row. required would have made it a compile error.

Proposal

Mark the properties required on both result types:

  • SpeedTestResultBytesProcessed, ElapsedMilliseconds, RequestsSucceeded, RequestsFailed
  • LatencyTestResult — same treatment, for consistency

All properties, not just the newest ones. A result missing BytesProcessed is as meaningless as one missing its counts, and marking only the recent additions would read as an accident of history rather than a decision.

Known complication

All four console writers open with a parameterless placeholder standing in for a test that did not run:

var downloadResult = new SpeedTestResult();
var uploadResult = new SpeedTestResult();

Eight sites across CSVConsoleWriter, DefaultConsoleWriter, JsonConsoleWriter and MinimalConsoleWriter. required makes that construction illegal, so each local becomes SpeedTestResult? and the downstream uses have to handle null.

That is the change doing its job rather than an obstacle: the placeholder currently lies. Zero bytes and zero requests is indistinguishable from a genuine all-failed measurement — the same confusion the VariableSpeedTester bug caused. Making "did not run" a null rather than a zeroed object removes the ambiguity at the type level.

Scope

  • NetPace.CoreSpeedTestResult, LatencyTestResult, Clients/Ookla/OoklaSpeedtest, Clients/Testing/SpeedTestStub, Clients/Testing/VariableSpeedTester
  • NetPace.Console — the four console writers
  • Tests — ScriptedSpeedTester, SpeedTestResultTests, SpeedTestExtensionsTests.Guards

Outcome

  • A result object cannot be constructed without stating every value it reports.
  • A test that did not run is representable only as absence, never as a zeroed result.
  • Behaviour and output are unchanged; the existing suite passes without snapshot churn.

Versioning

Source-breaking for anyone implementing ISpeedTestService, so it warrants a MAJOR bump under Principle VII. Pre-V1, so the cost is acceptable now and rises later — this is the cheap moment to do it.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    housekeepingMaintenance, refactoring & general housekeeping

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions