[tests] Add Configuration info to test names #929

jonpryor · 2017-10-10T22:31:47Z

Context: 0077d15
Context: d1d9820

A "funny" thing happened when commit e9daf5e didn't build on Jenkins:
I realized that not all tests were run in all configurations. From
commit d1d9820:

Why are some tests Debug-only and some aren't?

The answer: time, primarily. Why run tests multiple times, when they
can be potentially time-consuming?

While tests can be slow, they're not always that slow -- except for
Xamarin.Android.Build.Tests and the BCL tests -- and even there,
program behavior can alter between Debug and Release configurations.
See in particular commit 0077d15, in which the BCL tests are run only
in the Debug configuration because tests failed when run in the
Release configuration.

The desire, then, is to run all tests in both Debug and Release
configurations. Yes, it'll take longer! So what! (Within reason:
Xamarin.Android.Build.Tests should only be run once!)

However, this raises two problems:

Filename collisions
Jenkins unit test display

Until now, all tests wrote files into a filename that didn't include
the Configuration, e.g. TestResult-Mono.Android_Tests.xml. If we did
run these tests twice, the second test invocation would overwrite the
first test invocation. This isn't desirable.

Then there's the display on Jenkins: if we did have e.g.
TestResult-Mono.Android_Tests-Debug.xml and
TestResult-Mono.Android_Tests-Release.xml, how will Jenkins display
that information? I haven't tested, but I would assume that one of two
things will occur, assuming reasonable Jenkins behavior:

Each test will be listed twice, e.g.

ApplicationContextIsApp
ApplicationContextIsApp

They'll be "merged" into a single entry.

Neither of these behaviors is desirable: if Debug passes but Release
fails, we need to be able to differentiate between them. Neither of
these possible renderings allows us to tell which configuration fails.

Solve both of these problems by introducing a new <RenameTestCases/>
task. This task takes three values of importance:

<RenameTestCases
    Configuration="CONFIGURATION"
    SourceFile="SOURCE"
    DestinationFolder="DESTINATION"
/>

The <RenameTestCases/> task will read in SOURCE, and if SOURCE
is an XML file which we determine is NUnit2-formatted XML (root
element of <test-case/>), we will update every //test-case/@name
value so that it ends with / CONFIGURATION. The updated XML is
then written to the DESTINATION directory, with a filename that
contains CONFIGURATION, and SOURCE is deleted.

Thus, if we have a Debug-configuration
TestResult-Mono.Android_Tests.xml file with XML fragment:

<test-case
    name="Mono.Android_Tests, Android.AppTests.ApplicationTest.ApplicationContextIsApp"
    ...
/>

then <RenameTestCases/> will create the file
TestResult-Mono.Android_Tests-Debug.xml file with XML fragment:

<test-case
    name="Mono.Android_Tests, Android.AppTests.ApplicationTest.ApplicationContextIsApp / Debug"
    ...
/>

This allows us to run tests in both Debug and Release configurations
while not inadvertently overwriting the TestResults*.xml files that
Jenkins reads, and ensuring that the Jenkins test result output is
rendered in a meaningfully useful fashion.

Aside: when updating //test-case/@name, the resulting value cannot
end in ). If it does, then the (root) package name issue fixed in
commit 23b2642 reappears for the generator unit tests.

Completely random aside about the state of xbuild:
A development version of <RenameTestCases/> was "saner", using
ITaskItem[] and not string:

partial class RenameTestCases {
    public      ITaskItem[]   SourceFiles {get; set;}
    // vs.
    //  public  string        SourceFile  {get; set;}
}

The problem is that the above, while entirely reasonable, did not work
at all correctly with xbuild:

<RenameTestCases SourceFiles="%(TestApk.ResultsPath)" />

Under xbuild, MSBuild properties would not be expanded, e.g.
RenameTestCases.SourceFiles would get a "bizarro" value of e.g.
$(OutputPath)Mono.Android_Tests-Signed.apk, which is useless and
would result in FileNotFoundExceptions.

MSBuild proper, of course, worked as desired.

TODO: Once this is merged, update the Jenkins Configuration page so
that instead of:

    make run-all-tests V=1 || exit 1

it instead runs both Debug and Release configuration tests:

    make run-all-tests SKIP_NUNIT_TESTS=1    V=1 || exit 1
    make run-all-tests CONFIGURATION=Release V=1 || exit 1

