From daa1ea2f7f720b99b1be045fc1414de25c706364 Mon Sep 17 00:00:00 2001
From: Jonathan Pryor <jonpryor@vt.edu>
Date: Wed, 8 Mar 2017 21:08:18 -0500
Subject: [PATCH] [tests] Report an error when .apk tests crash
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

While reviewing [PR 445][0] I noticed that it said that *41* tests had
been run, and 0 failed. Compare to e.g. [PR 474][1], which has *164*
passing tests.

Something isn't right here.

Further investigation showed that for PR 445, execution on the
emulator had crashed ("Segmentation fault"). The error was ignored.

Ignoring errors was in fact By Design™; see commit 6358a643.

So what we need is for the `RunUnitTestApks` target to *not* report
errors, while still reporting errors.

Solve this conundrum by updating the `<RunInstrumentationTests/>` task
so that when the `nunit2-results-path` value can't be found, it
reports a *successful* run, and sets the
`RunInstrumentationTests.FailedToRun` property to the name of the
component that failed. This allows the `ReleaseAndroidTarget` target
to check for any failures, and report an `<Error/>` after shutting
down the emulator instance.

This should allow the build to "fail" when `nunit2-results-path` can't
be found -- a sign that the `.apk` unit tests didn't finish executing
-- while also allowing the emulator to be shut down.

Aside: `xbuild` doesn't support `ContinueOnError="ErrorAndContinue"`,
nor does it set `<Output/>` properties whenever a task fails.
Consequently, the `<RunInstrumentationTests/>` task *must* return true
when `nunit2-results-path` isn't found, because there's no other way
to convince `xbuild` to collect the `<Output/>` values.

[0]: https://github.com/xamarin/xamarin-android/pull/445
[1]: https://github.com/xamarin/xamarin-android/pull/474
---
 build-tools/scripts/UnitTestApks.targets         |  7 +++++--
 .../RunInstrumentationTests.cs                   | 16 +++++++++++-----
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/build-tools/scripts/UnitTestApks.targets b/build-tools/scripts/UnitTestApks.targets
index 9bb4d6590f3..fdeaef1cf73 100644
--- a/build-tools/scripts/UnitTestApks.targets
+++ b/build-tools/scripts/UnitTestApks.targets
@@ -63,6 +63,10 @@
         ToolExe="$(AdbToolExe)"
         ToolPath="$(AdbToolPath)"
     />
+    <Error
+        Condition="'@(_FailedComponent)' != ''"
+        Text="Execution of the following components did not complete successfully: @(_FailedComponent->'%(Identity)', ', ')"
+    />
   </Target>
 
   <!--
@@ -96,7 +100,6 @@
   <Target Name="RunUnitTestApks"
       Condition=" '@(UnitTestApk)' != '' ">
     <RunInstrumentationTests
-        ContinueOnError="True"
         AdbTarget="$(_AdbTarget)"
         AdbOptions="$(AdbOptions)"
         Component="%(UnitTestApk.Package)/%(UnitTestApk.InstrumentationType)"
@@ -104,7 +107,7 @@
         TestFixture="$(TestFixture)"
         ToolExe="$(AdbToolExe)"
         ToolPath="$(AdbToolPath)">
-      <Output TaskParameter="NUnit2TestResultsFile" PropertyName="_ResultsFile "/>
+      <Output TaskParameter="FailedToRun" ItemName="_FailedComponent"/>
     </RunInstrumentationTests>
   </Target>
 </Project>
diff --git a/src/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RunInstrumentationTests.cs b/src/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RunInstrumentationTests.cs
index 23d195e3a3b..151ea7b4403 100644
--- a/src/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RunInstrumentationTests.cs
+++ b/src/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/RunInstrumentationTests.cs
@@ -21,9 +21,11 @@ public class RunInstrumentationTests : Adb
 
 		public                  string[]            InstrumentationArguments    { get; set; }
 
-		[Output]
 		public                  string              NUnit2TestResultsFile       { get; set; }
 
+		[Output]
+		public                  string              FailedToRun                 { get; set; }
+
 		protected   override    bool                LogTaskMessages {
 			get { return false; }
 		}
@@ -63,15 +65,19 @@ public override bool Execute ()
 			base.Execute ();
 
 			if (string.IsNullOrEmpty (targetTestResultsPath)) {
-				Log.LogError ("No `nunit2-results-path` bundle value found in command output! Cannot find NUnit2 XML output!");
-				return false;
+				FailedToRun = Component;
+				Log.LogError (
+						"Could not find NUnit2 results file after running component `{0}`: " +
+						"no `nunit2-results-path` bundle value found in command output!",
+						Component);
+				// Can return false once we use MSBuild and not xbuild
+				// return false;
+				return true;
 			}
 
 			executionState  = ExecuteState.PullFiles;
 			base.Execute ();
 
-			Log.LogMessage (MessageImportance.Low, $"  [Output] {nameof (NUnit2TestResultsFile)}: {NUnit2TestResultsFile}");
-
 			return !Log.HasLoggedErrors;
 		}