Code Review: MSBuild task

Oct 8, 2010 at 8:21 AM

Code review for: http://nupack.codeplex.com/workitem/205 (an MSBuild task for creating NuPack packages)

Fork: https://hg01.codeplex.com/forks/anglicangeek/nupackmsbuild

Notes:

  • I'm not sure if this project is the appropriate long-term home for an MSBuild task, but Phil suggested we start with it here and can move it later
  • The two significant changes I made to NuPack.Core are:
    • Extracted an IPackageBuilder interface from PackageBuilder so for TDD
    • Added some footprint to IFileSystem (CreateFile, GetFullPath, GetCurrentDirectory) for TDD; this required compensating changes, but I made the new stuff NotSupportedException where it wasn't being used
  • Logger and Root don't seem like they belong on IFileSystem; maybe move them to ProjectSystem?
  • The new stuff is mostly in NuPack.MSBuild
    • There is an IPackageBuilderFactory (and accompanying concrete) that I created so I could TDD, thanks to the static PackageBuilder.ReadFrom; I didn't put this in NuPack.Core, as I wanted to see what other folks thought
    • Having static PackageBuilder.ReadFrom is a pain in the arse
    • There is a FileSystem impl' here that is somewhat redundant to FileBasedProjectSystem, but it didn't seem appropriate to consume that; I think we might want to break the file system abstraction and project system abstraction apart a bit so we can eliminate the redundancy
  • There are both unit tests and integration tests
  • The NuPack.MSBuild.dll is ILMerged with NuPack.Core
  • The unit tests are the best place to go for a "spec" on how it behaves; if there are questions, that means I missed some tests, which I'll add

Thanks!

Developer
Oct 8, 2010 at 9:21 AM
  • I think it's fine to use the abstractions we have defined in the core, but I don't think we should expand them for other purposes. If you need something specific from the file system create another type. 
  • Logger has to be on the file system, it's the way it logs to the command line :) (run Install-Package -Verbose to see the output). 
  • Root is a fail, but we need it to get the physical path (which breaks the abstraction) so we can add assembly references (limitation of VS).
  • The reason we have IFileSystem and ProjectSystem are for solutions and projects. FileBasedProjectSystem could be split into PhysicalFileSystem and FileBaseProjectSystem but that seems like overkill with little benefit.

 

That was the initial sweep, I'll have more comments later :)

Oct 8, 2010 at 2:38 PM
  • It's funny you mention not extending IFileSystem; that was a late change. I originally had an IFileSystem in NuPack.MSBuild that extended the one in core.
  • Logger still doesn't make sense to me there; not that logging isn't needed, it just doesn't seem like the right composition to me. It was just a thought, obviously not a change I need.
  • Yeah, I understand the need for IFileSystem and ProjectSystem, and I just think we could change the composition such that we have a true file system abstraction for everyone to use, and the project system where needed. To be honest, I didn't dig into that part to a great extent, so I might be totally wrong
  • I'll wait to see the rest of the criticism, but I'm leaning toward just using a tighter file system abstraction for the MSBuild task, which will reduce the changes in core. I can also move IPackageBuilder to MSBuild if you don't want it in Core, but I think making Core testable is a good thing.
Developer
Oct 8, 2010 at 7:27 PM

The core is testable, you mean *more* testable. The only reason I don't want the IPackageBuilder right now is because i think IPackageBuilder should implement IPackage (just didn't make the change yet) and IPackageBuilder should just have Save. I also think the statics are for convenience, look at File.ReadAllText etc. Sure you still have FileStream and Stream but for the 90% case of reading a package from a nuspec is easy.

Oct 8, 2010 at 7:38 PM

Sure, I can be more specific regarding testability: I think making PackageBuilder testable is a good thing, which in turns make Core *more* testable.

I agree with you on the composition of PackageBuilder; it's overloaded and the change you mentioned would be a good one.

Is there anything else you want me to change before a second review (I know you're busy, so no rush)? Right now, I'll:

  • Back out all of the Core changes
  • Use abstractions where needed in MSBuild (file system, package builder)
Developer
Oct 8, 2010 at 11:56 PM

Sounds like a plan.

Oct 9, 2010 at 6:03 AM

Here's a new fork after code review: https://hg01.codeplex.com/forks/anglicangeek/issue205

Notes

  • No changes to Core
  • Several abstractions and wrappers in MSBuild that will hopefully go away in the future as we refactor Core
Developer
Oct 9, 2010 at 8:46 PM

Even though it's poorly named right now, NuPack.Test is really NuPack.Core.Test (I haven't had time to change it yet). We'll be having one unit test project per source project. Other than that
Looks good :). Make sure it's fxcop clean, we just added a whole set of rules.

Oct 9, 2010 at 9:07 PM

"one unit test project per source project". What's the reason behind that? It just means you end up with a lot more projects than you really need. I find one test project is enough, using folder/namespaces to line up with the code you're testing. Then you only have to test one assembly instead of n and can get aggreated test results and coverage easier (although most unit test frameworks these days can do that aggregation for you, and most usually do)

Coordinator
Oct 9, 2010 at 10:28 PM

In general, I agree with Bil. I like to have one unit test project for the solution. However, the drawback here is that it makes that one project tightly coupled to all the others.

Why is this a problem? Consider the fact that we'll probably add a NuPack.Mono.sln file which will exclude projects that don't make sense for the Mono world (per Mono team's request). Well, this is a problem if our one unit test project references *every* project in the solution. When you load the mono solution, it wouldn't compile. 

If we keep a unit test project per project, then we can compose projects together in a solution however we want. Unless you have another solution to the Mono solution issue. :)

Oct 9, 2010 at 10:29 PM

I'm a big fan of testing based on what those tests are for.
*All unit testing goes in one project per solution.
*Integration testing (touching outside resources) goes to its own project.
*Then we have some higher level customer interaction type testing that may involve watin/<insert bdd framework here> and be written from a customer/user perspective.

"Given a user is in the nupack gui
and they do not have nhibernate
when they click nhibernate in nupack
it should download nhibernate and download antlr
and add a reference to nhibernate
and add a refence to antlr."

The test is literally written exactly like that and you create the underlying translation (specflow or storevil are frameworks that support this).

Testing is usually the poorest treated thing when it comes to any project I've seen, but it is how people familiarize themselves with your project.
____
Rob
"Be passionate in all you do"

http://devlicio.us/blogs/rob_reynolds
http://ferventcoder.com
http://twitter.com/ferventcoder

Oct 9, 2010 at 10:38 PM

Hmmm. Didn't consider the mono issue but I agree that it might cause issues trying to load everything. Maybe we have one core test project and one for windows specific tests (which would require VS) with that being basically an integration testing project. So:

  • NuPack.Test.Core
  • NuPack.Test.VisualStudio

The mono solution would only load the first test project while the windows version would load both (as would our CI server)

 

Oct 11, 2010 at 7:02 PM

So, should I leave the test in Core's unit tests, or create a separate one for the MSBuild task?

Developer
Oct 12, 2010 at 3:50 AM

I see no one wants to reply. I say new project (esp if this goes into the msbuild contrib or whatever)

Oct 12, 2010 at 4:19 AM

While I'm not sure about a 1:1 assembly to test project ratio, it makes sense to me for at least Core and each payload to have their own tests. I'll make that change and send the pull request.