Repository: google/swift-benchmark
Pull request #45 (2020-06-15 13:51:47)

Make cli settings override benchmark/suite settings
This change makes command-line arguments take higher priority than settings set in the code at benchmark or suite level.


Patch:
diff --git a/Sources/Benchmark/BenchmarkArguments.swift b/Sources/Benchmark/BenchmarkArguments.swift
index ae67b09..5c3fc90 100644
--- a/Sources/Benchmark/BenchmarkArguments.swift
+++ b/Sources/Benchmark/BenchmarkArguments.swift
@@ -75,8 +75,9 @@ public struct BenchmarkArguments: ParsableArguments {
         if iterations != nil && iterations! <= 0 {
             throw ValidationError(positiveNumberError(flag: "--iterations", of: "integer"))
         }
-        if warmupIterations != nil && warmupIterations! <= 0 {
-            throw ValidationError(positiveNumberError(flag: "--warmup-iterations", of: "integer"))
+        if warmupIterations != nil && warmupIterations! < 0 {
+            throw ValidationError(
+                nonNegativeNumberError(flag: "--warmup-iterations", of: "integer"))
         }
         if maxIterations != nil && maxIterations! <= 0 {
             throw ValidationError(positiveNumberError(flag: "--max-iterations", of: "integer"))
@@ -99,4 +100,8 @@ public struct BenchmarkArguments: ParsableArguments {
     func positiveNumberError(flag: String, of type: String) -> String {
         return "Value provided via \(flag) must be a positive \(type)."
     }
+
+    func nonNegativeNumberError(flag: String, of type: String) -> String {
+        return "Value provided via \(flag) must be a non-negative \(type)."
+    }
 }
diff --git a/Sources/Benchmark/BenchmarkRunner.swift b/Sources/Benchmark/BenchmarkRunner.swift
index 045171b..6a813bc 100644
--- a/Sources/Benchmark/BenchmarkRunner.swift
+++ b/Sources/Benchmark/BenchmarkRunner.swift
@@ -40,9 +40,9 @@ public struct BenchmarkRunner {
     mutating func run(benchmark: AnyBenchmark, suite: BenchmarkSuite) throws {
         let settings = BenchmarkSettings([
             defaultSettings,
-            self.settings,
             suite.settings,
             benchmark.settings,
+            self.settings,
         ])
 
         let filter = try BenchmarkFilter(settings.filter)
@@ -54,8 +54,9 @@ public struct BenchmarkRunner {
         let totalStart = now()
 
         var warmupState: BenchmarkState? = nil
-        if let n = settings.warmupIterations {
-            warmupState = doNIterations(n, benchmark: benchmark, suite: suite, settings: settings)
+        if settings.warmupIterations > 0 {
+            warmupState = doNIterations(
+                settings.warmupIterations, benchmark: benchmark, suite: suite, settings: settings)
         }
 
         var state: BenchmarkState
diff --git a/Sources/Benchmark/BenchmarkSetting.swift b/Sources/Benchmark/BenchmarkSetting.swift
index 7cb60cc..e6b1ad7 100644
--- a/Sources/Benchmark/BenchmarkSetting.swift
+++ b/Sources/Benchmark/BenchmarkSetting.swift
@@ -125,8 +125,12 @@ public struct BenchmarkSettings {
     }
 
     /// Convenience accessor for WarmupIterations setting.
-    public var warmupIterations: Int? {
-        return self[WarmupIterations.self]?.value
+    public var warmupIterations: Int {
+        if let value = self[WarmupIterations.self]?.value {
+            return value
+        } else {
+            return 0
+        }
     }
 
     /// Convenience accessor for the Filter setting.


Test patch:
diff --git a/Tests/BenchmarkTests/BenchmarkCommandTests.swift b/Tests/BenchmarkTests/BenchmarkCommandTests.swift
index d4e2080..121ee93 100644
--- a/Tests/BenchmarkTests/BenchmarkCommandTests.swift
+++ b/Tests/BenchmarkTests/BenchmarkCommandTests.swift
@@ -73,10 +73,16 @@ final class BenchmarkCommandTests: XCTestCase {
         }
     }
 
-    func testParseZeroWarmupIterationsError() throws {
-        AssertFailsToParse(
-            ["--warmup-iterations", "0", "--allow-debug-build"],
-            with: "Value provided via --warmup-iterations must be a positive integer.")
+    func testParseZeroWarmupIterations() throws {
+        AssertParse(["--warmup-iterations", "0", "--allow-debug-build"]) { settings in
+            XCTAssertEqual(settings.warmupIterations, 0)
+        }
+    }
+
+    func testParseNoWarmupIterations() throws {
+        AssertParse(["--allow-debug-build"]) { settings in
+            XCTAssertEqual(settings.warmupIterations, 0)
+        }
     }
 
     func testParseMaxIterations() throws {
@@ -87,7 +93,7 @@ final class BenchmarkCommandTests: XCTestCase {
 
     func testParseZeroMaxIterationsError() throws {
         AssertFailsToParse(
-            ["--warmup-iterations", "0", "--allow-debug-build"],
+            ["--max-iterations", "0", "--allow-debug-build"],
             with: "Value provided via --max-iterations must be a positive integer.")
     }
 
@@ -110,9 +116,10 @@ final class BenchmarkCommandTests: XCTestCase {
         ("testParseIterations", testParseIterations),
         ("testParseZeroIterationsError", testParseZeroIterationsError),
         ("testParseWarmupIterations", testParseWarmupIterations),
-        ("testParseZeroWarmupIterationsError", testParseZeroWarmupIterationsError),
+        ("testParseZeroWarmupIterations", testParseZeroWarmupIterations),
+        ("testParseNoWarmupIterations", testParseNoWarmupIterations),
         ("testParseMaxIterations", testParseWarmupIterations),
-        ("testParseZeroMaxIterationsError", testParseZeroWarmupIterationsError),
+        ("testParseZeroMaxIterationsError", testParseZeroMaxIterationsError),
         ("testParseMinTime", testParseMinTime),
         ("testParseZeroMinTimeError", testParseZeroMinTimeError),
     ]
diff --git a/Tests/BenchmarkTests/BenchmarkSettingTests.swift b/Tests/BenchmarkTests/BenchmarkSettingTests.swift
index 6c2b776..e28105e 100644
--- a/Tests/BenchmarkTests/BenchmarkSettingTests.swift
+++ b/Tests/BenchmarkTests/BenchmarkSettingTests.swift
@@ -21,7 +21,7 @@ final class BenchmarkSettingTests: XCTestCase {
     func assertNumberOfIterations(
         suite: BenchmarkSuite,
         counts expected: [Int],
-        cli settings: [BenchmarkSetting] = [Iterations(100000)]
+        cli settings: [BenchmarkSetting]
     ) throws {
         var runner = BenchmarkRunner(
             suites: [suite], settings: settings, reporter: BlackHoleReporter())
@@ -40,7 +40,10 @@ final class BenchmarkSettingTests: XCTestCase {
             suite.benchmark("a") {}
             suite.benchmark("b") {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [100000, 100000])
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [1_000_000, 1_000_000],
+            cli: [])
     }
 
     func testSuiteSetting() throws {
@@ -48,7 +51,10 @@ final class BenchmarkSettingTests: XCTestCase {
             suite.benchmark("a") {}
             suite.benchmark("b") {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [42, 42])
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [42, 42],
+            cli: [])
     }
 
     func testBenchmarkSetting() throws {
@@ -56,7 +62,10 @@ final class BenchmarkSettingTests: XCTestCase {
             suite.benchmark("a") {}
             suite.benchmark("b", settings: Iterations(42)) {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [100000, 42])
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [1_000_000, 42],
+            cli: [])
     }
 
     func testBenchmarkSettingOverridesSuiteSetting() throws {
@@ -64,43 +73,54 @@ final class BenchmarkSettingTests: XCTestCase {
             suite.benchmark("a") {}
             suite.benchmark("b", settings: Iterations(21)) {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [42, 21])
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [42, 21],
+            cli: [])
     }
 
     func testCliSetting() throws {
-        let cli: [BenchmarkSetting] = [Iterations(1)]
         let suite = BenchmarkSuite(name: "Test") { suite in
             suite.benchmark("a") {}
             suite.benchmark("b") {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [1, 1], cli: cli)
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [1, 1],
+            cli: [Iterations(1)])
     }
 
-    func testSuiteOverridesCliSetting() throws {
-        let cli: [BenchmarkSetting] = [Iterations(1)]
+    func testCliOverridesSuiteSetting() throws {
         let suite = BenchmarkSuite(name: "Test", settings: Iterations(2)) { suite in
             suite.benchmark("a") {}
             suite.benchmark("b") {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [2, 2], cli: cli)
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [1, 1],
+            cli: [Iterations(1)])
     }
 
-    func testBenchmarkOverridesCliSetting() throws {
-        let cli: [BenchmarkSetting] = [Iterations(1)]
+    func testCliOverridesBenchmarkSetting() throws {
         let suite = BenchmarkSuite(name: "Test") { suite in
             suite.benchmark("a") {}
             suite.benchmark("b", settings: Iterations(2)) {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [1, 2], cli: cli)
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [1, 1],
+            cli: [Iterations(1)])
     }
 
-    func testBenchmarkAndSuiteOverridesCliSetting() throws {
-        let cli: [BenchmarkSetting] = [Iterations(1)]
+    func testCliOverridesBenchmarkAndSuiteSetting() throws {
         let suite = BenchmarkSuite(name: "Test", settings: Iterations(2)) { suite in
             suite.benchmark("a") {}
             suite.benchmark("b", settings: Iterations(3)) {}
         }
-        try assertNumberOfIterations(suite: suite, counts: [2, 3], cli: cli)
+        try assertNumberOfIterations(
+            suite: suite,
+            counts: [1, 1],
+            cli: [Iterations(1)])
     }
 
     static var allTests = [
@@ -109,8 +129,8 @@ final class BenchmarkSettingTests: XCTestCase {
         ("testBenchmarkSetting", testBenchmarkSetting),
         ("testBenchmarkSettingOverridesSuiteSetting", testBenchmarkSettingOverridesSuiteSetting),
         ("testCliSetting", testCliSetting),
-        ("testSuiteOverridesCliSetting", testSuiteOverridesCliSetting),
-        ("testBenchmarkOverridesCliSetting", testBenchmarkOverridesCliSetting),
-        ("testBenchmarkAndSuiteOverridesCliSetting", testBenchmarkAndSuiteOverridesCliSetting),
+        ("testCliOverridesSuiteSetting", testCliOverridesSuiteSetting),
+        ("testCliOverridesBenchmarkSetting", testCliOverridesBenchmarkSetting),
+        ("testCliOverridesBenchmarkAndSuiteSetting", testCliOverridesBenchmarkAndSuiteSetting),
     ]
 }