Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use custom mobile testing API #53871

Merged
merged 9 commits into from
Jun 10, 2021
Merged

Use custom mobile testing API #53871

merged 9 commits into from
Jun 10, 2021

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Jun 8, 2021 •
edited
Loading

This PR mainly fixe these two areas:

  1. Use Collection Fixtures syntax for runtime tests generation script, when target is mobile. And invoke app installation/uninstallation at shared fixture class.
  2. Use custom mobile testing API's to install/uninstall mobile apps once per XUnitWrapper

Fixes #53382
Contribute to #45568

@ghost
Copy link

ghost commented Jun 8, 2021

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR mainly fixe these two areas:

  1. Update runtime tests generation script to use proper xunit Collection Fixtures syntax
  2. Use custom mobile testing API's to install/uninstall mobile apps once per XUnitWrapper
Author: fanyang-mono
Assignees: -
Labels:

area-Infrastructure-coreclr
Milestone: -

@fanyang-mono fanyang-mono requested a review from trylek June 8, 2021 14:59
@premun
Copy link
Member

premun commented Jun 8, 2021

This should improve things, however, I am a bit worried what happens when timeouts occur. We won't be able to uninstall in those cases?

@fanyang-mono
Copy link
Member Author

This should improve things, however, I am a bit worried what happens when timeouts occur. We won't be able to uninstall in those cases?

The Dispose() function should be called when the fixture object gets destroyed.

@premun
Copy link
Member

premun commented Jun 8, 2021

But I presume that when Helix kills the job and it shuts down the dotnet xunit.dll ... process, we won't have that?

@fanyang-mono
Copy link
Member Author

But I presume that when Helix kills the job and it shuts down the dotnet xunit.dll ... process, we won't have that?

That will leave all the artifacts that tests create on the device, not just the app. To be 100% sure that tests will be running against the newest version of the app, I could call the uninstall function right before install function. Would that resolve your concern?

@premun
Copy link
Member

premun commented Jun 8, 2021

That will leave all the artifacts that tests create on the device, not just the app. To be 100% sure that tests will be running against the newest version of the app, I could call the uninstall function right before install function. Would that resolve your concern?

@greenEkatherine I think we already do uninstall before install, right?

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Jun 8, 2021 •
edited
Loading

That will leave all the artifacts that tests create on the device, not just the app. To be 100% sure that tests will be running against the newest version of the app, I could call the uninstall function right before install function. Would that resolve your concern?

@greenEkatherine I think we already do uninstall before install, right?

According to the log, it does try to uninstall the same app before installing it. Here is an example

XHarness command issued: android install --package-name=net.dot.tracing_eventcounter --app=/datadisks/disk1/work/A282089B/w/B61609C3/e/tracing/eventcounter/tracing_eventcounter.apk --output-directory=/datadisks/disk1/work/A282089B/w/B61609C3/uploads/Reports/install
[15:49:38] info: Will attempt to run device on detected architecture: 'x86_64'
[15:49:38] info: Active Android device set to serial 'emulator-5556'
[15:49:38] info: Waiting for device to be available (max 5 minutes)
[15:49:38] info: Attempting to remove apk 'net.dot.tracing_eventcounter': 
[15:49:38] info: APK 'net.dot.tracing_eventcounter' not on device.
[15:49:38] info: Attempting to install /datadisks/disk1/work/A282089B/w/B61609C3/e/tracing/eventcounter/tracing_eventcounter.apk: 
[15:49:38] info: Successfully installed /datadisks/disk1/work/A282089B/w/B61609C3/e/tracing/eventcounter/tracing_eventcounter.apk.
[15:49:38] info: Killing all running processes for 'net.dot.tracing_eventcounter': 
XHarness exit code: 0

Both the installation and uninstallation logs are saved at HELIX_WORKITEM_UPLOAD_ROOT for future possible investigation after CI runs.

I also noticed that the android test command would install the app, if it couldn't find the app. That is a surprise to me. It is quite convenient. But I would rather it fails the test, so that I know the app failed to install before tests started running. Otherwise, it could become long running tests silently.

