-
Notifications
You must be signed in to change notification settings - Fork 564
[tests] run runtime test in Release configuration and Release/AOT #748
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.
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
[tests] run runtime test in Release configuration and Release/AOT #748
Conversation
| <?xml version="1.0" encoding="UTF-8" ?> | ||
| <Project DefaultTargets="RunApkTests" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <Configuration Condition=" '$(Condition)' == '' ">Debug</Configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol oops. :-)
| <Package>Mono.Android_Tests</Package> | ||
| <InstrumentationType>xamarin.android.runtimetests.TestInstrumentation</InstrumentationType> | ||
| <ResultsPath>$(MSBuildThisFileDirectory)..\..\..\TestResult-Mono.Android_Tests.xml</ResultsPath> | ||
| <ResultsPath Condition=" '$(Configuration)' != 'Release' " >$(MSBuildThisFileDirectory)..\..\..\TestResult-Mono.Android_Tests.xml</ResultsPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to get unwieldy fast: the next time we need to generate another set of configurations, this is going to need to be updated again.
Perhaps we should encode things for everything?
<ResultsPath>$(MSBuildThisFileDirectory)..\..\..\TestResult-Mono.Android_Tests-$(Configuration)+Aot=$(AotAssemblies).xml</ResultsPath>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered similar name, but because AotAssemblies is usually empty I opted for the conditions. If you don't mind the names ending with +Aot=.xml I can change it. Or we can have 2 cases with and without Aot in the name. Like TestResult-Mono.Android_Tests-$(Configuration).xml and TestResult-Mono.Android_Tests-$(Configuration)-Aot.xml. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus I wasn't sure whether other parts (Jenkins, wrench?) use the TestResult-Mono.Android_Tests.xml filename, so the original PR version keeps the original filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to avoid making the entire $(ResultsPath) value conditional, as it'll be too easy to overlook something when we inevitably need to update things later.
We could split the difference: in tests/RunApkTests.targets, before the <Import/>s, do:
<PropertyGroup>
<_AotName Condition=" '$(AotAssemblies)' != '' ">-Aot</_AotName>
</PropertyGroup>Then you should be able to use:
<ResultsPath>$(MSBuildThisFileDirectory)..\..\..\TestResult-Mono.Android_Tests-$(Configuration)$(_AotName).xml</ResultsPath>Plus I wasn't sure whether other parts (Jenkins, wrench?) use the
TestResult-Mono.Android_Tests.xmlfilename
Jenkins does, but through file glob: at the xamarin-android Configuration page, in the Publish xUnit test result report section, is an NUnit-2 (default) Pattern textfield:
xamarin-android/TestResult-*.xml
This is the file glob of files to look for which contain NUnit2 test result files.
Since this is a file glob, there should be no problem using a templatei-zed %(UnitTestApk.ResultsPath) value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good. It will also make it easier to update other .projitems files when needed.
Makefile
Outdated
| TEST_APK_PROJECTS_RELEASE = \ | ||
| src/Mono.Android/Test/Mono.Android-Tests.csproj | ||
|
|
||
| # Syntax: $(call BUILD_TEST_APK,path/to/project.csproj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be updated, as you're now providing two values to the define.
| MSBUILD="$(MSBUILD)" tools/scripts/xabuild $(MSBUILD_FLAGS) /t:SignAndroidPackage $(2) $(1) | ||
| endef # BUILD_TEST_APK | ||
|
|
||
| define RUN_APK_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likewise have a Syntax comment:
# Syntax: $(call RUN_APK_TESTS,path/to/project.csproj,extra_flags)
Makefile
Outdated
| endef # BUILD_TEST_APK | ||
|
|
||
| define RUN_APK_TESTS | ||
| $(foreach p, $(1), $(call BUILD_TEST_APK, $(p),$(2))); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't use ; \; I think that will silence any errors which are reported.
04c97e2 to
d0469ad
Compare
- besides running in different configurations than default (Debug), we will also use the startup times measurements in the Jenkins plots - also fixes a typo in RunApkTests.targets, where supposedly 'Condition' was used in place of 'Configuration' - modifies logcat timing file names, so that it is based on test results filename instead of just package name. that way we can have multiple timing results for the same test with various configurations
d0469ad to
9ca9db4
Compare
| $(foreach p, $(TEST_APK_PROJECTS), $(call BUILD_TEST_APK, $(p))) | ||
| $(MSBUILD) $(MSBUILD_FLAGS) tests/RunApkTests.targets | ||
| $(call RUN_APK_TESTS, $(TEST_APK_PROJECTS), ) | ||
| ifneq ($(wildcard bin/Release),) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary, unless the intent is to prevent these tests from running on the PR builder.
The bin/Debug/... artifacts should be able to build an app with /p:Configuration=Release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bin/Debug/... artifacts are able to build normal apps with /p:Configuration=Release, such as samples/HelloWorld.
However, our unit tests are not "normal" apps; they import e.g. Configuration.props, and depend strongly on $(Configuration), so all manner of things break if they don't exist.
besides running in different configurations than default (Debug),
we will also use the startup times measurements in the Jenkins
plots
also fixes a typo in RunApkTests.targets, where supposedly
'Condition' was used in place of 'Configuration'
modifies logcat timing file names, so that it is based on test
results filename instead of just package name. that way we can have
multiple timing results for the same test with various
configurations