Netcore project support #12

ManfredLange · 2017-08-06T00:44:22Z

This PR is a suggested fix for issue #11

A description of the changes:

I've added tests and implementation to account for the variations that I am aware of when settings are changed in the new csproj file format for .NET Core projects.

I've tried to keep changes to existing code to a minimum. Majority of new functionality is in a new method in VSProject.cs.

I've also added tests. One is another TestCase for an existing method. I also added two more tests with several TestCases each.

I also tested building and using the nuget package locally. I'm sure that if you decided to accept this PR that you want to test this in your environment as well.

I add file .editorconfig which is present in the main NUnit project and which I felt was missing here for consistency. VS 2017 has support for it. In it I added a rule for *.cs files to have a new line before left hand braces and a rule for all files to have a newline at the very end. The latter makes selection of text in files with the keyboard much easier. Also, since I use a different code style in other projects, adding .editorconfig made it much easier to follow the existing code style.

I updated CHANGE.txt to include a short description to reflect the changes if this fix. If you decide to merge this pull request please review the changes in this file as well in particular the release number.

Let me know if there are any oversights on my end. Constructive feedback is definitely appreciated. Thank you!

…dded two settings: a) insert_final_newline for all files and b) csharp_new_line_before_open_brace = all. 2/ Added test case for the minimal .NET Core project file that is created with the "dotnet new console" (I used version 1.0.4) 3/ Added test case for when an OutputPath is set for Release config. In the minimal project file there is no configuration, so Debug and Release exist by convention. If an OutputPath is configured, then the code needs to detect that.

…ward slashes for creating the convention based OutputPath for Debug and Release configuration if no OutputPath is set.

…embly name from the .NET Core project file if the name is specified. Updated CHANGES.txt to include these changes. Version number in CHANGES.txt needs to be reviewed in case this commit is included in a different release.

CharliePoole · 2017-08-06T01:43:15Z

CHANGES.txt

@@ -1,3 +1,12 @@
+VS Project Loader Extension 3.7 - August 6, 2017


We don't change CHANGES.txt as we go because we don't know in advance which changes will end up being in the next release or when it will be made. Instead we track the changes in GitHub and edit CHANGES.txt as a part of our release process.

Fair enough, no problem and good to know.

If we did add changes as we go, and the changes didn't end up in the next release, neither would the line in CHANGES.txt. Right? I'm thinking out loud because I was favorably impressed with this practice on another project, and if we were to document which scenarios would be affected by a change in behavior, I'm thinking this would be the way to do it: in the same PR for review.

I think it needs to be up to whomever does the releases for a particular project. When I did the releases, I treated the CHANGES.txt and corresponding release notes as a writing task, changing descriptions for clarity and adding general descriptions of the release and the main features added. Of course, this can still happen if the file is updated by the person making a change but in some ways it's cleaner if there is nothing new in the file until the release is being prepared.

Of course, this particular project has neither a project lead nor a release manager. I volunteered at one point.

Of course, this particular project has neither a project lead nor a release manager. I volunteered at one point.

Do you still volunteer? If so, let's get that in place and voted on of whatever - I'd be keen to know who's looking after the extensions. 🙂

CharliePoole · 2017-08-06T01:44:29Z

src/extension/VSProject.cs

            try
            {
                _doc = new XmlDocument();
-                _doc.Load( rdr );


Try to avoid making irrelevant changes to layout. It makes it slightly harder to review the PR because it gives us more lines of changes.

Sorry, my bad. This change was not intended.

CharliePoole · 2017-08-06T01:51:06Z

src/extension/VSProject.cs

                {
                    case ".csproj":
+                        // We try legacy project first, then new format for .NET Core projects
+                        if (!TryLoadLegacyProject())
+                            if (!TryLoadDotNetCoreProject())


Any reason for checking in this order rather than the reverse?

I.E. MSBuild followed by .NET Core

LoadMSBuildProject() throws exception if you try loading a .NET Core project file. That is what trigger this PR. I didn't wan to touch LoadMSBuildProject() so the only option left was trying the new project file format before the old format.

Ideally for more consistency all three methods trying to load a project should either return a boolean or throw an exception. I went for the option to return a boolean in line with TryLoadLegacyProject(). In general I prefer returning a value/object instead of throwing an exception if I can as it requires fewer CPU cycles (AFAIK).

CharliePoole

This looks good to me except for the bit with the failing tests in Linux and the creation of Debug and Release configs if they don't exist. Not sure why you want to do that.

CharliePoole · 2017-08-06T01:56:30Z

src/extension/VSProject.cs

+                // By convention there is a Debug and a Release configuration unless others are explicitly 
+                // mentioned in the project file. If we have less than 2 then at least one of those is missing.
+                // We cannot tell however if the existing configuration is meant to replace Debug or Release.
+                // Therefore we just add what is missing. The one that has been replaced will not be used.


I don't understand what you are doing here. The project has whatever configs it has. Why would we want to add a pretended Debug or Release? NUnit doesn't care what the names of the configs are. They are just tags. None of the other project formats do this.

A project file for .NET core, if you generate the minimum project file, does not contain any configs at all (see also file netcoreapp1.1-minimal.proj, included as test resource). So if we were looking only for configs that exist explicitly in the project file, then this would mean we wouldn't be adding any config when reading the project file. As a consequence we would not discover any tests in the project output. However. if you build the project - either Debug or Release config - it will happily produce the output, even though those configs are not listed at all. In other words Debug and Release config exist implicitly.

Therefore my suggested change is based on the observation that unless you specify some values for either Debug or Release config or you create an entirely new configuration, the default configs are Debug and Release even if one is or both are absent in the project file. And unless OutputPath has been set (which would also create the respective config explicitly in the project file), the names of the configs (implicit or explicit) plus the target framework are used to create the OutputPath, e.g. /bin/Release/netcoreapp1.1/my-assembly.dll.

Does that imply we should be doing this for some of the other project formats? If so, it could be done as a separate issue, of course.

@CharliePoole I don't think that the implicit configs are part of the other project formats, only the new csproj format. The new format has many default values that if not included in the project file get automatically added.

Well good! I wondered how come I had never known that.

Yes, the new project format makes a lot of implicit assumptions. As a result the files is much slimmer unless you start deviating from the defaults, and even then it only adds what you want to be different compared to the defaults. I think it's kind of consistent with the convention based concepts around for example MVC and other topics.

CharliePoole · 2017-08-06T01:58:29Z

src/tests/VisualStudioProjectLoaderTests.cs

+        [TestCase("netcoreapp1.1-minimal.csproj", "Debug", @"bin\Debug\netcoreapp1.1")]
+        [TestCase("netcoreapp1.1-minimal.csproj", "Release", @"bin\Release\netcoreapp1.1")]
+        [TestCase("netcoreapp1.1-with-output-path.csproj", "Debug", @"bin\Debug\netcoreapp1.1")]
+        [TestCase("netcoreapp1.1-with-output-path.csproj", "Release", @"bin\Release\special")]


These will fail on Linux due to the backwards slash.

I'll change that so that if BasePath contains backslashes they will be replaced with forward slashes. Also I'll change the TestCase data to use forward slashes. This may also fix the failing four tests on Travis-CI for mono target.

CharliePoole · 2017-08-06T02:05:11Z

Hi Manfred! Haven't talked to you for a long time. Welcome here.

I reviewed your code and asked for some changes or perhaps an explanation of what you are doing in one case. The comments I made about changing formatting, etc. don't need to be dealt with. They are just info for the future. I like how you kept the .NET Core stuff separate.

ManfredLange · 2017-08-06T04:45:29Z

@CharliePoole Thank you for your feedback and comments. I've provided further info, hoping it will help with understanding why I did things a particular way.

Yes, indeed, we haven't spoken in a while. But perhaps we'll meet again eventually on some conference. Or if you happen to come to New Zealand, let me know. :-)

…e to address difference between Linux and Windows which use forward slashes and backwards slashes respectively.

ManfredLange · 2017-08-06T04:53:39Z

@CharliePoole I just pushed another commit that addresses the backslash issue. I don't have a Linux environment to test, though, so it might be that my change may not work on Linux.

ManfredLange · 2017-08-06T04:58:29Z

With these most recent changes the tests are now also passing on Linux and MacOS (see Travis CI).

CharliePoole · 2017-08-07T09:27:57Z

We usually require a second review, but I'm going to go ahead and merge this. It makes no changes in our existing support, enables reading the .NET Core project format is Manfred is known to me.

Note that this does nothing to make the engine actually recognize and run .NET Core assemblies. It will now give a slightly better error message, indicating that the assembly is not supported, but incorrectly calling it "Portable."

pavzaj · 2017-10-06T15:15:05Z

When do you plan to release this?
It works for new .csprojs, but it does not work with multitargetes projects.
For multitargeted .csproj I get an error:
`NUnit Console Runner 3.7.0
Copyright (c) 2017 Charlie Poole, Rob Prouse

Runtime Environment
OS Version: Microsoft Windows NT 6.1.7601 Service Pack 1
CLR Version: 4.0.30319.42000

Installed Extensions
Extension Point: /NUnit/Engine/NUnitV2Driver
Extension Point: /NUnit/Engine/TypeExtensions/IService
Extension Point: /NUnit/Engine/TypeExtensions/ITestEventListener
Extension: NUnit.Engine.Listeners.TeamCityEventListener
Extension Point: /NUnit/Engine/TypeExtensions/IDriverFactory
Extension Point: /NUnit/Engine/TypeExtensions/IProjectLoader
Extension: NUnit.Engine.Services.ProjectLoaders.VisualStudioProjectLoader
FileExtension: .sln .csproj .vbproj .vjsproj .vcproj .fsproj
Extension Point: /NUnit/Engine/TypeExtensions/IResultWriter

System.ArgumentException: Invalid project file format: NLog.Extensions.Logging.Tests.csproj ---> System.NullReferenceException: Object reference not set to an instance of an object.
at NUnit.Engine.Services.ProjectLoaders.VSProject.TryLoadDotNetCoreProject() in C:\source\vs-project-loader-master\src\extension\VSProject.cs:line 247
at NUnit.Engine.Services.ProjectLoaders.VSProject.Load() in C:\source\vs-project-loader-master\src\extension\VSProject.cs:line 203
--- End of inner exception stack trace ---
at NUnit.Engine.Services.ProjectLoaders.VSProject.ThrowInvalidFormat(String projectPath, Exception e) in C:\source\vs-project-loader-master\src\extension\VSProject.cs:line 444
at NUnit.Engine.Services.ProjectLoaders.VSProject.Load() in C:\source\vs-project-loader-master\src\extension\VSProject.cs:line 229
at NUnit.Engine.Services.ProjectLoaders.VisualStudioProjectLoader.LoadFrom(String path) in C:\source\vs-project-loader-master\src\extension\VisualStudioProjectLoader.cs:line 53
at NUnit.Engine.Services.ProjectService.ExpandProjectPackage(TestPackage package)
at NUnit.Engine.Runners.MasterTestRunner.ExpandProjects(TestPackage package)
at NUnit.Engine.Runners.MasterTestRunner.EnsurePackagesAreExpanded(TestPackage package)
at NUnit.Engine.Runners.MasterTestRunner.EnsurePackagesAreExpanded(TestPackage package)
at NUnit.Engine.Runners.MasterTestRunner.InitializePackage()
at NUnit.Engine.TestEngine.GetRunner(TestPackage package)
at NUnit.ConsoleRunner.ConsoleRunner.RunTests(TestPackage package, TestFilter filter)
at NUnit.ConsoleRunner.Program.Main(String[] args)
`

Manfred Lange added 3 commits August 3, 2017 22:25

Added 2 more tests to ensure that backslashes are used instead of for…

82d4dff

…ward slashes for creating the convention based OutputPath for Debug and Release configuration if no OutputPath is set.

CharliePoole reviewed Aug 6, 2017

View reviewed changes

CharliePoole suggested changes Aug 6, 2017

View reviewed changes

Changed paths for four testcases to use forward slashes and added cod…

16d9eee

…e to address difference between Linux and Windows which use forward slashes and backwards slashes respectively.

CharliePoole approved these changes Aug 6, 2017

View reviewed changes

CharliePoole mentioned this pull request Aug 7, 2017

NetStandard Null ReferenceExcpetion nunit/nunit-console#277

Closed

CharliePoole merged commit 91f0294 into nunit:master Aug 7, 2017

CharliePoole mentioned this pull request Aug 7, 2017

Invalid Project Format Exception if Project is new project file format #11

Closed

ManfredLange deleted the netcore-project-support branch August 8, 2017 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Netcore project support #12

Netcore project support #12

ManfredLange commented Aug 6, 2017

CharliePoole Aug 6, 2017

ManfredLange Aug 6, 2017

jnm2 Oct 10, 2017 •

edited

Loading

CharliePoole Oct 10, 2017

ChrisMaddock Oct 10, 2017 •

edited

Loading

CharliePoole Aug 6, 2017

ManfredLange Aug 6, 2017 •

edited

Loading

CharliePoole Aug 6, 2017

CharliePoole Aug 6, 2017

ManfredLange Aug 6, 2017 •

edited

Loading

CharliePoole left a comment

CharliePoole Aug 6, 2017

ManfredLange Aug 6, 2017 •

edited

Loading

CharliePoole Aug 6, 2017

rprouse Aug 8, 2017

CharliePoole Aug 8, 2017

ManfredLange Aug 8, 2017

CharliePoole Aug 6, 2017

ManfredLange Aug 6, 2017 •

edited

Loading

CharliePoole commented Aug 6, 2017

ManfredLange commented Aug 6, 2017 •

edited

Loading

ManfredLange commented Aug 6, 2017 •

edited

Loading

ManfredLange commented Aug 6, 2017

CharliePoole commented Aug 7, 2017

pavzaj commented Oct 6, 2017 •

edited

Loading

		@@ -1,3 +1,12 @@
		VS Project Loader Extension 3.7 - August 6, 2017

Netcore project support #12

Netcore project support #12

Conversation

ManfredLange commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMaddock Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManfredLange Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManfredLange Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

CharliePoole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManfredLange Aug 6, 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

ManfredLange Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

CharliePoole commented Aug 6, 2017

ManfredLange commented Aug 6, 2017 • edited Loading

ManfredLange commented Aug 6, 2017 • edited Loading

ManfredLange commented Aug 6, 2017

CharliePoole commented Aug 7, 2017

pavzaj commented Oct 6, 2017 • edited Loading

jnm2 Oct 10, 2017 •

edited

Loading

ChrisMaddock Oct 10, 2017 •

edited

Loading

ManfredLange Aug 6, 2017 •

edited

Loading

ManfredLange Aug 6, 2017 •

edited

Loading

ManfredLange Aug 6, 2017 •

edited

Loading

ManfredLange Aug 6, 2017 •

edited

Loading

ManfredLange commented Aug 6, 2017 •

edited

Loading

ManfredLange commented Aug 6, 2017 •

edited

Loading

pavzaj commented Oct 6, 2017 •

edited

Loading