[WIP] idea to speed up Xamarin.Android.Build.Tests

[jonathanpeppers]

I wanted to send this in to discuss next week.

The current situation

Currently the Xamarin.Android.Build.Tests are doing the following:

• Running msbuild w/ verbose logging to file and /noconsolelogger
• Combining stdout and stderror into a large string named LastBuildOutput
• Many tests are asserting against the text in LastBuildOutput
• There is also some code that opens the log file on disk to find the MSBuild build duration

So a couple issues from this:

• We use a lot of memory here, with potentially enormous strings, some of it is duplicated strings
• Asserting against LastBuildOutput is doing a string.Contains on potentially massive strings
• The test BuildAMassiveApp always fails on Windows with OutOfMemoryException
• On Windows, our Process.Start/WaitForExit implementation can hang. If the stdout or stderror buffer gets full, when WaitForExit hangs with the 10 minute timeout. I see this happening mostly if a test fails (because the output is bigger). Link about this here.

I'm sure this code evolved over time--it probably wasn't planned to be implemented this way.

An idea

The "idea" is we could instead:

• Run msbuild with verbose logging to the console (not to file at all)
• Use the OutputDataReceived and ErrorDataReceived events to assert against MSBuild output as it streams through
• Append this output to a file for failures later if needed

The problem with doing this, is you would have to setup assertions before running the build. Then checking if they all passed after the build. This is a bit confusing, and a juxtaposition of the Arrange, Act, Assert pattern people think about when writing tests.

The initial results

I did a quick implementation of this idea, and compared the results of the following test class with 44 tests (on Mac):

make run-nunit-tests TEST=Xamarin.Android.Build.Tests.AndroidUpdateResourcesTest

The results on master:

Test Run Summary
  Overall result: Warning
  Test Count: 44, Passed: 35, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 9
    Skipped Tests - Ignored: 9, Explicit: 0, Other: 0
  Start time: 2017-11-03 16:46:54Z
    End time: 2017-11-03 16:54:49Z
    Duration: 474.909 seconds

The results with this PR:

Test Run Summary
  Overall result: Failed
  Test Count: 44, Passed: 30, Failed: 5, Warnings: 0, Inconclusive: 0, Skipped: 9
    Failed Tests - Failures: 5, Errors: 0, Invalid: 0
    Skipped Tests - Ignored: 9, Explicit: 0, Other: 0
  Start time: 2017-11-03 20:04:41Z
    End time: 2017-11-03 20:11:00Z
    Duration: 379.545 seconds

There are still some failures I'll look into fixing here. These failures seemed to be running the full test (they aren't failing and taking less than <10ms that would influence the shorter time).

This boils down to maybe a 20% speed increase of these tests. These tests are currently taking about 1 hour on Jenkins PRs.

Drawbacks

This seems like a decent amount of work--and the not very fun kind. It also seems scary that we could mistakenly change a test to make it always pass.

Investing in this, while it would improve quality & build times, doesn't bring new features to Xamarin.Android.

[dnfclas]

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

[jonathanpeppers]

Ignore this, this is something else to figure out about these tests on Windows.

[jonathanpeppers]

This PR leaves a couple methods that would be removed if we moved forward with this.

I left them here with NotImplementedException so it would compile.

[jonathanpeppers]

I think we need to switch to the OutputDataReceived and ErrorDataReceived events to fix Windows hanging on WaitForExit. Dean should have a PR for this from our team meeting last week.

The other stuff in here related to changing assertions seems like it is too much work to port across all the tests.