Fixing #45

dotnet · Jan 5, 2016 · 60047e7 · 60047e7
1 parent 8b05636
commit 60047e7
Show file tree

Hide file tree

Showing 3 changed files with 133 additions and 9 deletions.
diff --git a/BenchmarkDotNet.IntegrationTests/ParamsAttributeStaticTest.cs b/BenchmarkDotNet.IntegrationTests/ParamsAttributeStaticTest.cs
@@ -1,7 +1,6 @@
 using BenchmarkDotNet.Tasks;
 using System;
 using System.Collections.Generic;
-using System.Linq;
 using BenchmarkDotNet.Plugins;
 using BenchmarkDotNet.Plugins.Loggers;
 using Xunit;
@@ -21,7 +20,7 @@ public void Test()
         {
             var logger = new BenchmarkAccumulationLogger();
             var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
-            var reports = new BenchmarkRunner(plugins).Run<ParamsTestStaticProperty>();
+            var reports = new BenchmarkRunner(plugins).Run(this.GetType());
             foreach (var param in new[] { 1, 2, 3, 8, 9, 10 })
                 Assert.Contains($"// ### New Parameter {param} ###" + Environment.NewLine, logger.GetLog());
             Assert.DoesNotContain($"// ### New Parameter {default(int)} ###" + Environment.NewLine, logger.GetLog());
@@ -39,6 +38,35 @@ public void Benchmark()
         }
     }
 
+    // Delibrately made the Property "static" to ensure that Params also work okay in this scenario
+    public class ParamsTestStaticPrivateProperty
+    {
+        [Params(1, 2, 3, 8, 9, 10)]
+        public static int StaticParamProperty { get; private set; }
+
+        private static HashSet<int> collectedParams = new HashSet<int>();
+
+        [Fact]
+        public void Test()
+        {
+            var logger = new BenchmarkAccumulationLogger();
+            var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
+            // System.InvalidOperationException : Property "StaticParamProperty" must be public and writable if it has the [Params(..)] attribute applied to it
+            Assert.Throws<InvalidOperationException>(() => new BenchmarkRunner(plugins).Run(this.GetType()));
+        }
+
+        [Benchmark]
+        [BenchmarkTask(mode: BenchmarkMode.SingleRun, processCount: 1, warmupIterationCount: 1, targetIterationCount: 1)]
+        public void Benchmark()
+        {
+            if (collectedParams.Contains(StaticParamProperty) == false)
+            {
+                Console.WriteLine($"// ### New Parameter {StaticParamProperty} ###");
+                collectedParams.Add(StaticParamProperty);
+            }
+        }
+    }
+
     // Delibrately made everything "static" (as well as using a Field) to ensure that Params also work okay in this scenario
     public class ParamsTestStaticField
     {
@@ -69,4 +97,33 @@ public static void Benchmark()
             }
         }
     }
+
+    // Delibrately made everything "static" (as well as using a Field) to ensure that Params also work okay in this scenario
+    public class ParamsTestStaticPrivateField
+    {
+        [Params(1, 2, 3, 8, 9, 10)]
+        private static int StaticParamField = 0;
+
+        private static HashSet<int> collectedParams = new HashSet<int>();
+
+        [Fact]
+        public static void Test()
+        {
+            var logger = new BenchmarkAccumulationLogger();
+            var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
+            // System.InvalidOperationException : Field "StaticParamField" must be public if it has the [Params(..)] attribute applied to it
+            Assert.Throws<InvalidOperationException>(() => new BenchmarkRunner(plugins).Run<ParamsTestStaticPrivateField>());
+        }
+
+        [Benchmark]
+        [BenchmarkTask(mode: BenchmarkMode.SingleRun, processCount: 1, warmupIterationCount: 1, targetIterationCount: 1)]
+        public static void Benchmark()
+        {
+            if (collectedParams.Contains(StaticParamField) == false)
+            {
+                Console.WriteLine($"// ### New Parameter {StaticParamField} ###");
+                collectedParams.Add(StaticParamField);
+            }
+        }
+    }
 }
diff --git a/BenchmarkDotNet.IntegrationTests/ParamsAttributeTest.cs b/BenchmarkDotNet.IntegrationTests/ParamsAttributeTest.cs
@@ -19,7 +19,7 @@ public void Test()
         {
             var logger = new BenchmarkAccumulationLogger();
             var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
-            var reports = new BenchmarkRunner(plugins).Run<ParamsTestProperty>();
+            var reports = new BenchmarkRunner(plugins).Run(this.GetType());
             foreach (var param in new[] { 1, 2, 3, 8, 9, 10 })
                 Assert.Contains($"// ### New Parameter {param} ###" + Environment.NewLine, logger.GetLog());
             Assert.DoesNotContain($"// ### New Parameter {default(int)} ###" + Environment.NewLine, logger.GetLog());
@@ -37,6 +37,34 @@ public void Benchmark()
         }
     }
 
