New unit test runner #1075

grendello · 2017-12-06T16:43:58Z

This is a rewrite of our (very old) NUnit test runner to support tests using
other test suites. The reason for this is that Mono BCL tests are starting to
use xUnit tests and we want to be able to run all of them.

At the same time this code drops support for the "GUI" we previously had to run
tests (not maintained, makes the actual runner code messy) and restructures the
runner infrastructure so that it's possible to add any number of runners in the
future without rewriting any base code. We now use only Android instrumentations
to execute the code.

A few changes to the test running code were necessary since we now support more
than one instrumentation per apk, with different result outputs, options etc.
Support for granting Android permissions on apk install time was added as
well (might be necessary when running tests on devices with Oreo)

radekdoulik · 2017-12-06T17:01:44Z

src/Mono.Android/Test/Mono.Android-Tests.projitems

+  <ItemGroup>
+    <TestApkInstrumentation Include="xamarin.android.runtimetests.TestInstrumentation">
+      <Package>Mono.Android_Tests</Package>
+      <ResultsPath>$(OutputPath)TestResult-Mono.Android_Tests.xml</ResultsPath>


Do we need Package here? It looks like it is duplicated in all the cases.

Yes, we do - each instrumentation is assigned to a single package, think of it as a relation between tables in SQL. It's also easier to write the batched task execution this way. It's cleaner (and yeasier) this way than to create a comma or semicolon separated list in the metadata item for TestApk

I see it only used in the https://github.com/xamarin/xamarin-android/pull/1075/files#diff-6969d1e0d6b6392b34ee6cdcf770c84f and the Package is the same.

Wouldn't it be at least nice to have the Package set in a property, so that it can be set at single place?

Like:

<ItemGroup> <TestApk Include="$(OutputPath)Mono.Android_Tests-Signed.apk"> <Package>$(RuntimeTestPackage)</Package> ... <ItemGroup> <TestApkInstrumentation Include="xamarin.android.runtimetests.TestInstrumentation"> <Package>$(RuntimeTestPackage)</Package> <ResultsPath>$(OutputPath)TestResult-Mono.Android_Tests.xml</ResultsPath> ...

Package name in property name would be nice but it doesn't work in xbuild - been there done that :) It is also used here. It's the only sensible mechanism to support multiple instrumentations per package. We can't put the information in TestApk element and there's also no sane way to "link" <TestApkInstrumetation> to a particular TestApk instance.

I don't know why would it not work on xbuild. We did it before and it worked, like here: https://github.com/grendello/xamarin-android/blob/385699a58635d87a1cc65b60995d3420995f21d7/src/Mono.Android/Test/Mono.Android-Tests.projitems

I tried that approach and, unfortunately, this is still broken. Using a property instead of the string makes UndeployApkTests target fail to process half of them when batching - this is the issue I had sometime ago while working on mono runtime builds. So, as much as it would be a nice change, we can't do it right now :(

That is weird, it worked not so long time ago. What error do you get?

No error at all... Half of the items using that property are simply ignored (in my case in UndeployApks and DeployApks which iterate over @(TestApk)) - this is the behavior I saw with the runtime stuff. It appears that sometimes xbuild fails to process properties inside item metadata.

jonpryor · 2017-12-06T19:38:54Z

tests/Xamarin.Android.Bcl-Tests/TestRunner.Core/Extensions.Bool.cs

