Enabling performance tests #1796

johnbeisner · 2017-12-05T22:46:31Z

No description provided.

johnbeisner · 2017-12-11T15:56:56Z

@dsplaisted
Can I get a review of this please.

nguerrera · 2017-12-11T18:44:03Z

Hard to see what changed from #1718. Why did you drop @dsplaisted's commits and attribution?

tannergooding · 2017-12-11T20:45:06Z

sdk.sln

@@ -110,6 +110,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.NET.Build.Tests",
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.NET.Pack.Tests", "src\Tests\Microsoft.NET.Pack.Tests\Microsoft.NET.Pack.Tests.csproj", "{8746DC05-3035-4F24-9F2C-BAAAB5B50FD3}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.NET.Perf.Tests", "src\Tests\Microsoft.NET.Perf.Tests\Microsoft.NET.Perf.Tests.csproj", "{AFB72217-54A0-4E76-870A-42718B8E0AE5}"


Do we want the perf tests to run as part of the unit test switch, or do we want them to run separately (which, I believe, is what most of the other repos are doing)?

We want the perf tests to run with a single iteration during normal tests. That will ensure that they don't get functionally broken. When running in the perf lab, they should run with more iterations, but this will only happen automatically after a PR is merged (though it will be possible to request a perf run for an unmerged PR).

johnbeisner · 2017-12-11T21:03:13Z

@nguerrera
SDK is now moved to repo-toolset - some of the files Daniel had changed no longer exist. Therefore, I started from the core perf tests and worked to enabled them using Daniel's changes as a guide.

tmat · 2017-12-13T22:18:04Z

src/Tests/Microsoft.NET.Perf.Tests/Microsoft.NET.Perf.Tests.csproj

+    <PackageReference Include="Microsoft.Diagnostics.Tracing.TraceEvent" Version="1.0.3-alpha-experimental" />
+    <PackageReference Include="Microsoft.DotNet.Cli.Utils" Version="$(MicrosoftDotNetCliUtilsVersion)" />
+    <PackageReference Include="FluentAssertions" Version="$(FluentAssertionsVersion)" />
+    <PackageReference Include="xunit.performance.api" Version="1.0.0-beta-build0011" />


Add a variable $(XUnitPerformanceApiVersion)

dsplaisted · 2017-12-18T19:46:26Z

src/Tests/Microsoft.NET.Perf.Tests/BuildPerf.cs

@@ -24,7 +24,9 @@ public BuildPerf(ITestOutputHelper log) : base(log)

        //  These tests are currently disabled for full framework MSBuild because the CI machines don't
        //  have an MSBuild that supports the /restore command-line argument
-        [CoreMSBuildOnlyTheory]
+        //  Also, Microsoft.Xunit.Performance.Api.ProcessExitInfo doesn't handle non-Windows
+        //  information gathering - disabling for non-Windows


Please file a bug for this in https://github.com/microsoft/xunit-performance/issues and link to that issue in this comment.

dsplaisted · 2018-01-03T23:43:16Z

src/Tests/Microsoft.NET.Perf.Tests/BuildPerf.cs

+        //  These tests are currently disabled for full framework MSBuild because the CI machines don't
+        //  have an MSBuild that supports the /restore command-line argument
+        //  Also, Microsoft.Xunit.Performance.Api.ProcessExitInfo doesn't handle non-Windows
+        //  information gathering - disabling for non-Windows


File a bug in https://github.com/Microsoft/xunit-performance for this, and link to it from this comment.

dsplaisted · 2018-01-03T23:57:29Z

src/Tests/Microsoft.NET.TestFramework/Microsoft.NET.TestFramework.csproj

-    <Copy SourceFiles="@(_CopyDirectoryBuildTestDependenciesInput)"
-          DestinationFiles="@(_CopyDirectoryBuildTestDependenciesOutput)" />
+  <Target Name="_CopyDirectoryBuildTestDependencies" AfterTargets="Build" Inputs="@(_CopyDirectoryBuildTestDependenciesInput)" Outputs="@(_CopyDirectoryBuildTestDependenciesOutput)">
+    <Copy SourceFiles="@(_CopyDirectoryBuildTestDependenciesInput)" DestinationFiles="@(_CopyDirectoryBuildTestDependenciesOutput)" />


