Lightweight .NET Standard version of the Engine #205

rprouse · 2017-03-30T22:10:53Z

I would like to start reviewing this approach, but ~~we should not merge until end-to end testing with the adapter has been performed.~~ I will add some comments inline with the code as to some of the decisions that I made, but the overall approach is,

We need versions of the VS Adapter that targets the full CLR, .NET Core and UWP to run tests for those platforms in Visual Studio and with the newer version of the dotnet test command. Our adapter uses the engine in-process and without many of the extensions, so that is what I am targeting. At first, I attempted to use the nunit.portable.agent project, but the majority of that code was unused and I was pulling in too much code from this project, so I switched back to creating a .NET Standard version of the engine.

This version of the engine is only intended to be used in-process. If we support running .NET Core tests from our current desktop console runner, it will launch a .NET Core agent that uses this engine, but the console runner will not.
I decided to make it a different NuGet package to differentiate the purpose
This does not support extensions. There is very little support in .NET Standard for loading assemblies from other directories which makes the plugin model difficult.
There is no support for AppDomains or separate processes
This only supports classes needed for the Visual Studio adapter. We can add more as needed, but I think we should keep it lean

For now, I plan on using CI builds of the NuGet package in the adapter, then release an alpha version of this and the adapter so we can get people in the community who are using .NET Core to test and give feedback.

Fixes #10

…he mock assembly.

…NET Standard

… files

rprouse · 2017-03-30T22:17:31Z

src/NUnitEngine/nunit.engine.api/TestPackage.cs

@@ -49,7 +51,14 @@ public TestPackage(string filePath)

            if (filePath != null)
            {
+#if NETSTANDARD1_3
+                if (!Path.IsPathRooted(filePath))


This is one major difference between the full engine and this version. The paths in .NET Standard vary dramatically depending on how you run the project. For example, it you use dotnet run, it compiles a project and runs it. The assembly is in a sub-directory, but the current directory is the location of the project file. If you use dotnet execute the path is that of the assembly. Because of this, our code that figures out the full path does not work in all cases, so I decided to only allow full paths and leave it up to the calling runner to resolve that path.

rprouse · 2017-03-30T22:18:51Z

src/NUnitEngine/nunit.engine.netstandard/Runners/DirectTestRunner.cs

+    /// DirectTestRunner is the abstract base for runners 
+    /// that deal directly with a framework driver.
+    /// </summary>
+    public class DirectTestRunner : AbstractTestRunner


Any engine files that were dramatically different I created a new copy for the .NET Standard engine. They were getting way too complicated with the #if statements.

I wonder if the parts in common could be abstracted to a base class as well, at some point, to help manage the complexity? Things like NUnitNetStandardDriverTests could then be consolidated and test all the drivers at once via a common interface at least.

rprouse · 2017-03-30T22:20:18Z

src/NUnitEngine/nunit.engine.tests/Internal/PathUtilTests.cs


-		[Test]
+        [Test]


Looks like we have some old mixed tabs and spaces in this file.

rprouse · 2017-03-30T22:20:55Z

src/NUnitEngine/nunit.engine.tests/Internal/PathUtilTests.cs

@@ -86,7 +86,8 @@ public void Canonicalize()
 				PathUtils.Canonicalize( @"folder1\folder2\..\..\..\file.tmp" ) );
 		}

-		[Test]
+#if !NETCOREAPP1_1


Tests are all a .NET Core app because they are self-executing NUnitLite.

rprouse · 2017-03-30T22:21:57Z

