Support custom commands in Android XHarness SDK #7582

premun · 2021-07-01T13:03:24Z

Adds support for <CustomCommands> in Android flows
Changes Android windows payload script from .bat to .ps1
Allows to re-use 1 app for multiple work items but different inputs (and specify work item name)
Adds tests for custom commands
De-duplicates common code unit test projects
Makes unit tests work in non-ci runs

premun · 2021-07-01T13:04:58Z

@fanyang-mono this PR introduces the proper way how we will need to create XHarness Helix jobs in Android/Apple (Apple is ready for some time).

The reason for that is that it will enable us to react to infrastructural issues, do retries and log build telemetry correctly.

I can assist with introducing this for the runtime tests.

The README describes how it's intended to be used:
https://github.com/dotnet/arcade/blob/f2e84e9577e49e3046c215d74e7e4bb499a1a74f/src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/Readme.md

premun · 2021-07-01T13:13:24Z

@imhameed @fanyang-mono @SamMonoRT this PR also adds a new capability - we can now re-use 1 application for multiple work items and have each work item for example call the app with different commands (but in parallel on multiple machines) so just an FYI in case this would help you in some scenarios where for example some tests take a long time and you want to split them.

fanyang-mono · 2021-07-01T14:33:28Z

Nice improvement! However, I am not sure how the current runtime tests infrastructure would be able to leverage this. For runtime tests, the xunit tests themselves are running on host. The tests then calls to bash or powershell scripts. Inside the bash or powershell script, the xharness test command gets invoked. If I understand this feature correctly, the runtime test infrastructure would need to be rewrite like how the library tests are, in order to adopt this feature.

premun · 2021-07-01T14:39:11Z

@fanyang-mono it's sort of also not a choice to use it. We will require it so that the Helix mobile device infrastructure is actually maintainable for us. We already spoke about this with Imran and Sam.
Another reason for this is we will start adding monitoring around XHarness jobs and alerting around failing devices and such so we will require everyone using the devices to go through the XHarness SDK.
We will also add telemetry so that we can measure infrastructural failures.
We also add retries in case we recognize a failing device so there's bonuses for you too.

And all these are inside of the XHarness SDK wrapper which changes quite often as we find new way to improve stability.

Also please consider that we have to maintain a good state of all devices so that a bad runtime test won't affect other customers of mobile devices.

However, it can be as simple as not creating the <HelixWorkItem> with the dotnet xUnit.dll ... Helix command but creating <XHarnessApkToTest> instead with the <CustomCommand> being set to dotnet xUnit.dll ... so the change technically doesn't have to be large.

fanyang-mono · 2021-07-01T15:03:12Z

@premun Yes, I understand that we need to make it better and easier to monitor and maintain the devices. If the change is simple as you said, we could give it a try to see what happens. When are you targeting for us to adopt this feature?

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/xharness-helix-job.android.ps1

tests/UnitTests.XHarness.Android.Simulator.proj

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/xharness-helix-job.android.ps1

…nds-android

premun · 2021-07-07T13:01:26Z

@MattGal could you have a look at this PR please? Thanks!

SamMonoRT · 2021-07-07T19:51:21Z

@imhameed @fanyang-mono @SamMonoRT this PR also adds a new capability - we can now re-use 1 application for multiple work items and have each work item for example call the app with different commands (but in parallel on multiple machines) so just an FYI in case this would help you in some scenarios where for example some tests take a long time and you want to split them.

Do you plan to update the existing runtime and runtime staging lanes for devices too ?

MattGal

Seems reasonable, I'd like to chat about the work item payload construction (mostly the recreation of zips over and over) before this merges.

src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs

src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/Readme.md

MattGal · 2021-07-08T00:57:24Z

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/Readme.md

+  <AndroidInstrumentationName>net.dot.MonoRunner</AndroidInstrumentationName>
+</XHarnessApkToTest>
+
+<XHarnessApkToTest Include="System.Text.Json-with-custom-commands">


Now that I've read this far, I think it's likely this will delete and re-zip the same APK payload over and over.

It will create new archive for each work item and each archive will contain the APK plus the custom payload scripts created for the work item.

For example in this example, the command.sh file will differ in the archives.

I don't think this is such a big problem, considering we're doing this in parallel. It would be a bit more difficult to synchronize this between processes and create one base archive and then clone it and inject the command.sh so I am not sure it is worth the effort?

I still think it is worth the effort to upload once and reuse work item payloads with this feature. The biggest costs we're seeing lately are from storage (@ulisesh was just showing me this) and the XHarness on-prem testing seems correlated to the biggest increases in spending.

Since you're using these as work item payloads, the bandwidth costs should be roughly the same making N payloads for 1 app, but the storage will rise because users are fully within their rights to use 1 APK for 400 tests now. Additionally, the time sink of uploading the same thing potentially hundreds of times will also be noticeable when prepping the payloads in these scenarios. I'm not blocking the PR, just noting this for future consideration.

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/xharness-helix-job.android.ps1