+{
+	public static partial class Extensions
+	{
+		public static string YesNo (this bool b)


Why do we have this method? YesNo() isn't used anywhere.

It is used here

jonpryor · 2017-12-06T22:47:06Z

build-tools/mono-runtimes/ProfileAssemblies.projitems

@@ -192,6 +192,10 @@
    <MonoTestAssembly Include="monodroid_corlib_test.dll">
      <SourcePath>corlib</SourcePath>
    </MonoTestAssembly>
+    <MonoTestAssembly Include="monodroid_corlib_xunit-test.dll">
+      <SourcePath>corlib</SourcePath>
+      <TestsType>xunit</TestsType>


Should we add %(MonoTestAssembly.TestsType) to everything, and use nunit for the existing ones?

Also, shouldn't this be singular, i.e. TestType not TestsType?

Changed to TestType but regarding adding nunit to the other items, I don't think it makes sense. This entire thing is a temporary measure until we have xUnit equivalent of make test in Mono and adding the nunit string to all the other items would generate unnecessary noise since it wouldn't be used anywhere anyway.

jonpryor · 2017-12-06T22:49:49Z

build-tools/scripts/RunTests.targets

@@ -18,7 +18,7 @@
    <_ApkTestProject Include="$(_TopDir)\src\Mono.Android\Test\Mono.Android-Tests.csproj" />
    <_ApkTestProject Include="$(_TopDir)\tests\CodeGen-Binding\Xamarin.Android.JcwGen-Tests\Xamarin.Android.JcwGen-Tests.csproj" />
    <_ApkTestProject Include="$(_TopDir)\tests\locales\Xamarin.Android.Locale-Tests\Xamarin.Android.Locale-Tests.csproj" />
-    <_ApkTestProject Include="$(_TopDir)\tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests.csproj" />
+    <_ApkTestProject Include="$(_TopDir)\tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests.csproj" />


This looks weird, in that Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests is duplicative, and Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests.csproj is triplicative.

jonpryor · 2017-12-06T22:51:05Z

Xamarin.Android-Tests.sln

-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Xamarin.Android.Bcl-Tests", "tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests.csproj", "{E865C28E-32AF-4210-A41D-5791C39A9D85}"
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Xamarin.Android.Bcl-Tests", "tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests\Xamarin.Android.Bcl-Tests.csproj", "{04D4E45E-0EBE-40F7-B170-0A8F4D74DCBC}"
+EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "TestRunner.Core", "tests\Xamarin.Android.Bcl-Tests\TestRunner.Core\TestRunner.Core.csproj", "{3CC4E384-4985-4D93-A34C-73F69A379FA7}"


Perhaps the TestRunner.Core project should be moved "up" a level? (This would be particularly useful if we ever get any "takers" to migrate our existing NUnit tests to xUnit!)

I think this could be tests/TestRunner.Core, etc.

I moved the runner backend assemblies to test and renamed tests/Xamarin.Android.Bcl-Tests to tests/BCL-Tests because we have not one but two projects inside that dir and don't think we want to pollute tests/ with these two (since LocalTests.NUnit is not an app, but just an assembly with tests unrelated to and unused by the other projects in tests/)

grendello · 2017-12-07T16:52:41Z

build

grendello · 2017-12-12T13:15:35Z

build

jonpryor · 2017-12-12T14:46:17Z

tests/TestRunner.xUnit/XUnitFilterType.cs

+	public enum XUnitFilterType
+	{
+		Trait,
+		TypeName


Please use trailing , on all enum/etc. values, to reduce "diff noise" when we add new enum values.

jonpryor · 2017-12-12T14:49:45Z

tests/TestRunner.NUnit/NUnitTestRunner.cs

+				//if (!result.IsError && !result.IsFailure && !result.IsSuccess && !result.Executed)
+				//Writer.WriteLine ("\t[INFO] {0}", result.Message);
+				if (result.ResultState.Status != TestStatus.Failed &&
+				    result.ResultState.Status != TestStatus.Skipped &&


The indentation here is wonky/inconsistent.

Expressions within if/while/etc. should be indented one tab stop further than the contents of the if/while/etc.

Indentation should be consistent on every line.

Thus:

if (result.ResultState.Status != TestStatus.Failed && result.ResultState.Status != TestStatus.Skipped && result.ResultState.Status != TestStatus.Passed && result.ResultState.Status != TestStatus.Inconclusive) { Logger.OnInfo ("\t[INFO] {0}", result.Message); }

grendello · 2017-12-12T17:09:17Z

build

grendello · 2017-12-12T22:04:18Z

build

PR dotnet#1075 keeps rebuilding all the Mono runtimes because the two files in this commit are part of the logic to determine whether to use cached Mono or not. The result isn't cached, because it's a PR builder, and so we keep waiting for 7h+ each time a smallest change is made. This commit will cause Mono to be rebuilt and cached and, in turn, speed up round-trip time for PR 1075

grendello · 2018-01-09T22:07:05Z

Do not merge until #1174 is merged and master is built, please

Context: #1075 PR #1075 keeps rebuilding all the Mono runtimes because of the `build-tools/mono-runtimes/*` changes -- which invalidate the bundle hash (97f08f7) -- which means the entire world needs to be rebuilt, which is *very slow* (7+ hours per build). This is getting quite tiresome. Split out the `build-tools/mono-runtimes` changes from PR #1075 so that a new bundle can be built *and cached*, which will speed up subsequent PR #1075 builds.

grendello · 2018-01-10T15:59:04Z

build

This is a rewrite of our (very old) NUnit test runner to support tests using other test suites. The reason for this is that Mono BCL tests are starting to use xUnit tests and we want to be able to run all of them. At the same time this code drops support for the "GUI" we previously had to run tests (not maintained, makes the actual runner code messy) and restructures the runner infrastructure so that it's possible to add any number of runners in the future without rewriting any base code. We now use only Android instrumentations to execute the code. A few changes to the test running code were necessary since we now support more than one instrumentation per apk, with different result outputs, options etc. Support for granting Android permissions on apk install time was added as well (might be necessary when running tests on devices with Oreo)

grendello added do-not-merge PR should not be merged. full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) labels Dec 6, 2017

grendello requested review from jonpryor, dellis1972, jonathanpeppers and radekdoulik December 6, 2017 16:43

dnfclas added the cla-already-signed label Dec 6, 2017

grendello force-pushed the new-unit-tests-runner branch 2 times, most recently from 680b837 to 0eeb3d9 Compare December 6, 2017 17:00

radekdoulik reviewed Dec 6, 2017

View reviewed changes

jonpryor reviewed Dec 6, 2017

View reviewed changes

grendello force-pushed the new-unit-tests-runner branch 3 times, most recently from ce9db25 to 0a85563 Compare December 7, 2017 16:28

dnfadmin removed the cla-already-signed label Dec 12, 2017

jonpryor reviewed Dec 12, 2017

View reviewed changes

grendello force-pushed the new-unit-tests-runner branch from 0a85563 to 2f63367 Compare December 12, 2017 16:51

grendello force-pushed the new-unit-tests-runner branch 3 times, most recently from d696758 to 5f12662 Compare December 14, 2017 10:36

grendello force-pushed the new-unit-tests-runner branch from 5f12662 to 97b1d68 Compare January 3, 2018 12:29

grendello removed the do-not-merge PR should not be merged. label Jan 8, 2018

grendello force-pushed the new-unit-tests-runner branch 2 times, most recently from 5c31fd1 to 60ab204 Compare January 9, 2018 13:21

grendello mentioned this pull request Jan 9, 2018

Preparation for the new test runner #1174

Merged

grendello force-pushed the new-unit-tests-runner branch 2 times, most recently from 713e854 to 9469613 Compare January 10, 2018 14:53

grendello force-pushed the new-unit-tests-runner branch from 9469613 to f282079 Compare January 10, 2018 19:49

grendello force-pushed the new-unit-tests-runner branch from f282079 to 5b05a04 Compare January 11, 2018 08:50

jonpryor merged commit c4e8165 into dotnet:master Jan 11, 2018

grendello deleted the new-unit-tests-runner branch January 11, 2018 21:39

github-actions bot locked and limited conversation to collaborators Feb 2, 2024

New unit test runner #1075

New unit test runner #1075

Uh oh!

Conversation

grendello commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radekdoulik Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grendello commented Dec 7, 2017

Uh oh!

grendello commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grendello commented Dec 12, 2017

Uh oh!

grendello commented Dec 12, 2017

Uh oh!

grendello commented Jan 9, 2018

Uh oh!

grendello commented Jan 10, 2018

Uh oh!

Uh oh!

radekdoulik Dec 6, 2017 •

edited

Loading