src/NUnitEngine/nunit.engine/Internal/SettingsStore.cs


                foreach (XmlElement element in doc.DocumentElement["Settings"].ChildNodes)
                {
                    if (element.Name != "Setting")
-                        throw new ApplicationException("Unknown element in settings file: " + element.Name);
+                        throw new Exception("Unknown element in settings file: " + element.Name);


ApplicationException is not available 😦

rprouse · 2017-03-30T22:23:25Z

src/NUnitEngine/nunit.engine/Services/ResultService.cs

+#if NETSTANDARD1_3
+                default:
+                    return null;
+#else
                case "user":
                    return new XmlTransformResultWriter(args);


None of the XML transform classes we use are available and they aren't used in the VS Adapter.

rprouse · 2017-03-30T22:25:00Z

build.ps1

@@ -89,7 +89,7 @@ if(!$PSScriptRoot){
 $TOOLS_DIR = Join-Path $PSScriptRoot "tools"
 $NUGET_EXE = Join-Path $TOOLS_DIR "nuget.exe"
 $CAKE_EXE = Join-Path $TOOLS_DIR "Cake/Cake.exe"
-$NUGET_URL = "https://dist.nuget.org/win-x86-commandline/latest/nuget.exe"
+$NUGET_URL = "https://dist.nuget.org/win-x86-commandline/v4.0.0/nuget.exe"


The .NET Core restore needs the 4.0 version of NuGet, but latest is currently pointing at 3.5. Once latest is updated, this can be reverted.

I ran into this as well. An alternative which I would recommend is to leave the bootstrapper alone and replace nuget restore with msbuild /t:restore.
See https://gitter.im/cake-build/cake?at=58d156017b3f37e7541e55b0 and http://blog.nuget.org/20170316/NuGet-now-fully-integrated-into-MSBuild.html

Switching to MSBuild would require v15 which would force all team members and build machines to use Visual Studio 2017.

rprouse · 2017-03-30T22:28:47Z

I can't make much of the build failures on Travis. If anyone has ideas, I would appreciate the help, otherwise I may just not build .NET Core on Linux/Mac 😄

Could be that Travis hasn't updated .NET core, but it is showing as not installed, so it shouldn't be building and shouldn't be giving MSBuild errors. Possibly because I am using the newer DotNetCore cake tasks? Maybe we need to run --experimental on Travis?

ghost · 2017-03-31T15:09:02Z

@rprouse - is this any help?

https://github.com/dotnet/docs/blob/master/docs/core/tools/dotnet-install-script.md

You can install the runtime by way of a bash script.

rprouse · 2017-03-31T20:09:18Z

@Fir3pho3nixx thanks for that, it might be helpful.

ghost · 2017-04-01T11:19:01Z

Figured this out, the main battle here is figuring out how to get libunwind working on travis using ubuntu trusty.

This is my travis yaml file that worked.

language: csharp

solution: fortress.sln

mono: none

dotnet: none

sudo: required

dist: trusty

before_install:  

    - sudo apt-get -qq update
    
    - sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
  
    - sudo apt-get install -y libunwind8

    - wget -O - https://raw.githubusercontent.com/dotnet/cli/rel/1.0.0/scripts/obtain/dotnet-install.sh | bash -s -- -i ./dotnetcli/

script:

    - ./dotnetcli/dotnet restore ./src/Fortress.Windsor/Fortress.Windsor.csproj
    
    - ./dotnetcli/dotnet restore ./src/Fortress.Facilities.Logging/Fortress.Facilities.Logging.csproj
    
    - ./dotnetcli/dotnet restore ./src/Fortress.Windsor.Tests/Fortress.Windsor.Tests.csproj
    
    - ./dotnetcli/dotnet test ./src/Fortress.Windsor.Tests/Fortress.Windsor.Tests.csproj

rprouse · 2017-04-19T02:47:48Z

I am going to disable building .NET Core on Travis, but in the meantime, this is ready for review. It creates a .NET Standard version of the engine with the limitations mentioned above. It is working well in the Visual Studio adapter to run tests in Visual Studio and on the command line with vstest and dotnet test.

My thinking is to release an alpha version of the .NET Standard engine to NuGet and consume it in an alpha release of the adapter.

rprouse · 2017-04-19T16:41:39Z

@nunit/engine-team I cannot seem to get Travis building for the life of me. It is failing in the NuGet restore of the main project for some Travis builds. The nuget restore hasn't changed and the main solution hasn't changed. The only difference that I can think of is that there are project.json files in some of the projects. That is no different than the nunit framework repo though.

I turned up the verbosity of the nuget restore and it is showing the error

NuGet.CommandLine.CommandLineException: 
  at NuGet.CommandLine.MsBuildUtility+<GetProjectReferencesAsync>d__5.MoveNext () <0x40fa8830 + 0x0125b> in <filename unknown>:0

The successful Travis build is finding MSBuild 15, but this is finding MSBuild 14. My Windows box finds 14 and succeeds though.

Any help or ideas would be appreciated.

rprouse · 2017-04-22T13:56:08Z

I've managed to get this building on my Linux box. At this point, it doesn't look like it will require any code changes or changes to build.cake, it will only be changes to the Travis environment to ensure the correct tools are installed on Linux and Mac.

Based on that, this is ready to review. I would like feedback from the @nunit/core-team and @nunit/engine-team on the approach. I would also like to do an alpha release of just the .NET Core engine soon.

@CharliePoole, you also had concerns about calling this an engine. I would appreciate it if you could take a look and see if you still feel that way or possibly suggest better naming to prevent confusion.

One other major question is the fact that I copied and rewrote several files rather than use too many #if blocks. I worry that decision might cause maintenance problems in the future. Most of the files are shared between the two solutions, but the following files are copies,

TestEngine.cs
DirectTestRunner.cs
MasterTestRunner.cs
DefaultTestRunnerFactory.cs
DriverService.cs

rprouse · 2017-04-25T00:46:32Z

CI is now passing on this PR. The Linux Mono 4.8.1 failure is unrelated, see #211.

This can be reviewed and merged when ready.

ChrisMaddock

This looks good! It's a lot of code to review as one, but there's nothing glaringly obvious to me which seems out of place.

I agree the duplication is a concern - but I'd personally prioritise getting an alpha version out there with a view to deal with this later. What's the long term plan - I presume it's not a separate mini-engine?

ChrisMaddock · 2017-04-26T17:37:38Z

src/NUnitEngine/nunit.engine.netstandard/Services/DefaultTestRunnerFactory.cs

+
+            // TODO: What about bad extensions?
+            return new DirectTestRunner(package);
+        }


This method seems a little odd now with all the project stuff stripped out. Return an AggregatingTestRunner if there are 'any' assemblies, and a DirectTestRunner only if there are zero? If extensions aren't supported, shouldn't everything be an assembly, and anything else invalid?

Agreed, but I didn't want to strip out too much that we might end up re-adding. We don't need NUnit project loading or solution loading for the VS Adapter, but I thought that once we move to other runners like Xamarin or UWP, we might end up adding some of the extensions back in.

ChrisMaddock · 2017-04-26T17:44:17Z

src/NUnitEngine/nunit.engine.tests.netstandard/API/TestPackageTests.cs

+        }
+
+        [Test]
+        public void AssemblyPathIsUsedAsFilePath()


Should this be AssemblyPathIsUsedAsFullName?

Yes, fixing.

rprouse · 2017-04-28T02:40:38Z

@ChrisMaddock thanks for the review, I have pushed an update for one of your comments.

@jnm2 I would appreciate a second code review on this before I merge if you have some time.

@nunit/core-team I plan on putting out an alpha1 release of this once it is merged so that it can be put into the adapter and we can get an alpha1 release of that out too. I was planning on calling this 3.7.0-alpha1 in prep for the final 3.7 release. I was planning on calling the adapter 4.0.0-alpha1 because it is a major change in functionality.

Does anyone see any problem with that plan or does anyone have suggestions?

jnm2

This is an impressive amount of work and I can't claim to completely understand it, but I can't wait to have an excuse to try it out!

jnm2 · 2017-04-28T11:45:20Z

src/NUnitEngine/mock-assembly/mock-assembly.netstandard.csproj

+    <GenerateAssemblyInformationalVersionAttribute>false</GenerateAssemblyInformationalVersionAttribute>
+    <GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute>
+    <GenerateAssemblyTitleAttribute>false</GenerateAssemblyTitleAttribute>
+    <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>


These 9 lines can be replaced with <GenerateAssemblyInfo>false</GenerateAssemblyInfo>

That cleans things up, thanks. Do you know where all of the new options are documented? I've just been looking through examples of Microsoft projects.