…nds-android

premun · 2021-07-08T15:58:21Z

@SamMonoRT

Do you plan to update the existing runtime and runtime staging lanes for devices too ?

I am not sure what you mean?

SamMonoRT · 2021-07-08T16:00:33Z

@SamMonoRT

Do you plan to update the existing runtime and runtime staging lanes for devices too ?

I am not sure what you mean?

Is there any followup work needed for individual device CI lanes or will this change in SDK just flow down without required changes in yaml scripts ?

premun · 2021-07-08T16:13:04Z

@SamMonoRT
Is there any followup work needed for individual device CI lanes or will this change in SDK just flow down without required changes in yaml scripts ?

We need to check this in, let Maestro bump Arcade in runtime and then this is the state we're in I believe:

iOS is still in PR (CoreCLR runtime tests + Mono on the x64 iOS simulator runtime#43954)
Android is already checked in
Both need to start using the <CustomCommands> feature
- Android is less important at the moment because there is not that much going on in the wrapper scripts
- iOS should not be checked in without this (we spoke about this)
iOS still has perf issues if I understand it correctly as the tests just take too long and one of the work items still needs 4 hours to run so there's more work on the iOS side too
This change actually allows to split the long running work item into several smaller work items
I understand that the approach taken by @fanyang-mono and @imhameed was now unified (I see @imhameed using MobileAppHandler) so technically the change can happen for Android and @imhameed can then merge it into his PR and start using it too?
I spoke with @fanyang-mono about the change to <CustomCommands> briefly, however, I am off next week and partially the one after. There are docs around the feature (the Readme in this PR) and conceptually it is totally doable however I am not sure how complex the MSBuild wrapper runtime tests around the Helix SDK have are

SamMonoRT · 2021-07-08T17:26:31Z

@SamMonoRT
Is there any followup work needed for individual device CI lanes or will this change in SDK just flow down without required changes in yaml scripts ?

We need to check this in, let Maestro bump Arcade in runtime and then this is the state we're in I believe:

iOS is still in PR (CoreCLR runtime tests + Mono on the x64 iOS simulator runtime#43954)

Android is already checked in

Both need to start using the <CustomCommands> feature

Android is less important at the moment because there is not that much going on in the wrapper scripts

iOS should not be checked in without this (we spoke about this)

iOS still has perf issues if I understand it correctly as the tests just take too long and one of the work items still needs 4 hours to run so there's more work on the iOS side too

This change actually allows to split the long running work item into several smaller work items

I understand that the approach taken by @fanyang-mono and @imhameed was now unified (I see @imhameed using MobileAppHandler) so technically the change can happen for Android and @imhameed can then merge it into his PR and start using it too?

I spoke with @fanyang-mono about the change to <CustomCommands> briefly, however, I am off next week and partially the one after. There are docs around the feature (the Readme in this PR) and conceptually it is totally doable however I am not sure how complex the MSBuild wrapper runtime tests around the Helix SDK have are

Thanks for detailing the changes required. For iOS we are targetting getting this in by next Tuesday (Preview 7 cutoff). Will this land prior to that so iOS can leverage this goodness of smaller workitems.

premun · 2021-07-09T07:54:06Z

@SamMonoRT I only need this reviewed but seems like @MattGal is off. If you need this in, I could check it in and work through his potential feedback in retrospective

…nds-android

SamMonoRT · 2021-07-09T12:37:19Z

@SamMonoRT I only need this reviewed but seems like @MattGal is off. If you need this in, I could check it in and work through his potential feedback in retrospective

Let's wait for the right approvals. Hopefully we have it in early next week.

tests/XHarness/XHarness.TestApks.proj

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/xharness-helix-job.android.sh

…nds-android

premun · 2021-07-20T14:12:54Z

/azp run

azure-pipelines · 2021-07-20T14:13:06Z

Azure Pipelines successfully started running 1 pipeline(s).

...DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/CreateXHarnessAppleWorkItemsTests.cs

MattGal

Changes look good to me. I have some misgivings about making distinct work item payloads for the same app package but it's really hard to grasp the final cost until it's in use and working correctly.

src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs

MattGal · 2021-07-20T15:16:34Z

src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs

            ITaskItem appBundleItem)
        {
-            string appFolderPath = appBundleItem.ItemSpec.TrimEnd(Path.DirectorySeparatorChar);
+            // The user can re-use the same .apk for 2 work items so the name of the work item will come from ItemSpec and path from metadata


Can't tell, does this make use of the payload reuse behavior (so upload once, download multiple times)?

MattGal · 2021-07-20T15:18:42Z

src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs

+            "--xcode \"$xcode_path\" " +
+            "-v " +
+            (includesTestRunner
+                ? $"--launch-timeout \"$launch_timeout\" "


Another place where an interpolated string can prevent errors in future changes.

MattGal · 2021-07-20T15:26:58Z

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/Readme.md

+  <AndroidInstrumentationName>net.dot.MonoRunner</AndroidInstrumentationName>
+</XHarnessApkToTest>
+
+<XHarnessApkToTest Include="System.Text.Json-with-custom-commands">


I still think it is worth the effort to upload once and reuse work item payloads with this feature. The biggest costs we're seeing lately are from storage (@ulisesh was just showing me this) and the XHarness on-prem testing seems correlated to the biggest increases in spending.

Since you're using these as work item payloads, the bandwidth costs should be roughly the same making N payloads for 1 app, but the storage will rise because users are fully within their rights to use 1 APK for 400 tests now. Additionally, the time sink of uploading the same thing potentially hundreds of times will also be noticeable when prepping the payloads in these scenarios. I'm not blocking the PR, just noting this for future consideration.

premun added 7 commits July 1, 2021 10:55

Add support for CustomCommands in Android XHarness SDK

f17a217

Rename unit test projects to match Apple

29c81d9

Fix bugs in sh/ps

3dcb76d

Update Readme.md

0221fd1

Allow re-using of apps for multiple workitems

f258a14

Test custom commands in Arcade PR

9082b30

Deduplicate code

f2e84e9

premun requested review from MattGal, greenEkatherine and lpatalas July 1, 2021 13:03

Fix unit tests

7e25174

Fix calling xharness in Apple

d5aa0c0

lpatalas reviewed Jul 1, 2021

View reviewed changes

premun added 6 commits July 7, 2021 12:32

Merge remote-tracking branch 'origin/main' into prvysoky/custom-comma…

87aed9d

…nds-android

Address PowerShell feedback

0963d57

Fix PS parameter declaration

0ac8ac5

--instrumentation now accepts value always

57e2f87

Lowercase the archive names

d0b892d

Suppress PS warning

bb606fe

MattGal reviewed Jul 8, 2021

View reviewed changes

premun added 2 commits July 8, 2021 10:45

Address PR feedback

ebbd9af

Merge remote-tracking branch 'origin/main' into prvysoky/custom-comma…

931b94e

…nds-android

premun added 2 commits July 9, 2021 10:43

Add unit tests

80f8c53

Merge remote-tracking branch 'origin/main' into prvysoky/custom-comma…

57fd232

…nds-android

greenEkatherine approved these changes Jul 9, 2021

View reviewed changes

tests/XHarness/XHarness.TestApks.proj Outdated Show resolved Hide resolved

src/Microsoft.DotNet.Helix/Sdk/tools/xharness-runner/xharness-helix-job.android.sh Show resolved Hide resolved

premun added 2 commits July 20, 2021 15:17

Merge remote-tracking branch 'origin/main' into prvysoky/custom-comma…

a3ac613

…nds-android

Use $variables to have more coverage

7a6eab6

MattGal reviewed Jul 20, 2021

View reviewed changes

...DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/CreateXHarnessAppleWorkItemsTests.cs Show resolved Hide resolved

MattGal approved these changes Jul 20, 2021

View reviewed changes

premun added 2 commits July 21, 2021 12:55

Use string interpolation to prevent missing spaces

a3bdfa2

Fix retry/reboot on Android windows

5c6e9d6

premun merged commit dc5deb1 into dotnet:main Jul 21, 2021

premun deleted the prvysoky/custom-commands-android branch July 21, 2021 13:54

danmoseley mentioned this pull request Feb 7, 2022

Add RuntimeConfiguration.NotRelease danmoseley/arcade#1

Closed

1 task

Support custom commands in Android XHarness SDK #7582

Support custom commands in Android XHarness SDK #7582

Uh oh!

Conversation

premun commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

premun commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

premun commented Jul 1, 2021

Uh oh!

fanyang-mono commented Jul 1, 2021

Uh oh!

premun commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanyang-mono commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

premun commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamMonoRT commented Jul 7, 2021

Uh oh!

MattGal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MattGal Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

premun Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

MattGal Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

premun commented Jul 8, 2021

Uh oh!

SamMonoRT commented Jul 8, 2021

Uh oh!

premun commented Jul 8, 2021

Uh oh!

SamMonoRT commented Jul 8, 2021

Uh oh!

premun commented Jul 9, 2021

Uh oh!

SamMonoRT commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

premun commented Jul 20, 2021

Uh oh!

azure-pipelines bot commented Jul 20, 2021

Uh oh!

Uh oh!

MattGal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MattGal Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

MattGal Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

MattGal Jul 20, 2021

premun commented Jul 1, 2021 •

edited

Loading

premun commented Jul 1, 2021 •

edited

Loading

premun commented Jul 1, 2021 •

edited

Loading

fanyang-mono commented Jul 1, 2021 •

edited

Loading

premun commented Jul 7, 2021 •

edited

Loading