Skip to content
New issue

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

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

Already on GitHub? # to your account

[Android] Run CoreCLR functional tests on Android #112283

Merged

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Feb 7, 2025

Description

This PR introduces a new job to runtime and runtime-extra-platforms for running CoreCLR functional tests on the Android emulator.

@kotlarmilos kotlarmilos added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it area-Infrastructure-coreclr os-android labels Feb 7, 2025
@kotlarmilos kotlarmilos added this to the 10.0.0 milestone Feb 7, 2025
@kotlarmilos kotlarmilos self-assigned this Feb 7, 2025
Copy link
Contributor

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

@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos kotlarmilos marked this pull request as ready for review February 10, 2025 16:38
@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

@kotlarmilos kotlarmilos removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Feb 10, 2025
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

LGTM! I left couple of minor notes, thanks!

jobParameters:
testGroup: innerloop
nameSuffix: AllSubsets_CoreCLR
buildArgs: -s clr.runtime+clr.alljits+clr.corelib+clr.nativecorelib+clr.tools+clr.packages+libs+libs.tests -c $(_BuildConfig) /p:ArchiveTests=true /p:RunSmokeTestsOnly=true
Copy link
Member

Choose a reason for hiding this comment

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

Should we also include host+packs subsets?

@@ -723,7 +723,7 @@
BuildInParallel="false" />
</ItemGroup>

<ItemGroup Condition="'$(ArchiveTests)' == 'true' and '$(RunSmokeTestsOnly)' != 'true' and '$(RunGrpcTestsOnly)' != 'true' and '$(TargetOS)' == 'android'">
<ItemGroup Condition="'$(ArchiveTests)' == 'true' and '$(RunSmokeTestsOnly)' != 'true' and '$(RunGrpcTestsOnly)' != 'true' and '$(TargetOS)' == 'android' and '$(RuntimeFlavor)' == 'Mono'">
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are adding the condition to separate mono and coreclr ProjectReference.

@@ -759,6 +759,12 @@
AdditionalProperties="%(AdditionalProperties);_IsPublishing=true;Configuration=Release;PublishDir=$(XUnitLogCheckerLibrariesOutDir);ROOTFS_DIR=$(ROOTFS_DIR);RuntimeIdentifier=$(OutputRID)" />
</ItemGroup>

<ItemGroup Condition="'$(ArchiveTests)' == 'true' and '$(TargetOS)' == 'android' and '$(RuntimeFlavor)' == 'CoreCLR'">
<ProjectReference Remove="@(ProjectReference)" />
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is needed since we remove the '$(RuntimeFlavor)' == 'Mono'" condition above, if we kept the condition above, could we just include the Android.Device_Emulator.JIT.Test.csproj here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests are included by default in:

<ProjectReference Include="$(MSBuildThisFileDirectory)*\tests\**\*.Tests.csproj"
                      Exclude="@(ProjectExclusions)"
                      Condition="'$(TestAssemblies)' == 'true'"
                      BuildInParallel="$(BuildTestInParallel)" />

To exclude the default set of tests, we set TestAssemblies to false.

@kotlarmilos
Copy link
Member Author

/azp run runtime-androidemulator

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos kotlarmilos merged commit 7be653f into dotnet:main Feb 12, 2025
150 of 158 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Feb 12, 2025
* main:
  [Android] Run CoreCLR functional tests on Android (dotnet#112283)
  [LoongArch64] Fix some assertion failures for Debug ILC building Debug NativeAOT testcases. (dotnet#112229)
  Fix suspicious code fragments (dotnet#112384)
  `__ComObject` doesn't support dynamic interface map (dotnet#112375)
  Native DLLs: only load imported DLLs from System32 (dotnet#112359)
  [main] Update dependencies from dotnet/roslyn (dotnet#112314)
  Update SVE instructions that writes to GC regs (dotnet#112389)
  Bring up android+coreclr windows build.  (dotnet#112256)
  Never use heap for return buffers (dotnet#112060)
  Wait to complete the test before releasing the agile reference. (dotnet#112387)
  Prevent returning disposed HTTP/1.1 connections to the pool (dotnet#112383)
  Fingerprint dotnet.js if writing import map to html is enabled (dotnet#112407)
  Remove duplicate definition of CORECLR_HOSTING_API_LINKAGE (dotnet#112096)
  Update the exception message to reflect current behavior. (dotnet#112355)
  Use enum for frametype not v table (dotnet#112166)
  Enable AltJits build for LoongArch64 and RiscV64 (dotnet#110282)
  Guard members of MonoType union & fix related bugs (dotnet#111645)
  Add optional hooks for debugging OpenSSL memory allocations (dotnet#111539)
  JIT: Optimize struct parameter register accesses in the backend (dotnet#110819)
  NativeAOT: Cover more opcodes in type preinitializer (dotnet#112073)
Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

I'm very sorry for the late review, I was out of office and missed this PR when I came back.

Comment on lines +133 to +134
- android_x64
- android_arm64
Copy link
Member

Choose a reason for hiding this comment

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

Do we have arm64 emulators setup? I thought that by default these will go to devices, see

- ${{ if in(parameters.platform, 'android_arm', 'android_arm64', 'linux_bionic_arm', 'linux_bionic_arm64') }}:
- Windows.11.Amd64.Android.Open

Comparing the logs between x64 and am64, I would say that the arm64 are run on devices https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-112283-merge-8bd807b9687245838c/Android.Device_Emulator.JIT.Test/1/console.36c43a9c.log?helixlogtype=result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it seems that arm64 run on devices so we can move it to extra-platforms-android.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also wondering, if we have the same runs in runtime.yml (which runs always), why we need them again in extra-platforms?

I think we can have just runtime.yml for now and when we include full libraries run we should move that to extra-platforms.


#
# Android emulators
# Build the whole product using CoreCLR and run libraries tests
Copy link
Member

Choose a reason for hiding this comment

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

Are these library tests? I thought that functional tests are usually under the runtime test suit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Functional tests are under libraries. There might be smoke tests under the runtime tests.

Copy link
Member

@matouskozak matouskozak Feb 12, 2025

Choose a reason for hiding this comment

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

Ah, I thought it was the reverse. The https://github.com/dotnet/runtime/blob/main/src/tests/FunctionalTests/Android/Device_Emulator/JIT/Android.Device_Emulator.JIT.Test.csproj is placed under src/tests so I got mistaken that it is part of runtime tests.

In case of smoke tests, I think these run functional tests and System.Runtime.Tests which comes from libraries.

runtimeFlavor: coreclr
platforms:
- android_x64
- android_arm64
Copy link
Member

Choose a reason for hiding this comment

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

If these indeed run on devices (see my other comment), we need to make sure we have enough devices so that we can run them on PRs by default and not block other Android jobs.

Copy link
Member Author

Choose a reason for hiding this comment

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

How can we test the load?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should monitor the Windows.11.Amd64.Android.Open queue in Grafana to see that the waiting times don't grow so that we don't slow down runtime pipeline. I'm not aware of any stress we have for this.

@@ -964,6 +964,47 @@ extends:
eq(variables['monoContainsChange'], true),
eq(variables['isRollingBuild'], true))

#
# Android devices
Copy link
Member

Choose a reason for hiding this comment

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

Devices or emulators?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be x64 emulator and arm64 device.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants