Migration to netstandard 1.4 #406

mayankbansal018 · 2017-01-30T15:23:34Z

Migrating CoreUtilites to netstandard1.4
Adding a platform abstraction layer, to implement platform specific requirements

1. Migrating CoreUtilites to netstandard1.4 2. Adding a platform abstraction layer, to implement platform specific requirements

msftclas · 2017-01-30T15:23:38Z

Hi @mayankbansal018, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Mayank Bansal). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

codito · 2017-01-31T02:26:20Z

scripts/build.ps1

@@ -252,6 +252,22 @@ function Publish-Package
    Write-Log "Publish-Package: Complete. {$(Get-ElapsedTime($timer))}"
 }

+function Patch-Publish-Package


Publish-PlatfromAbstractions, call from within Publish-Package.

codito · 2017-01-31T02:26:34Z

scripts/build.ps1

+    $fullCLRPackageDir = Get-FullCLRPackageDirectory
+    $coreCLRPackageDir = Get-CoreCLRPackageDirectory
+
+	$platformAbstraction = Join-Path $env:TP_ROOT_DIR "src\Microsoft.TestPlatform.PlatformAbstractions\bin\$TPB_Configuration"


codito · 2017-01-31T02:28:05Z

src/Microsoft.TestPlatform.CrossPlatEngine/Microsoft.TestPlatform.CrossPlatEngine.csproj

@@ -30,6 +30,9 @@
    <PackageReference Include="System.Diagnostics.Process">
      <Version>4.3.0</Version>
    </PackageReference>
+    <PackageReference Include="System.Threading.Thread">


Was this dependency getting implicitly pulled in earlier?

yes It was coming from CoreUtilities.csproj

codito · 2017-01-31T02:28:24Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/Tracing/IEqtTrace.cs

