-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Netcore project support #12
Changes from 3 commits
f576f28
82d4dff
a72914a
16d9eee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
# EditorConfig helps developers define and | ||
# maintain consistent coding styles between | ||
# different editors and IDEs | ||
|
||
# http://EditorConfig.org | ||
|
||
# top-most EditorConfig file | ||
root = true | ||
|
||
[*] | ||
indent_style = space | ||
indent_size = 4 | ||
end_of_line = crlf | ||
# Make selection with keyboard easier in all files | ||
insert_final_newline = true | ||
|
||
[*.proj] | ||
indent_size = 2 | ||
|
||
[*.csproj] | ||
indent_size = 2 | ||
|
||
[*.vcxproj] | ||
indent_size = 2 | ||
|
||
[*.xproj] | ||
indent_size = 2 | ||
|
||
[*.json] | ||
indent_size = 2 | ||
|
||
[*.config] | ||
indent_size = 2 | ||
|
||
[*.nuspec] | ||
indent_size = 2 | ||
|
||
[*.xml] | ||
indent_size = 2 | ||
|
||
[*.cs] | ||
csharp_new_line_before_open_brace = all |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -187,17 +187,23 @@ private void Load() | |
ThrowInvalidFileType( ProjectPath ); | ||
|
||
StreamReader rdr = new StreamReader(ProjectPath, System.Text.Encoding.UTF8); | ||
|
||
try | ||
{ | ||
_doc = new XmlDocument(); | ||
_doc.Load( rdr ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, my bad. This change was not intended. |
||
_doc.Load(rdr); | ||
|
||
string extension = Path.GetExtension( ProjectPath ); | ||
string extension = Path.GetExtension(ProjectPath); | ||
|
||
switch ( extension ) | ||
switch (extension) | ||
{ | ||
case ".csproj": | ||
// We try legacy project first, then new format for .NET Core projects | ||
if (!TryLoadLegacyProject()) | ||
if (!TryLoadDotNetCoreProject()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason for checking in this order rather than the reverse? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I.E. MSBuild followed by .NET Core There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
LoadMSBuildProject(); | ||
break; | ||
|
||
case ".vbproj": | ||
case ".vjsproj": | ||
case ".fsproj": | ||
|
@@ -228,6 +234,74 @@ private void Load() | |
} | ||
} | ||
|
||
/// <summary> | ||
/// Load a project in the new project format for .NET Core 1.0/1.1. Note that this method | ||
/// is only called for file extensions .csproj. | ||
/// </summary> | ||
/// <returns>True if the project was successfully loaded, false otherwise.</returns> | ||
private bool TryLoadDotNetCoreProject() { | ||
XmlNode root = _doc.SelectSingleNode("Project"); | ||
|
||
if (root != null && SafeAttributeValue(root, "Sdk") != null) | ||
{ | ||
string targetFramework = _doc.SelectSingleNode("Project/PropertyGroup/TargetFramework").InnerText; | ||
|
||
XmlNode assemblyNameNode = _doc.SelectSingleNode("Project/PropertyGroup/AssemblyName"); | ||
// Even console apps are dll's even if <OutputType> has value 'EXE' | ||
string assemblyName = assemblyNameNode == null ? $"{Name}.dll" : $"{assemblyNameNode.InnerText}.dll"; | ||
|
||
XmlNodeList nodes = _doc.SelectNodes("/Project/PropertyGroup"); | ||
|
||
string commonOutputPath = null; | ||
|
||
foreach (XmlElement configNode in nodes) | ||
{ | ||
string configName = GetConfigNameFromCondition(configNode); | ||
|
||
XmlElement outputPathElement = (XmlElement)configNode.SelectSingleNode("OutputPath"); | ||
string outputPath = null; | ||
if (outputPathElement != null) | ||
outputPath = outputPathElement.InnerText; | ||
|
||
if (configName == null) | ||
{ | ||
commonOutputPath = outputPath; | ||
continue; | ||
} | ||
|
||
if (outputPath == null) | ||
outputPath = commonOutputPath; | ||
|
||
if (outputPath != null) | ||
_configs.Add(configName, new ProjectConfig(this, configName, outputPath, assemblyName)); | ||
} | ||
|
||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CharliePoole I don't think that the implicit configs are part of the other project formats, only the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well good! I wondered how come I had never known that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (_configs.Count < 2) | ||
{ | ||
if (!_configs.ContainsKey("Debug")) | ||
{ | ||
string configName = "Debug"; | ||
string outputPath = $@"bin\{configName}\{targetFramework}"; | ||
_configs.Add(configName, new ProjectConfig(this, configName, outputPath, assemblyName)); | ||
} | ||
if (!_configs.ContainsKey("Release")) | ||
{ | ||
string configName = "Release"; | ||
string outputPath = $@"bin\{configName}\{targetFramework}"; | ||
_configs.Add(configName, new ProjectConfig(this, configName, outputPath, assemblyName)); | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/// <summary> | ||
/// Load a project in the legacy VS2003 format. Note that this method is not | ||
/// called for C++ projects using the same format, because the details differ. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
|
||
using System; | ||
using System.IO; | ||
using System.Linq; | ||
using NUnit.Engine.Extensibility; | ||
using NUnit.Engine.Tests.resources; | ||
using NUnit.Framework; | ||
|
@@ -119,6 +120,7 @@ public void CannotLoadWebProject() | |
[TestCase("legacy-cpp-sample.vcproj", new string[] { "Debug|Win32", "Release|Win32" }, "cpp-sample")] | ||
[TestCase("legacy-cpp-library-with-macros.vcproj", new string[] { "Debug|Win32", "Release|Win32" }, "legacy-cpp-library-with-macros")] | ||
[TestCase("legacy-cpp-makefile-project.vcproj", new string[] { "Debug|Win32", "Release|Win32" }, "MakeFileProject")] | ||
[TestCase("netcoreapp1.1-minimal.csproj", new string[] { "Debug", "Release" }, "netcoreapp1.1-minimal")] | ||
public void CanLoadVsProject(string resourceName, string[] configs, string assemblyName) | ||
{ | ||
Assert.That(_loader.CanLoadFrom(resourceName)); | ||
|
@@ -142,6 +144,38 @@ public void CanLoadVsProject(string resourceName, string[] configs, string assem | |
} | ||
} | ||
|
||
[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")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These will fail on Linux due to the backwards slash. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
public void PicksUpCorrectOutputPath(string resourceName, string configuration, string expectedOutputPath) | ||
{ | ||
using (TestResource file = new TestResource(resourceName)) | ||
{ | ||
IProject project = _loader.LoadFrom(file.Path); | ||
|
||
var package = project.GetTestPackage(configuration); | ||
Assert.That(package.Settings["BasePath"].ToString().EndsWith(expectedOutputPath)); | ||
} | ||
} | ||
|
||
[TestCase("netcoreapp1.1-minimal.csproj", "netcoreapp1.1-minimal")] | ||
[TestCase("netcoreapp1.1-with-assembly-name.csproj", "the-assembly-name")] | ||
public void PicksUpCorrectAssemplyName(string resouresName, string expectedAssemblyName) | ||
{ | ||
using (TestResource file = new TestResource(resouresName)) | ||
{ | ||
IProject project = _loader.LoadFrom(file.Path); | ||
|
||
foreach(var config in project.ConfigNames) | ||
{ | ||
TestPackage package = project.GetTestPackage(config); | ||
|
||
Assert.That(Path.GetFileNameWithoutExtension(package.SubPackages[0].FullName) == expectedAssemblyName); | ||
} | ||
} | ||
} | ||
|
||
[Test] | ||
public void FromVSSolution2003() | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>netcoreapp1.1</TargetFramework> | ||
</PropertyGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>netcoreapp1.1</TargetFramework> | ||
<AssemblyName>the-assembly-name</AssemblyName> | ||
</PropertyGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>netcoreapp1.1</TargetFramework> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'"> | ||
<OutputPath>bin\Release\special</OutputPath> | ||
</PropertyGroup> | ||
|
||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, no problem and good to know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