+    public class ParamsTestPrivateProperty
+    {
+        [Params(1, 2, 3, 8, 9, 10)]
+        public int ParamProperty { get; private set; }
+
+        private HashSet<int> collectedParams = new HashSet<int>();
+
+        [Fact]
+        public void Test()
+        {
+            var logger = new BenchmarkAccumulationLogger();
+            var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
+            // System.InvalidOperationException : Property "ParamProperty" must be public and writable if it has the [Params(..)] attribute applied to it
+            Assert.Throws<InvalidOperationException>(() => new BenchmarkRunner(plugins).Run(this.GetType()));
+        }
+
+        [Benchmark]
+        [BenchmarkTask(mode: BenchmarkMode.SingleRun, processCount: 1, warmupIterationCount: 1, targetIterationCount: 1)]
+        public void Benchmark()
+        {
+            if (collectedParams.Contains(ParamProperty) == false)
+            {
+                Console.WriteLine($"// ### New Parameter {ParamProperty} ###");
+                collectedParams.Add(ParamProperty);
+            }
+        }
+    }
+
     public class ParamsTestField
     {
         [Params(1, 2, 3, 8, 9, 10)]
@@ -49,7 +77,7 @@ public void Test()
         {
             var logger = new BenchmarkAccumulationLogger();
             var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
-            var reports = new BenchmarkRunner(plugins).Run<ParamsTestField>();
+            var reports = new BenchmarkRunner(plugins).Run(this.GetType());
             foreach (var param in new[] { 1, 2, 3, 8, 9, 10 })
                 Assert.Contains($"// ### New Parameter {param} ###" + Environment.NewLine, logger.GetLog());
             Assert.DoesNotContain($"// ### New Parameter 0 ###" + Environment.NewLine, logger.GetLog());
@@ -66,4 +94,32 @@ public void Benchmark()
             }
         }
     }
+
+    public class ParamsTestPrivateField
+    {
+        [Params(1, 2, 3, 8, 9, 10)]
+        private int ParamField = 0;
+
+        private HashSet<int> collectedParams = new HashSet<int>();
+
+        [Fact]
+        public void Test()
+        {
+            var logger = new BenchmarkAccumulationLogger();
+            var plugins = BenchmarkPluginBuilder.CreateDefault().AddLogger(logger).Build();
+            // System.InvalidOperationException : Field "ParamField" must be public if it has the [Params(..)] attribute applied to it
+            Assert.Throws<InvalidOperationException>(() => new BenchmarkRunner(plugins).Run(this.GetType()));
+        }
+
+        [Benchmark]
+        [BenchmarkTask(mode: BenchmarkMode.SingleRun, processCount: 1, warmupIterationCount: 1, targetIterationCount: 1)]
+        public void Benchmark()
+        {
+            if (collectedParams.Contains(ParamField) == false)
+            {
+                Console.WriteLine($"// ### New Parameter {ParamField} ###");
+                collectedParams.Add(ParamField);
+            }
+        }
+    }
 }
diff --git a/BenchmarkDotNet/BenchmarkConverter.cs b/BenchmarkDotNet/BenchmarkConverter.cs
@@ -26,20 +26,31 @@ public static IEnumerable<Benchmark> TypeToBenchmarks(Type type)
             }
 
             // If there is one, get the single Field or Property that has the [Params(..)] attribute
-            var fields = type.GetFields().Select(f => new
+            var reflectionFlags = BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic;
+            var fields = type.GetFields(reflectionFlags).Select(f => new
             {
                 f.Name,
                 Attribute = f.ResolveAttribute<ParamsAttribute>(),
+                Private = f.IsPrivate,
                 IsStatic = f.IsStatic,
             });
-            var properties = type.GetProperties().Select(f => new
+            var properties = type.GetProperties(reflectionFlags).Select(p => new
             {
-                f.Name,
-                Attribute = f.ResolveAttribute<ParamsAttribute>(),
-                IsStatic = f.GetSetMethod().IsStatic
+                p.Name,
+                Attribute = p.ResolveAttribute<ParamsAttribute>(),
+                Private = p.GetSetMethod() == null ? true : false,
+                IsStatic = p.GetSetMethod() != null ? p.GetSetMethod().IsStatic : false
             });
             var fieldOrProperty = fields.Concat(properties).FirstOrDefault(i => i.Attribute != null);
 
+            var privateField = fields.FirstOrDefault(f => f.Attribute != null && f.Private);
+            if (privateField != null)
+                throw new InvalidOperationException($"Field \"{privateField.Name}\" must be public if it has the [Params(..)] attribute applied to it");
+
+            var privateProperty = properties.FirstOrDefault(p => p.Attribute != null && p.Private);
+            if (privateProperty != null)
+                throw new InvalidOperationException($"Property \"{privateProperty.Name}\" must be public and writable if it has the [Params(..)] attribute applied to it");
+
             for (int i = 0; i < methods.Length; i++)
             {
                 var methodInfo = methods[i];