+
+namespace Microsoft.VisualStudio.TestPlatform.ObjectModel
+{
+    //


nit: /// for summaries.

codito · 2017-01-31T02:29:02Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/Tracing/IEqtTrace.cs

+    // Summary:
+    //     Specifies what messages to output for the System.Diagnostics.Debug, System.Diagnostics.Trace
+    //     and System.Diagnostics.TraceSwitch classes.
+    public enum CustomTraceLevel


Should we call this TestPlatformTraceLevel?

codito · 2017-01-31T02:30:45Z

Remove ProjectGuid from PlatformAbstractions csproj.

codito · 2017-01-31T02:34:45Z

src/Microsoft.TestPlatform.PlatformAbstractions/net46/Tracing/RollingFileTraceListener.cs

@@ -37,7 +34,7 @@ public class RollingFileTraceListener : TextWriterTraceListener
        {
            if (string.IsNullOrWhiteSpace(fileName))
            {
-                throw new ArgumentException(Resources.CannotBeNullOrEmpty, nameof(fileName));
+                throw new ArgumentException(nameof(fileName));


Is the string Resources.CannotBeNullOrEmpty redundant?

codito · 2017-01-31T02:37:28Z

src/Microsoft.TestPlatform.PlatformAbstractions/netcore/Tracing/EqtTrace.cs

+    using System.IO;
+    using System.Threading;
+
+    /// <summary>


We can move documentation for public members to IPlatformEqtTrace and use /// <inheritdoc/> to reduce duplication.

codito · 2017-01-31T02:38:02Z

src/TestPlatform.TranslationLayer.nuspec

@@ -49,12 +49,14 @@
    <file src="net46\$Runtime$\Microsoft.TestPlatform.VsTestConsole.TranslationLayer.dll" target="lib\net46\" />
    <file src="net46\$Runtime$\Microsoft.TestPlatform.CommunicationUtilities.dll" target="lib\net46\" />
    <file src="net46\$Runtime$\Microsoft.TestPlatform.CoreUtilities.dll" target="lib\net46\" />    
+	<file src="net46\$Runtime$\Microsoft.TestPlatform.PlatformAbstractions.dll" target="lib\net46\" />


nit: indent. Same for below.

codito · 2017-01-31T02:38:12Z

src/TestPlatform.ObjectModel.nuspec

@@ -49,9 +49,11 @@
  <files>	
    <file src="net46\$Runtime$\Microsoft.VisualStudio.TestPlatform.ObjectModel.dll" target="lib\net46\" />
    <file src="net46\$Runtime$\Microsoft.TestPlatform.CoreUtilities.dll" target="lib\net46\" />
+	<file src="net46\$Runtime$\Microsoft.TestPlatform.PlatformAbstractions.dll" target="lib\net46\" />


nit: indent. Same for below.

codito · 2017-01-31T02:39:01Z

src/Microsoft.TestPlatform.PlatformAbstractions/netcore/Tracing/EqtTrace.cs

+    ///       We pass through exceptions thrown due to incorrect arguments to <c>EqtTrace</c> methods.
+    /// Usage: <c>EqtTrace.Info("Here's how to trace info");</c>
+    /// </summary>
+    public class PlatformEqtTrace : IPlatformEqtTrace


What is the test strategy for this class?

codito

Looks good with few comments.

AbhitejJohn · 2017-01-31T05:17:46Z

src/Microsoft.TestPlatform.PlatformAbstractions/Interfaces/Tracing/IEqtTrace.cs

+
+        void SetTraceLevel(CustomTraceLevel value);
+
+#if NET46


:( Isn't this smell in an interface?

AbhitejJohn · 2017-01-31T05:22:23Z

src/Microsoft.TestPlatform.PlatformAbstractions/netstandard1.0/Tracing/EqtTrace.cs

+    /// </summary>
+    public class PlatformEqtTrace : IPlatformEqtTrace
+    {
+        public void WriteLine(CustomTraceLevel level, string message)


We are planning to fill this in later? Looks like there might be common code with desktop for this.

AbhitejJohn · 2017-01-31T05:24:14Z

src/Microsoft.TestPlatform.CoreUtilities/Tracing/EqtTrace.cs


+        public static CustomTraceLevel TraceLevel


Isn't this a breaking change?

harshjain2 · 2017-01-31T05:28:48Z

src/Microsoft.TestPlatform.PlatformAbstractions/net46/Tracing/RollingFileTraceListener.cs

@@ -37,7 +34,7 @@ public class RollingFileTraceListener : TextWriterTraceListener
        {
            if (string.IsNullOrWhiteSpace(fileName))


Suggestion : Can use ValidateArg.NotNullOrEmpty here.

Can't use, it will create circular dependency

codito

With few suggestions

codito · 2017-02-01T02:53:47Z

@mayankbansal018 please ensure sign.proj includes the new assemblies. It will break TestPlatform.CI.Real otherwise.

…ansal018/vstest-1 into migration_netstandard1.4

Migration to netstandard 1.4

6912c6c

1. Migrating CoreUtilites to netstandard1.4 2. Adding a platform abstraction layer, to implement platform specific requirements

mayankbansal018 self-assigned this Jan 30, 2017

mayankbansal018 requested review from codito and saikrishnav January 30, 2017 15:23

mayankbansal018 added 3 commits January 30, 2017 21:04

removing additional IEqtTrace.cs

df1fb51

Throw exception if file name empty

a97035b

Modifying Sln to stop build break

bd9608c

codito reviewed Jan 31, 2017

View reviewed changes

AbhitejJohn reviewed Jan 31, 2017

View reviewed changes

harshjain2 reviewed Jan 31, 2017

View reviewed changes

codito approved these changes Feb 1, 2017

View reviewed changes

mayankbansal018 added 6 commits February 3, 2017 19:59

changes as per suggestions on PR

393abb1

Missed changes

83b5edb

merge issues

489678f

Making EqtTrace.TraceLevel available as public, & Test error fixes

3f20cce

Merge branch 'master' into migration_netstandard1.4

0b57714

Merge branch 'master' into migration_netstandard1.4

9c2e494

mayankbansal018 added 3 commits February 7, 2017 12:08

adding test for EqtTrace

61391ff

Merge branch 'migration_netstandard1.4' of https://github.com/mayankb…

b0791c5

…ansal018/vstest-1 into migration_netstandard1.4

Merge branch 'master' into migration_netstandard1.4

5d304b5

mayankbansal018 merged commit 92b930d into microsoft:master Feb 7, 2017

mayankbansal018 deleted the migration_netstandard1.4 branch August 1, 2017 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration to netstandard 1.4 #406

Migration to netstandard 1.4 #406

mayankbansal018 commented Jan 30, 2017

msftclas commented Jan 30, 2017

codito Jan 31, 2017

codito Jan 31, 2017

codito Jan 31, 2017

mayankbansal018 Jan 31, 2017

codito Jan 31, 2017

codito Jan 31, 2017

codito commented Jan 31, 2017

codito Jan 31, 2017

codito Jan 31, 2017

codito Jan 31, 2017

codito Jan 31, 2017

codito Jan 31, 2017

codito left a comment

AbhitejJohn Jan 31, 2017

AbhitejJohn Jan 31, 2017

AbhitejJohn Jan 31, 2017

harshjain2 Jan 31, 2017

mayankbansal018 Jan 31, 2017

codito left a comment

codito commented Feb 1, 2017

		@@ -37,7 +34,7 @@ public class RollingFileTraceListener : TextWriterTraceListener
		{
		if (string.IsNullOrWhiteSpace(fileName))

Migration to netstandard 1.4 #406

Migration to netstandard 1.4 #406

Conversation

mayankbansal018 commented Jan 30, 2017

msftclas commented Jan 30, 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

codito commented Jan 31, 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

codito left a 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

codito left a comment

Choose a reason for hiding this comment

codito commented Feb 1, 2017