search

adamralph / bau

The C# task runner
MIT License
152 stars 17 forks source link

Run Xunit in-process #204

Open eatdrinksleepcode opened 9 years ago

eatdrinksleepcode commented 9 years ago

Currently the Xunit task Xunit as an external process. This has a number of downsides:

1) The task must detect what runtime to use in order to launch the process. Both the detection and the specification of the runtime are prone to error. 2) The task is vulnerable to changes in the command line syntax (#195). 3) There is extra overhead in launching and waiting for the external process.

An alternative approach would be to execute in-process with Bau. This is the default approach taken by both Maven and Gradle when running JUnit tests, and it avoids the downsides noted above (most importantly the error-prone runtime detection and specification).

adamralph commented 9 years ago

Hi @eatdrinksleepcode, thanks for raising this, it's an interesting proposal. In response to your points:

  1. I'm not sure this is so prone to error. AFAIK the only detection that needs to happen is 'mono or not mono'. There are plenty of precedents which work reliably using this method -https://github.com/scriptcs/scriptcs/blob/dev/test/ScriptCs.Tests.Acceptance/Support/ScriptCsExe.cs#L12
  2. This is true, but an in process task would be vulnerable to API changes. IMHO an out of process runner is less vulnerable to changes since a command line syntax typically has less churn than an API. I'm assuming xunit 1.x -> xunit 2.0 is the driver behind this point. If Bau.Xunit was currently using the xunit 1.x API it would be a lot more work to convert it to xunit 2.0, since the API has changed beyond all recognition. The change to use the new command line syntax is trivial in comparision.
  3. This is true, but is the overhead is significant? How many invocations are made in a typical build script? The majority of execution time will be spent in test discovery and execution. How does the overhead of the external process invocation compare with this?

Another thing to consider is that it will be easier to support both xunit 1.x and 2.x in Bau.Xunit if command line execution is being used. If it were in-process, we'd have to reference both the relevant 1.x and 2.x assemblies and this would likely involve more 1.x/2.x abstraction in our code. I suspect that the best solution here would be to have separate xunit 1.x and xunit 2.x Bau plugins.

Also, given that the move from xunit 1.x to 2.0 is the driver behind this, it's worth remembering that this is the first time in xunit's history that such a breaking change has been made, and xunit 3.0 is not coming any time soon. The intention is for xunit 2.0 to live for a very long time.

Ultimately, it's a trade off between tight/loose coupling and the benefits/costs associated with each. Right now I'm not really seeing enough benefit from the move from loose coupling (external process via command line) to tight coupling (internal via API).

Of course, you are more than welcome to offer your own alternate in-process xunit plugin for Bau on NuGet. In fact, this would be a good way to test and prove the approach which, if successful, could be adopted by the official package later.

eatdrinksleepcode commented 9 years ago
  1. The only detection that needs to be done right now is 'mono or not mono'. However, soon (technically now) we will also have DNX to deal with. Moreover, even when you detect that a particular runtime is in use, you cannot guarantee that it is the same version of that runtime that will be used by executing the default launcher for that runtime on the command line. I may have multiple versions of Mono installed (which is pretty common), and I am running the current build on a version other than what I currently have set as my system default. All of these problems are avoided by simply not starting another process.
  2. The command line interface and the code interface are both public APIs; one should not have more churn than the other. And more significantly, when there is a change, binding to the command line makes it more difficult to know what version you are working with. #195 proposes to solve this by doing a reflection-only load of the runner executable in order to determine which version it is from. This is more complicated than simply examining the assembly that is already being loaded by the test project being executed.
    • I didn't mention it in the original post, but executing an external process also requires the test runner executable to be available during the build process and located by the build, which usually requires extra setup because the test runner executable is not typically referenced directly by the test project (although in some cases it can be). Using an in-process runner alleviates this requirement.
  3. The overhead is significant enough that the CoreFX repo is going to significant lengths in its build process to avoid executing the test runner if it can determine ahead of time that there are no tests to run. For solutions with a small number of projects it makes little difference, but for larger solutions that are trying to keep a short build time for the sake of developer productivity, any unnecessary overhead should be avoided.

I am willing to prove out my recommended approach in a separate plugin. But it may be a while before I can get to that.

adamralph commented 9 years ago

@eatdrinksleepcode you make some interesting points. I must admit the world of DNX etc. is still somewhat mysterious to me. One thing to bear in mind is that if you run in-process, you are tied to whatever runtime scriptcs, and therefore Bau, is executing under. I think if you launch a separate process there may be more scope for choosing which runtime under which to execute your tests. The general idea of Bau is not really to invoke the software under test within the same runtime as Bau is running, but I can see that in some cases this may be desirable. I think, for now, this is probably best placed as an alternative xunit plug in for Bau. If the approach is proven and turns out to be better than Bau.Xunit, which for now will remain an out of process plug in, then I'll be more than happy to deprecate Bau.Xunit and recommend the in-process plug in instead.

eatdrinksleepcode commented 9 years ago

I must admit the world of DNX etc. is still somewhat mysterious to me.

DNX is still pretty mysterious to me too :) That being said, I am pretty excited about the potential, and I would really like to see Bau support it.

The general idea of Bau is not really to invoke the software under test within the same runtime as Bau is running, but I can see that in some cases this may be desirable.

Where applicable, I have always run my build with the same runtime that I use to compile/test/run my app. I don't see much advantage to doing otherwise, and it makes things so much simpler than having to manage multiple runtimes. I can't say there would never be a case in which you might want to invoke your build under a different runtime than your app, but I would think that would be the exception, not the rule.

If the approach is proven and turns out to be better than Bau.Xunit, which for now will remain an out of process plug in, then I'll be more than happy to deprecate Bau.Xunit and recommend the in-process plug in instead.

Fair enough.

adamralph commented 9 years ago