jnm2 · 2017-04-28T11:45:52Z

src/NUnitEngine/mock-assembly/mock-assembly.netstandard.csproj

+  </PropertyGroup>
+  <PropertyGroup>
+    <AssemblyOriginatorKeyFile>..\..\nunit.snk</AssemblyOriginatorKeyFile>
+    <GeneratePackageOnBuild>False</GeneratePackageOnBuild>


Off the top of my head I think this is the default and this line could be left out?

jnm2 · 2017-04-28T11:50:42Z

src/NUnitEngine/mock-assembly/mock-assembly.netstandard.csproj

+    <RootNamespace>NUnit.Tests</RootNamespace>
+    <TargetFramework>netstandard1.6</TargetFramework>
+    <AssemblyName>mock-assembly</AssemblyName>
+    <IntermediateOutputPath>obj\$(Configuration)\netstandard\</IntermediateOutputPath>


Pretty sure this is the default and this line could be left out?

jnm2 · 2017-04-28T11:52:20Z

src/NUnitEngine/nunit.engine.api/Extensibility/IFrameworkDriver.cs

@@ -39,7 +40,7 @@ public interface IFrameworkDriver
        /// used to ensure that test ids are unique across drivers.
        /// </summary>
        string ID { get; set; }
-
+        


Spaces are such a giant pain. I have been enjoying https://marketplace.visualstudio.com/items?itemName=MadsKristensen.TrailingWhitespaceVisualizer. When you save the file it helps by stripping all trailing whitespace.