This looks like it's just a formatting change (probably by Visual Studio). Revert this.

tannergooding · 2018-01-04T20:57:08Z

src/Tests/Microsoft.NET.Perf.Tests/PerfTest.cs

+
+namespace Microsoft.NET.Perf.Tests
+{
+    public class PerfTest


I thought that both perf frameworks provided infrastructure for all this. Why are we writing our own?

dsplaisted · 2018-01-04T21:04:25Z

src/Tests/Microsoft.NET.Perf.Tests/PerfTest.cs

+            TestName = TestName ?? callerName;
+
+            Stopwatch stopwatch = new Stopwatch();
+            TimeSpan[] executionTimes = new TimeSpan[NumberOfIterations];


The stopwatch and executionTimes variables are left over from previous versions of the code and should now be deleted.

tannergooding · 2018-01-04T21:07:38Z

Overall it looks ok. However, I would like to see the following:

Removal of all the unused variables/etc (after talking with @dsplaisted, many are leftover from previous prototypes of the code).
Bugs logged against the perf framework we are using that track usability issues/improvements that are needed (we are currently doing a lot of manual setup/teardown and value initialization for things that should probably be part of the perf API).

livarcocc · 2018-01-04T23:53:21Z

src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj

@@ -88,6 +89,16 @@
      <LayoutFile Include="@(PackFile)">
        <TargetPath>..\NuGet.Build.Tasks.Pack\%(PackFile.RecursiveDir)%(PackFile.Filename)%(PackFile.Extension)</TargetPath>
      </LayoutFile>
+
+      <!-- Include some of the Sdks from the Stage 0 CLI for performance tests-->


Are these the SDK files that the CLI requires that are not produced by dotnet/sdk?

These are SDK files that the Performance tests require that are not produced by dotnet/sdk

livarcocc · 2018-01-04T23:54:51Z

src/Tests/Microsoft.NET.Perf.Tests/BuildPerf.cs

+        }
+
+        [CoreMSBuildOnlyTheory(Skip ="The code for these scenarios needs to be acquired during the test run (instead of relying on hard-coded local path)")]
+


NIT: empty line here seems weird.

livarcocc · 2018-01-04T23:55:47Z

src/Tests/Microsoft.NET.Perf.Tests/BuildPerf.cs

+
+            var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: operation.ToString());
+
+            TestProject(testAsset.Path, ".NET Core 2 Console App", operation);


This name, TestProject, does it mean go run a test on the project or is it a more of a name to create TestProject, which happens to do things like build.

The name seems strange here. Does not read like an action. At least, I didn't read it as such at first.

livarcocc · 2018-01-04T23:57:38Z

src/Tests/Microsoft.NET.Perf.Tests/FolderSnapshot.cs