@greenEkatherine
Copy link
Contributor

I also noticed that the android test command would install the app, if it couldn't find the app. That is a surprise to me. It is quite convenient. But I would rather it fails the test, so that I know the app failed to install before tests started running. Otherwise, it could become long running tests silently.

@fanyang-mono in that case you need android run - it doesn't install anything

@fanyang-mono
Copy link
Member Author

I also noticed that the android test command would install the app, if it couldn't find the app. That is a surprise to me. It is quite convenient. But I would rather it fails the test, so that I know the app failed to install before tests started running. Otherwise, it could become long running tests silently.

@fanyang-mono in that case you need android run - it doesn't install anything

Sorry I meant to say android run. It still does installation, when the app is not there...

@greenEkatherine
Copy link
Contributor

Sorry I meant to say android run. It still does installation, when the app is not there...

I double checked code - it shouldn't be a case. We don't even provide apk to install in that case, only package name. Could you please share a link to some build for example?

@fanyang-mono
Copy link
Member Author

Sorry I meant to say android run. It still does installation, when the app is not there...

I double checked code - it shouldn't be a case. We don't even provide apk to install in that case, only package name. Could you please share a link to some build for example?

I only tried it locally. Will share with you the long.

@fanyang-mono
Copy link
Member Author

Sorry I meant to say android run. It still does installation, when the app is not there...

I double checked code - it shouldn't be a case. We don't even provide apk to install in that case, only package name. Could you please share a link to some build for example?

@greenEkatherine You are right. android run doesn't install app's. Sorry about the confusion.

@fanyang-mono
Copy link
Member Author

Converting tests to use Collection Fixtures makes runtime tests running much slower than before, except for the Android lane. I am investigating it now.

@runfoapp runfoapp bot mentioned this pull request Jun 9, 2021
Open
@fanyang-mono
Copy link
Member Author

With this change, the time spent by runtime tests running on Android x64 has dropped more than 16% (from 2h to 1h40min).

premun
premun reviewed Jun 10, 2021
View reviewed changes
src/tests/Common/Coreclr.TestWrapper/MobileAppHandler.cs
string xharnessCmd;
string cmdStr;
string appExtension;
int timeout = 240000; // Set timeout to 4 mins, because the installation on Android arm64/32 devices could take up to 4 mins on CI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greenEkatherine are these 4 minutes valid?

fanyang-mono and others added 4 commits June 10, 2021 09:22
@fanyang-mono @premun
Update src/tests/Common/Coreclr.TestWrapper/MobileAppHandler.cs
69a8156 
Co-authored-by: Přemek Vysoký <premek.vysoky@microsoft.com>
@fanyang-mono @premun
Update src/tests/Common/Coreclr.TestWrapper/MobileAppHandler.cs
a6f69a4 
Co-authored-by: Přemek Vysoký <premek.vysoky@microsoft.com>
@fanyang-mono @premun
Update src/tests/run.proj
f0ce338 
Co-authored-by: Přemek Vysoký <premek.vysoky@microsoft.com>
@fanyang-mono
Fix the ios app extension
c1645d2
@fanyang-mono
Copy link
Member Author

Converting tests to use Collection Fixtures makes runtime tests running much slower than before, except for the Android lane. I am investigating it now.

The change of this PR only applies to mobile targets, so the runtime tests lanes on non-mobile targets are back to previous speed.

trylek
trylek approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

@fanyang-mono fanyang-mono merged commit e4751ae into dotnet:main Jun 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@premun premun premun left review comments

@trylek trylek trylek approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@greenEkatherine greenEkatherine Awaiting requested review from greenEkatherine

@imhameed imhameed Awaiting requested review from imhameed

Assignees
No one assigned
Labels
area-Infrastructure-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Mono][Android][test] Use xharness custom API's to run Android tests
4 participants
@fanyang-mono @premun @greenEkatherine @trylek