Skip to content
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

Support for CPS projects #300

Closed
jnm2 opened this issue Jan 27, 2017 · 15 comments
Closed

Support for CPS projects #300

jnm2 opened this issue Jan 27, 2017 · 15 comments
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Jan 27, 2017

Please support the new Common Project System .csproj coming with VS2017. As of build 15.0.26124.0, this is what it looks like:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net462</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="NUnit" Version="3.6.0" />
  </ItemGroup>
</Project>

When you build this, the console runner successfully detects and runs tests as usual. The tests do not show up in Visual Studio however, whether you use the test window, ReSharper, or NCrunch.

For comparison, test projects like this show up just fine in all of the above:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net462</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.0.0-preview-20170106-08" />
    <PackageReference Include="MSTest.TestAdapter" Version="1.1.8-rc" />
    <PackageReference Include="MSTest.TestFramework" Version="1.0.8-rc" />
  </ItemGroup>
</Project>

Right now we're blocked on #296.

@CharliePoole
Copy link
Contributor

@JMM2 I don't understand what you're asking for. NUnit doesn't "support" any project format. In the case of #296 we have something that NUnit is actually responsible for so we are able to discuss alternative ways to do it. But not here, since NUnit knows nothing about VS and the adapter works with assemblies, not projects.

As far as #296 being blocked, it doesn't seem to be. At least we haven't labeled it so. It's shown as a new issue under discussion.

@jnm2
Copy link
Contributor Author

jnm2 commented Jan 27, 2017

If you use VS2015 > new project, it works. If you use VS2017 > new project, it will not work. That's the distinction consumers will make when they come looking for discovery support for these test projects. They won't be searching this repo for Mono.Cecil, because you never expose the exception and they have no error message to direct them.

CPS projects result in CPS project outputs. CPS project outputs are currently not supported. As far as I know, they will effectively be supported as soon as #296 is fixed, but that's not a given. This is problem-space, #296 is solution-space. Hopefully we're interested in fixing #296 and anything else that comes up for the sake of accomplishing this goal.

#296 is not blocked, but as I said anyone who tries to use CPS is blocked on #296. They won't know it though; they'll see it as a missing feature.

@jnm2
Copy link
Contributor Author

jnm2 commented Jan 27, 2017

Plus, we aren't really supporting this scenario unless it's under test. This scenario will start working as a side effect of fixing #296 but without tests, another side effect could bring it all back down.
Building the test assembly will require msbuild 15. Which may be its own discussion.

@CharliePoole
Copy link
Contributor

Yes, I understand that users will perceive this as a problem with the project format. We get lots of issues like that and each time we carefully explain. It's part of our educational role. :-)

If you had posted under an alias, I probably would have done that for you too. But, of course,I know you as one of "us" - so I guess I'm asking you to translate this into solution-space, because it's the only space in which we can solve problems. 😄

We can't have an issue that says "Fix anything possible that may ever come up with Microsoft's new format." We need to identify specific things to fix.

One thing we definitely need for this project is integration or acceptance tests. We've talked about that and we have issues on it for some of the other projects. I don't see one for this project. Should we convert this to be that issue? Or part of it?

@jnm2
Copy link
Contributor Author

jnm2 commented Jan 27, 2017

But, of course,I know you as one of "us" - so I guess I'm asking you to translate this into solution-space, because it's the only space in which we can solve problems.

Part of the reason I posted this, besides keeping the bigger picture visible, is that I think it's valuable to think in terms of problem space. It's all too easy to think in terms of the how instead of the what. Education is great but I've felt even that can verge on TMI. The "what" is the goal. At the end of the day, particular implementation details only exist to serve that goal. I'd argue that as a consumer of NUnit there is no reason that I should have to care that there is a difference between supporting a kind of project and supporting the output that the kind of project creates. :)

We can't have an issue that says "Fix anything possible that may ever come up with Microsoft's new format." We need to identify specific things to fix.

What I'm trying to say is that this issue should be discussing the current problem that NUnit does not support CPS projects. Discussing intentions and discussing what will comprise the solution-space in order to be able to close this issue. This type of discussion is much bigger than #296 or any single fix.

Here's part of the solution:

  • Integration tests as you mentioned, which require msbuild 15

  • Fixing Mono.Cecil causes OutOfMemoryException with new .csproj PDBs #296 and anything else that prevents the integration tests from succeeding

  • Documentation of the intention to support CPS projects at https://github.com/nunit/docs/wiki/Visual-Studio-Test-Adapter. It's the difference between saying "you're on your own to figure out what we may or may not support and why things don't work" and saying "we do our best to make sure that the NUnit adapter works with CPS projects, whatever that entails in the future, and without you having to know about implementation details."

@jnm2
Copy link
Contributor Author

jnm2 commented Jan 27, 2017

About educating consumers to think in terms of solution space instead of problem space- if I intend to help implement, that's great. But this is not something we'd do to our customers, not even our customers who are programmers.

The awareness recently hit me that Scott Hanselman's "There are only a finite number of keystrokes left in your hands before you die" might apply just as much to learning about the internals of things. There are only so many things you have the time and energy to learn about in a week without losing the truly important parts and eventually burning yourself out. While you should never stop learning, you should invest it wisely.
I don't know much about my car. The mechanic's job is to take their fascinating and worthwhile insight of the internals of the car and explain to me the problem at hand in as simplified and problem-space a way as possible. If a mechanic tried to teach me to think like a mechanic, it's a drain on my time and energy. What makes it a waste is that I don't plan to ever attain that level of knowledge. I'm here because I have a car problem- I wouldn't be here otherwise. That is a legitimate perspective and IMO the correct perspective if you have other commitments to fulfill.