Today I also discovered a VS Code extension for this too (https://marketplace.visualstudio.com/items?itemName=shardulm94.trailing-spaces) which is totally awesome. I enabled trim on save.

jnm2 · 2017-04-28T11:57:53Z

src/NUnitEngine/nunit.engine.netstandard/Runners/DirectTestRunner.cs

+    /// DirectTestRunner is the abstract base for runners 
+    /// that deal directly with a framework driver.
+    /// </summary>
+    public class DirectTestRunner : AbstractTestRunner


I wonder if the parts in common could be abstracted to a base class as well, at some point, to help manage the complexity? Things like NUnitNetStandardDriverTests could then be consolidated and test all the drivers at once via a common interface at least.

jnm2 · 2017-04-28T12:02:23Z

src/NUnitEngine/nunit.engine.netstandard/nunit.engine.netstandard.csproj

+    <Compile Include="..\nunit.engine\Services\TestFilterBuilder.cs" Link="Services\TestFilterBuilder.cs" />
+    <Compile Include="..\nunit.engine\Services\TestFilterService.cs" Link="Services\TestFilterService.cs" />
+    <Compile Include="..\nunit.engine\Services\TestSelectionParser.cs" Link="Services\TestSelectionParser.cs" />
+    <Compile Include="..\nunit.engine\Services\Tokenizer.cs" Link="Services\Tokenizer.cs" />


Seems like a glob could take care of almost all these items linked from the ..\nunit.engine folder. That's one less csproj to constantly be touching. :D (I can not wait until we do this with all the projects!)

Possibly, but this is just a fraction of the files in the Engine and API projects so I would have to add #if !NETSTANDARD1_3 to all of those. That may happen when/if the projects ever merge, but for now I wanted to keep this as unobtrusive as possible.

Oh, good strategy.

jnm2 · 2017-04-28T12:08:00Z

src/NUnitEngine/nunit.engine/Services/ResultWriters/TestCaseResultWriter.cs

            {
                // Opening is enough to check
            }
        }

        public void WriteResultFile(XmlNode resultNode, string outputPath)
        {
-            using (var writer = new StreamWriter(outputPath, false, Encoding.UTF8))
+            using (var stream = new FileStream(outputPath, FileMode.Create, FileAccess.Write))
+            using (var writer = new StreamWriter(stream))


This is better because it no longer writes a byte order mark.

jnm2 · 2017-04-28T12:08:51Z

src/NUnitEngine/nunit.engine.tests.netstandard/nunit.engine.tests.netstandard.csproj

+    <TargetFramework>netcoreapp1.1</TargetFramework>
+    <SignAssembly>True</SignAssembly>
+    <AssemblyOriginatorKeyFile>..\..\nunit.snk</AssemblyOriginatorKeyFile>
+    <DelaySign>False</DelaySign>


Pretty sure this is the default and this line could be left out?

jnm2 · 2017-04-28T12:09:49Z

src/NUnitEngine/nunit.engine.tests.netstandard/nunit.engine.tests.netstandard.csproj

+    <GenerateAssemblyInformationalVersionAttribute>false</GenerateAssemblyInformationalVersionAttribute>
+    <GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute>
+    <GenerateAssemblyTitleAttribute>false</GenerateAssemblyTitleAttribute>
+    <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>


Same [These 9 lines can be replaced with <GenerateAssemblyInfo>false</GenerateAssemblyInfo>]

jnm2 · 2017-04-28T12:10:56Z

src/NUnitEngine/nunit.engine.tests.netstandard/nunit.engine.tests.netstandard.csproj

+    <Compile Include="..\nunit.engine.tests\Services\TestFilterBuilderTests.cs" Link="Services\TestFilterBuilderTests.cs" />
+    <Compile Include="..\nunit.engine.tests\Services\TestFilteringTests.cs" Link="Services\TestFilteringTests.cs" />
+    <Compile Include="..\nunit.engine.tests\Services\TestSelectionParserTests.cs" Link="Services\TestSelectionParserTests.cs" />
+    <Compile Include="..\nunit.engine.tests\Services\TokenizerTests.cs" Link="Services\TokenizerTests.cs" />


Same [Seems like a glob could take care of almost all these items linked from the ..\nunit.engine folder.]

Same, there are 53 CS files for the main test suite, we are only pulling in 14.

rprouse

@jnm2 thanks for the review. I am making most of your project file change suggestions. Anything to clean up the files and make them easier to maintain is a win in my books 👍

rprouse · 2017-04-28T18:18:51Z

src/NUnitEngine/nunit.engine.netstandard/nunit.engine.netstandard.csproj

+    <Compile Include="..\nunit.engine\Services\TestFilterBuilder.cs" Link="Services\TestFilterBuilder.cs" />
+    <Compile Include="..\nunit.engine\Services\TestFilterService.cs" Link="Services\TestFilterService.cs" />
+    <Compile Include="..\nunit.engine\Services\TestSelectionParser.cs" Link="Services\TestSelectionParser.cs" />
+    <Compile Include="..\nunit.engine\Services\Tokenizer.cs" Link="Services\Tokenizer.cs" />


Possibly, but this is just a fraction of the files in the Engine and API projects so I would have to add #if !NETSTANDARD1_3 to all of those. That may happen when/if the projects ever merge, but for now I wanted to keep this as unobtrusive as possible.

rprouse · 2017-04-28T18:22:25Z

src/NUnitEngine/nunit.engine.tests.netstandard/nunit.engine.tests.netstandard.csproj

+    <Compile Include="..\nunit.engine.tests\Services\TestFilterBuilderTests.cs" Link="Services\TestFilterBuilderTests.cs" />
+    <Compile Include="..\nunit.engine.tests\Services\TestFilteringTests.cs" Link="Services\TestFilteringTests.cs" />
+    <Compile Include="..\nunit.engine.tests\Services\TestSelectionParserTests.cs" Link="Services\TestSelectionParserTests.cs" />
+    <Compile Include="..\nunit.engine.tests\Services\TokenizerTests.cs" Link="Services\TokenizerTests.cs" />


Same, there are 53 CS files for the main test suite, we are only pulling in 14.

rprouse · 2017-04-28T18:26:05Z

src/NUnitEngine/mock-assembly/mock-assembly.netstandard.csproj

+    <GenerateAssemblyInformationalVersionAttribute>false</GenerateAssemblyInformationalVersionAttribute>
+    <GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute>
+    <GenerateAssemblyTitleAttribute>false</GenerateAssemblyTitleAttribute>
+    <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>


That cleans things up, thanks. Do you know where all of the new options are documented? I've just been looking through examples of Microsoft projects.

jnm2 · 2017-04-28T20:48:42Z

@rprouse

That cleans things up, thanks. Do you know where all of the new options are documented? I've just been looking through examples of Microsoft projects.

Tell me about it. 😄
I still can't locate documentation for GenerateAssemblyInfo. Got it off dotnet/project-system.

I've used these recently:

https://docs.microsoft.com/en-us/dotnet/articles/core/tools/csproj
https://docs.microsoft.com/en-us/dotnet/articles/core/tools/project-json-to-csproj
http://www.natemcmaster.com/blog/2017/03/09/vs2015-to-vs2017-upgrade/
https://docs.microsoft.com/en-us/dotnet/articles/core/migration/

rprouse added 18 commits March 18, 2017 13:35

.NET Standard version of the Mock Assembly

ef452dc

Added the API classes that are needed to the .NET Standard project

7873eba

Added empty test project for .NET Standard

7163824

Initial .NET Standard unit tests

428f9ef

.NET Standard version of the engine is compiling

a721659

Nearly all tests passing, but having trouble with the full path for t…

51d56c6

…he mock assembly.

Switched Mock Assembly to the newer CSPROJ format

0bcc9ff

Converted the .NET Standard engine to the new csproj format

959231f

Created a VS2017 solution

9326824

Fixing the .NET Standard unit tests

89c673f

Remove the parallel task worker pool because they are never used in .…

a4a60f7

…NET Standard

Added in the result writers

6d8c2e2

Adding in addtional classes needed by the VS Adapter

dbe77ec

Switched to just a .NET Core solution

eb7de68

Build, tests and package .NET Standard from Cake

9214f29

Updating Cake to work on AppVeyor

8f51abc

Need Nuget 4.0, but latest is still 3.5

59276c2

Moved the various assembly info back to the CS files from the project…

8441cc6

… files

rprouse commented Mar 30, 2017

View reviewed changes

rprouse added 3 commits April 1, 2017 09:12

Install .NET Core on Travis

cbb9d72

Another Travis attempt

e684e71

Execute permissions on Travis

56fb105

rprouse added 3 commits April 19, 2017 09:45

Don't build the .NET Standard engine on Travis

35fe556

Another attempt at getting all Travis builds running

0c7ba4e

Yet another travis attempt

06533ce

rprouse added 3 commits April 24, 2017 19:40

Using the Travis setup from OmniSharp

a000037

Now building on Linux

4a2b088

Remove travis msbuild update

f15bbe1

ChrisMaddock approved these changes Apr 26, 2017

View reviewed changes

Fixes based on code review feedback

90bddf0

Reverting one of the dependency changes

5ee708f

jnm2 approved these changes Apr 28, 2017

View reviewed changes

rprouse commented Apr 28, 2017

View reviewed changes

Update based on code review

ba194ae

rprouse merged commit 3f71e38 into master Apr 28, 2017

rprouse deleted the issue-10d branch April 28, 2017 20:05

yaakov-h mentioned this pull request Apr 29, 2017

Create a .NET Standard version of the Engine #10

Closed

Lightweight .NET Standard version of the Engine #205

Lightweight .NET Standard version of the Engine #205

Conversation

rprouse commented Mar 30, 2017 • edited Loading

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

rprouse commented Mar 30, 2017

ghost commented Mar 31, 2017

rprouse commented Mar 31, 2017

ghost commented Apr 1, 2017

rprouse commented Apr 19, 2017

rprouse commented Apr 19, 2017

rprouse commented Apr 22, 2017

rprouse commented Apr 25, 2017

ChrisMaddock 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

rprouse commented Apr 28, 2017

jnm2 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

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

rprouse 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

jnm2 commented Apr 28, 2017

rprouse commented Mar 30, 2017 •

edited

Loading