Note that $(SKIP_NUNIT_TESTS) is specified so that we only run the
lengthy (1+hr!) Xamarin.Android.Build.Tests tests in the Release
configuration, not the Debug + Release configurations.

jonpryor · 2017-10-10T22:34:32Z

build

Context: 0077d15 Context: d1d9820 A "funny" thing happened when commit e9daf5e didn't build on Jenkins: I realized that not all tests were run in all configurations. From commit d1d9820: > Why are some tests Debug-only and some aren't? The answer: time, primarily. Why run tests multiple times, when they can be potentially time-consuming? While tests can be slow, they're not always *that* slow -- except for `Xamarin.Android.Build.Tests` and the BCL tests -- and even there, program behavior can alter between Debug and Release configurations. See in particular commit 0077d15, in which the BCL tests are run only in the Debug configuration because tests *failed* when run in the Release configuration. The desire, then, is to run *all* tests in both Debug and Release configurations. Yes, it'll take longer! So what! (Within reason: `Xamarin.Android.Build.Tests` should only be run once!) However, this raises two problems: 1. Filename collisions 2. Jenkins unit test display Until now, all tests wrote files into a filename that didn't include the Configuration, e.g. `TestResult-Mono.Android_Tests.xml`. If we did run these tests twice, the second test invocation would overwrite the first test invocation. This isn't desirable. Then there's the display on Jenkins: if we did have e.g. `TestResult-Mono.Android_Tests-Debug.xml` and `TestResult-Mono.Android_Tests-Release.xml`, how will Jenkins display that information? I haven't tested, but I would assume that one of two things will occur, assuming reasonable Jenkins behavior: 1. Each test will be listed twice, e.g. ApplicationContextIsApp ApplicationContextIsApp 2. They'll be "merged" into a single entry. Neither of these behaviors is desirable: if Debug passes but Release fails, we need to be able to differentiate between them. Neither of these possible renderings allows us to tell which configuration fails. Solve both of these problems by introducing a new `<RenameTestCases/>` task. This task takes three values of importance: ```xml <RenameTestCases Configuration="CONFIGURATION" SourceFile="SOURCE" DestinationFolder="DESTINATION" /> ``` The `<RenameTestCases/>` task will read in `SOURCE`, and if `SOURCE` is an XML file which we determine is NUnit2-formatted XML (root element of `<test-case/>`), we will update every `//test-case/@name` value so that it ends with ` / CONFIGURATION`. The updated XML is then written to the `DESTINATION` directory, with a filename that contains `CONFIGURATION`, and `SOURCE` is deleted. Thus, if we have a Debug-configuration `TestResult-Mono.Android_Tests.xml` file with XML fragment: ```xml <test-case name="Mono.Android_Tests, Android.AppTests.ApplicationTest.ApplicationContextIsApp" ... /> ``` then `<RenameTestCases/>` will create the file `TestResult-Mono.Android_Tests-Debug.xml` file with XML fragment: ```xml <test-case name="Mono.Android_Tests, Android.AppTests.ApplicationTest.ApplicationContextIsApp / Debug" ... /> ``` This allows us to run tests in both Debug and Release configurations while not inadvertently overwriting the `TestResults*.xml` files that Jenkins reads, and ensuring that the Jenkins test result output is rendered in a meaningfully useful fashion. Aside: when updating `//test-case/@name`, the resulting value *cannot* end in `)`. If it does, then the `(root)` package name issue fixed in commit 23b2642 reappears for the `generator` unit tests. **Completely random aside about the state of `xbuild`**: A development version of `<RenameTestCases/>` was "saner", using `ITaskItem[]` and not string: ```csharp partial class RenameTestCases { public ITaskItem[] SourceFiles {get; set;} // vs. // public string SourceFile {get; set;} } ``` The problem is that the above, while entirely reasonable, did not work at all correctly with `xbuild`: ```xml <RenameTestCases SourceFiles="%(TestApk.ResultsPath)" /> ``` Under `xbuild`, MSBuild properties would not be expanded, e.g. `RenameTestCases.SourceFiles` would get a "bizarro" value of e.g. `$(OutputPath)Mono.Android_Tests-Signed.apk`, which is *useless* and would result in `FileNotFoundException`s. MSBuild proper, of course, worked as desired. TODO: Once this is merged, update the Jenkins Configuration page so that instead of: make run-all-tests V=1 || exit 1 it instead runs both Debug and Release configuration tests: make run-all-tests SKIP_NUNIT_TESTS=1 V=1 || exit 1 make run-all-tests CONFIGURATION=Release V=1 || exit 1 Note that `$(SKIP_NUNIT_TESTS)` is specified so that we only run the lengthy (1+hr!) `Xamarin.Android.Build.Tests` tests in the Release configuration, not the Debug + Release configurations.

jonpryor · 2017-10-14T01:04:35Z

Yay, time for sporadic test failures!

I/mono-stdout( 2690):       SendAsyncFile [FAIL] :   #1
I/mono-stdout( 2690):   Expected: True
I/mono-stdout( 2690):   But was:  False
I/mono-stdout( 2690):                 at MonoTests.System.Net.Sockets.SocketTest.SendAsyncFile () [0x000f1] in <dbafb5e444f34ec0a42ddccd79e1c9b6>:0 
I/mono-stdout( 2690):                 at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
I/mono-stdout( 2690):                 at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/corlib/System.Reflection/MonoMethod.cs:305 
...
E/mono    ( 2690): Unhandled Exception:
E/mono    ( 2690): System.ObjectDisposedException: Cannot access a disposed object.
E/mono    ( 2690): Object name: 'System.Net.Sockets.Socket'.
E/mono    ( 2690):   at System.Net.Sockets.Socket.ThrowIfDisposedAndClosed () [0x00021] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/System/System.Net.Sockets/Socket.cs:2622 
E/mono    ( 2690):   at System.Net.Sockets.Socket.EndSendFile (System.IAsyncResult asyncResult) [0x00000] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/System/System.Net.Sockets/Socket.cs:2188 
E/mono    ( 2690):   at MonoTests.System.Net.Sockets.SocketTest+<>c__DisplayClass243_1.<SendAsyncFile>b__1 (System.IAsyncResult ar) [0x00006] in <dbafb5e444f34ec0a42ddccd79e1c9b6>:0 
E/mono    ( 2690):   at System.Net.Sockets.Socket+<>c__DisplayClass259_0.<BeginSendFile>b__0 (System.IAsyncResult ar) [0x00000] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/System/System.Net.Sockets/Socket.cs:2183 
E/mono    ( 2690):   at (wrapper managed-to-native) System.Runtime.Remoting.Messaging.AsyncResult:Invoke (System.Runtime.Remoting.Messaging.AsyncResult)
E/mono    ( 2690):   at System.Runtime.Remoting.Messaging.AsyncResult.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem () [0x00000] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/corlib/System.Runtime.Remoting.Messaging/AsyncResult.cs:210 
E/mono    ( 2690):   at System.Threading.ThreadPoolWorkQueue.Dispatch () [0x00074] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/referencesource/mscorlib/system/threading/threadpool.cs:856 
E/mono    ( 2690):   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback () [0x00000] in /Users/builder/jenkins/workspace/xamarin-android/xamarin-android/external/mono/mcs/class/referencesource/mscorlib/system/threading/threadpool.cs:1211

Retrying the build. Let's see how consistent this is.

radekdoulik · 2017-10-14T09:59:15Z

It breaks timing and sizes measurements. But it is not fault of this commit as it was using ResultsPath, which is changed now. I will create a PR next week for that, where the timing and sizes measurement will not depend on ResultsPath anymore.

Changes: dotnet/java-interop@7f1a5ab...aac3e9a * dotnet/java-interop@aac3e9ac: [Java.Interop, Java.Base] Fix xamarin-android integration issues (#929) * dotnet/java-interop@bc5bcf4f: [Java.Base] Begin binding JDK-11 java.base module (#909) * dotnet/java-interop@af91b9c2: [ci] Use descriptive test run titles. (#926) * dotnet/java-interop@aae8f251: [Java.Interop.Tools.Generator] Add some enumification helper methods (#866) * dotnet/java-interop@111ebca8: [Java.Interop] Treat warnings as errors. (#925) * dotnet/java-interop@7a32bb97: [java-source-utils] Transform XML using XSLT (#924) * dotnet/java-interop@7f55b2dc: [logcat-parse] Use C# verbatim strings for paths (#927) * dotnet/java-interop@2601146b: [java-source-utils] Fix regresion from 77c9c5fa (#923) (So many inadvertent issues as part of this bump…) dotnet/java-interop@bc5bcf4f added an *altered* `Java.Interop.JavaTypeParametersAttribute` custom attribute, altered to "appease" various Code Analysis warnings such as [CA1813][0] "Avoid unsealed attributes". This introduction caused some unit tests to fail to build: error CS0433: The type 'JavaTypeParametersAttribute' exists in both 'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' [C:\…\temp\LibraryProjectZipWithLint\BindingsProject\BindingsProject.csproj] The original idea had been to use [type forwarders][1], so that `Java.Interop.JavaTypeParametersAttribute` from `Mono.Android.dll` would be forwarded to the same type in `Java.Interop.dll`. This idea was rejected after further consideration, as it would likely break [forward compatibility][2] (dotnet/java-interop@e56a8c8e). dotnet/java-interop@aac3e9ac fixes this by making `JavaTypeParametersAttribute` conditional on `NET`, causing the type to only be present when targeting .NET 6+. `Mono.Android.dll` for .NET 6 -- and *not* Classic Xamarin.Android -- can then use a type forwarder. This can be done because there's no forward compatibility constraint on .NET 6. However, by making `JavaTypeParametersAttribute` conditional on `NET`, the `_CheckApiCompatibility` target started failing -- rightfully! -- because the `JavaTypeParametersAttribute` type had been removed. "Worse", there was no way to accept this change in e.g. `tests/api-compatibility/acceptable-breakages-vReference.txt`, because the `<CheckApiCompatibility/>` task requires that there be no extra entries. Square this circle by adding a `CheckApiCompatibility.TargetFramework` property, and looking for `acceptable-breakages-*-{TargetFramework}.txt` *before* looking for `acceptable-breakages-*.txt`. This allows us to special-case the acceptable breakages based on `$(TargetFramework)`. Finally, update `azure-pipelines.yaml` so that the `java-source-utils` unit tests are *not* run on Windows. This is because they currently fail on Windows, which is something we'll worry about later. For now, disable the `java-source-utils` tests when building on Windows. [0]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1813 [1]: https://docs.microsoft.com/en-us/dotnet/standard/assembly/type-forwarding [2]: https://en.wikipedia.org/wiki/Forward_compatibility

dnfclas added the cla-already-signed label Oct 10, 2017

jonpryor requested review from dellis1972, grendello and radekdoulik October 10, 2017 22:31

jonpryor added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Oct 10, 2017

jonpryor force-pushed the jonp-release-tests branch 9 times, most recently from aa94397 to d836d8b Compare October 12, 2017 02:44

jonpryor added the do-not-merge PR should not be merged. label Oct 12, 2017

jonpryor force-pushed the jonp-release-tests branch 2 times, most recently from 9292443 to d6f6f99 Compare October 13, 2017 14:39

jonpryor changed the title ~~[tests] Distinguish tests between Configurations~~ [tests] Add Configuration info to test names Oct 13, 2017

grendello approved these changes Oct 13, 2017

View reviewed changes

jonpryor force-pushed the jonp-release-tests branch from d6f6f99 to 3be03a0 Compare October 13, 2017 19:55

jonpryor removed the do-not-merge PR should not be merged. label Oct 13, 2017

jonpryor force-pushed the jonp-release-tests branch from 3be03a0 to cf72b0d Compare October 13, 2017 23:09

radekdoulik merged commit 385699a into dotnet:master Oct 14, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[tests] Add Configuration info to test names #929

[tests] Add Configuration info to test names #929

Uh oh!

jonpryor commented Oct 10, 2017 •

edited

Loading

Uh oh!

jonpryor commented Oct 10, 2017

Uh oh!

jonpryor commented Oct 14, 2017

Uh oh!

radekdoulik commented Oct 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[tests] Add Configuration info to test names #929

[tests] Add Configuration info to test names #929

Uh oh!

Conversation

jonpryor commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor commented Oct 10, 2017

Uh oh!

jonpryor commented Oct 14, 2017

Uh oh!

radekdoulik commented Oct 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonpryor commented Oct 10, 2017 •

edited

Loading