Rant over, and of course this is just one guy's limited perspective. Likely I have much to learn here. :) Ever since I showed up here, asking to invest myself in NUnit to some small extent, I have really appreciated the open atmosphere and your willingness to spend time educating me. :')

@CharliePoole
Copy link
Contributor

All good ideas. Each item you list is something that I can imagine being "Done" at some point, with only a few exceptions.

  • Integration tests. Somebody could write that issue and volunteer to accomplish it. It would not require msbuild 15, precisely. Rather some tests would require msbuild 15. We should also have acceptance tests that run on VS2012, 2013 and 2015. But that could be taken care of by creating the structure for a series of acceptance tests and implementing those you are concerned with.

  • Fixing Mono.Cecil causes OutOfMemoryException with new .csproj PDBs #296 is quite clear. We should get to work on that. OTOH, "anything else" can't be an issue although the individual things that come up can still be issues, as they come up.

  • Documentation. We document what we support. We also document our vision of the product line and each product. We can't know what will be entailed by some new thing that comes up in the future and can only deal with it as it comes up. When a new feature comes up, we can decide if we will support it and document that or we can decide not to support it. We can even decide to drop this software product, as unlikely as that sounds. IOW, I don't think we can document the future.

I do think you are exaggerating a lot when you say "you're on your own...". We definitely don't say or imply that anywhere. 😄

I agree that we need to be having more discussions of future direction. Many of those have taken place privately between members of particular teams. Maybe that needs to be opened up a bit more so that others can see what's being discussed. Mainly, the choice of private discussions has been motivated by the fact that we don't have a good place to carry them out more openly. Issues don't seem to me to fill the need. They ought to be things we can accomplish in a reasonable time and have a clear definition of "doneness" - very much like stories in XP. Additionally, because of how GitHub is organized, it's hard to find a place to discuss things that cut across multiple projects, as this seems to do.

We will soon have a more formal statement of project organization and team setup, so I hope there will be a better place for such overall discussions.

If you want to create some text for us to review that states clearly what you want to see the nunit organization commit to WRT support of CPS, we could all review it. To the extent that it deals with the future, rather than the present, I think you'll find it's hard to make a committment... but let's see.

If you want to or want others to work on some of the individual issues, please create them.

@CharliePoole
Copy link
Contributor

As a general thing, I agree with you. However, the fact that NUnit knows nothing except what is in the assembly is one of five or six fundamental things I teach about how NUnit works when I introduce it. I really don't think that's a serious overload for anybody.

The thing is, our audience is mechanics. We're just more advanced (in some ways anyway) mechanics who can help them out with certain things. That's not much different from back in the day, when I used to bet hired to be the guy in a project who knew more about COM than the rest of the developers. In working with them, I didn't talk down to them but tried to treat them as programmers with a different specialty from what I had.

Judgement is obviously needed here. I think it's fine to talk about the new CPS format, so long as you drop down to a slightly lower level and say things like: the new format generates pdb files in a format we can't handle, but we are working on it. To only talk at the high level, is insulting the intelligence of our uses.

As far as problem- versus solution-space goes, that also varies by audience. Problem-space for our users does involve things like assemblies, reflection, overloads, etc. How we make it work is our solution-space and is at a much lower level than anything we are talking about here.

@codito
Copy link

codito commented Feb 13, 2017

I'm able to run NUnit desktop tests after adding the test adapter (NUnit3TestAdapter) package reference. This also has the workaround for #296 applied.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net462</TargetFramework>
    <DebugType Condition="'$(TargetFramework)' != '' AND '$(TargetFramework)' != 'netcoreapp1.0'">Full</DebugType>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="NUnit" Version="3.6.0" />
    <PackageReference Include="NUnit3TestAdapter" Version="3.7.0" />
  </ItemGroup>
</Project>

@OsirisTerje
Copy link
Member

@codito Do you need the testadapter in every project, or just in one?

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 14, 2017

Fwiw, ever since we moved away from packages.config and the problematic solution-local packages folder, I don't use the test adapter in any project. My Cake script downloads the test adapter package to a build tools folder and passes /testadapterpath: to VSTest.

@OsirisTerje
Copy link
Member

Ok, so what is the issue here? Afaik Visual Studio searches the solution folder (at least the packages folder) and downwards in order to find any testadapter dll's and loads them. If they are there, you say it still doesn't load in VS?

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 14, 2017

@OsirisTerje The problem is that the adapter gets loaded, but it crashes when reading assemblies with the new PDB format. See #296. It silently swallows the exception and makes it look as though nothing happened at all. No tests are discovered.

@thomaslevesque
Copy link

Thanks for the workaround @codito! <DebugType>Full</DebugType> fixed my problem.

@jnm2
Copy link
Contributor Author

jnm2 commented Mar 13, 2018

As of #454, available in https://www.nuget.org/packages/NUnit3TestAdapter/3.10.0, the adapter tests are contingent on support for the new SDK csproj format. That's the most solid support possible. 😃 Glad to be able to close this, as well as removing <DebugType> workarounds!

@jnm2 jnm2 closed this as completed Mar 13, 2018
@jnm2 jnm2 added this to the 3.10 milestone Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants