nunit / nunit.xamarin

NUnit test runners for Xamarin and mobile devices - No longer maintained
http://nunit.org
MIT License
50 stars 47 forks source link

Allow running multiple assemblies #82

Closed ChrisMaddock closed 7 years ago

ChrisMaddock commented 7 years ago

Fixes #59.

Overview: This builds a TestPackage containing all the assemblies, which ones them sequentially, and gathers the results into a TestRunResult. This is then passed into a ResultSummary, which is used for the presenters. It looks a little bigger than it is, as it required modifying everything which uses the test results, and adding the <test-run> xml and summary.

The presenters and services were all previously based off the ITestResult from the Framework, so I tried to stay on that level to avoid rewriting all the presenters too. The other option would be to interact with the Framework on the Api.FrameworkController level, and get xml string out, and then parse them back into model objects. I thought that might be a phase 2 - if we're eventually aiming to have this interact with an engine of sorts?

In terms of review, it would be great if someone more experienced could have a closer look at ResultSummary.Summarize(ITestResult result) and the precedence there in calculating the overall result. Also, I'm afraid I still can't build iOS, but this seems good on Android and UWP.

rprouse commented 7 years ago

I still need to code review, but your approach sounds good for now. We are going to need a .NET Standard version of the engine if we are going to run .NET Core tests in the adapter. We can update this project using that when it is available.

ChrisMaddock commented 7 years ago

I think so. :+1: Not sure if we'd want to consider backwards compatibility with that at all, as I imagine a .NET Standard package could only be run in the lastest platform versions - but I think the benefits outweigh the backwards-compatibility cost here. I put a couple of comments on that in #87. 😄

rprouse commented 7 years ago

I really wish we had proper unit tests in this project. This looks good, but it is a whack of code. How did you test it? Should you add a second unit test project so that you can at least look at and validate the output?

We should probably also be adding warnings and multi-assert blocks although as separate pull requests.

@ChrisMaddock I am going to leave it up to you to merge this if you feel that the testing you have done is adequate.

ChrisMaddock commented 7 years ago

I really wish we had proper unit tests in this project.

Definitely - I thought the same with this. I think the original project was small enough to get away without, but it's definitely grown to a size where it needs them now.

We should probably also be adding warnings and multi-assert blocks although as separate pull requests.

There's an issue for warnings, which I was planning to get into the next release - it'll be two lines of code, but will conflict with changes in both this and #85 - so I was leaving it until these were merged. 😄

Multiple asserts - I was thinking were less urgent, as the legacy AggregatedException should be acceptable for now. I also wondered if it was worth switching over to the portable agent for execution before doing this - as then we'd be implementing it on top of xml results rather than ITestResults - which would mean we wouldn't need to redo the work when we eventually move to a full engine.

How did you test it? Should you add a second unit test project so that you can at least look at and validate the output?

Fully manually. I just added the test assembly twice, and then checked that the views and xml output were represented properly. I didn't leave that in as I thought it would just cause confusion as to why all the tests were displayed twice.

@ChrisMaddock I am going to leave it up to you to merge this if you feel that the testing you have done is adequate.

I remember thinking so at the time, mainly because my first implementation definitely wasn't correct. 😆 I'm going to merge it in - but will double check this before release - once all dependency updates are also merged in. Thanks!

I'll also create issues for your other two points, we should sort them really, at some point.