+    {
+        string OriginalPath { get; set; }
+        string BackupPath { get; set; }
+


NIT: there is an extra empty line here.

livarcocc · 2018-01-04T23:58:13Z

src/Tests/Microsoft.NET.Perf.Tests/FolderSnapshot.cs

+
+        public static FolderSnapshot Create(string path)
+        {
+            FolderSnapshot ret = new FolderSnapshot();


why is this called ret?

livarcocc · 2018-01-05T00:00:52Z

src/Tests/Microsoft.NET.Perf.Tests/FolderSnapshot.cs

+            {
+                Directory.Delete(dest, true);
+            }
+            CopyDirectory(source, dest);


NIT: empty line after closing brackets. Check this in the rest of the files.

We should add this to our editorconfig.

livarcocc · 2018-01-05T00:57:30Z

src/Tests/Microsoft.NET.Perf.Tests/ShowToolsetInfo.cs

+        }
+
+        [Fact]
+        public void ShowToolsetPaths()


What is this test doing?

livarcocc · 2018-01-05T00:58:05Z

src/Tests/Microsoft.NET.TestFramework/Commands/DotnetCommand.cs

        }

        protected override SdkCommandSpec CreateCommand(string[] args)
        {
-            return new SdkCommandSpec()
+            var ret = new SdkCommandSpec()


ret? better name please.

* First draft enabling performance tests... * Resolved BUG: microsoft/xunit-performance#248

* First draft enabling performance tests... * Disabling tests for non-Windows... * Updates... * Missing "\" * Set 'DefaultIterations' to "1" * Resolved BUG: microsoft/xunit-performance#248 * Fixing typos... * Formatting, variable names changes. * Fixing a typo...

First draft enabling performance tests...

388b042

johnbeisner self-assigned this Dec 5, 2017

johnbeisner changed the title ~~First draft enabling performance tests...~~ WIP: First draft enabling performance tests... Dec 6, 2017

johnbeisner mentioned this pull request Dec 8, 2017

Add perf tests #1718

Closed

Disabling tests for non-Windows...

c6d1505

johnbeisner requested review from nguerrera, livarcocc and dsplaisted December 8, 2017 20:05

tannergooding reviewed Dec 11, 2017

View reviewed changes

johnbeisner requested a review from wli3 December 11, 2017 21:03

tmat reviewed Dec 13, 2017

View reviewed changes

John Beisner added 2 commits January 1, 2018 14:32

Updates...

a65549d

Missing "\"

6a184ae

dsplaisted approved these changes Jan 3, 2018

View reviewed changes

Set 'DefaultIterations' to "1"

8298f8f

johnbeisner changed the title ~~WIP: First draft enabling performance tests...~~ Enabling performance tests Jan 4, 2018

tannergooding reviewed Jan 4, 2018

View reviewed changes

dsplaisted approved these changes Jan 4, 2018

View reviewed changes

livarcocc reviewed Jan 4, 2018

View reviewed changes

livarcocc reviewed Jan 5, 2018

View reviewed changes

src/Tests/Microsoft.NET.Perf.Tests/ShowToolsetInfo.cs

}

[Fact]

public void ShowToolsetPaths()

Copy link

Contributor

livarcocc Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this test doing?

livarcocc reviewed Jan 5, 2018

View reviewed changes

jbeisner added 6 commits January 9, 2018 01:55

Resolved BUG: microsoft/xunit-performance#248

6003018

Fixing typos...

d75a7a3

Formatting, variable names changes.

706efdf

Fixing a typo...

879e9f7

Merge remote-tracking branch 'upstream/release/2.0.0' into PerfTests

80d9622

Merge remote-tracking branch 'upstream/release/2.0.0' into PerfTests

3be7c34

johnbeisner merged commit 195044c into dotnet:release/2.0.0 Jan 24, 2018

johnbeisner pushed a commit that referenced this pull request Jan 30, 2018

Enabling performance tests (#1796) (#1914)

71207a4

* First draft enabling performance tests... * Resolved BUG: microsoft/xunit-performance#248

johnbeisner deleted the PerfTests branch February 25, 2018 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling performance tests #1796

Enabling performance tests #1796

johnbeisner commented Dec 5, 2017

johnbeisner commented Dec 11, 2017

nguerrera commented Dec 11, 2017

tannergooding Dec 11, 2017

dsplaisted Dec 18, 2017

johnbeisner commented Dec 11, 2017

tmat Dec 13, 2017

dsplaisted Dec 18, 2017

dsplaisted Jan 3, 2018

dsplaisted Jan 3, 2018

tannergooding Jan 4, 2018

dsplaisted Jan 4, 2018

johnbeisner Jan 9, 2018

tannergooding commented Jan 4, 2018

livarcocc Jan 4, 2018

johnbeisner Jan 9, 2018

livarcocc Jan 4, 2018

johnbeisner Jan 9, 2018

livarcocc Jan 4, 2018

livarcocc Jan 4, 2018

johnbeisner Jan 9, 2018

livarcocc Jan 4, 2018

johnbeisner Jan 9, 2018

livarcocc Jan 5, 2018

johnbeisner Jan 9, 2018

livarcocc Jan 5, 2018

livarcocc Jan 5, 2018

johnbeisner Jan 9, 2018

		}

		[CoreMSBuildOnlyTheory(Skip ="The code for these scenarios needs to be acquired during the test run (instead of relying on hard-coded local path)")]


		var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: operation.ToString());

		TestProject(testAsset.Path, ".NET Core 2 Console App", operation);

Enabling performance tests #1796

Enabling performance tests #1796

Conversation

johnbeisner commented Dec 5, 2017

johnbeisner commented Dec 11, 2017

nguerrera commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbeisner commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment