-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[mono] iOS Test runner #34976
[mono] iOS Test runner #34976
Conversation
Adding my 2 cents here. I think we should unify this with our current testing infrastructure/targets to run libraries tests instead of diverging paths by having some stuff under mono and some other under libraries. We could use a lot of what we have by adding some targets/commands to execute the app bundler and invoking the tests and what not. |
@safern At the very least, I would expect this PR to drive towards that. |
Yes, I think it should as a first step. |
@safern what would you like to be moved and to where? For now, only very specific libraries configurations are supported but the same infrastructure will be used for all other tests in dotnet/runtime so not sure if it makes sense to move it libraries as it's not libraries specific. |
src/mono/mono.proj
Outdated
<Error Condition="!Exists('$(TestDir)')" Text="TestDir=$(TestDir) doesn't exist" /> | ||
<Error Condition="!Exists('$(BclDir)')" Text="BclDir=$(BclDir) doesn't exist" /> | ||
|
||
<Copy SourceFiles="@(AppleTestRunnerBinaries)" DestinationFolder="$(TestDir)\%(RecursiveDir)" /> |
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 don't think this should be done here
I’d like to have this hooked up under /eng/testing and have the targets and all underlying logic there. Even though it will not be used by all configurations what we’re running are libraries tests. I don’t like all this manual copying of files and duplicating path calculation and all things involved here. We could hook it up in the infra we already have to run tests and everything would be much simpler. Instead of having to run specific mono targets I envision this as Or I think this is fine as is for the purpose of this PR, but we should eventually hook it up as o suggest above. I can definitely help with that, I started taking a look at how all this works yesterday and I think it is possible. |
@steveisok could you ensure @safern concerns are addressed |
cc: @ViktorHofer @danmosemsft |
@safern After talking w/ Egor & Alex, I'd like to land this PR so that we have a runner to use. Then in a separate PR, we can hook into what you outlined. We have additional ongoing work that I'd like to come together as well (in-tree runtime pack, AOT targets, etc). |
Yeah I don't want to block this PR. I just wanted to raise my concerns as a heads up that we should envision hooking it up the same way we already run libraries tests and of course happy to help with doing that. |
} | ||
} | ||
|
||
public class SimpleTestRunner : iOSApplicatonEntryPoint, IDevice |
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.
iOSApplicatonEntryPoint
-- is this a typo? iOSApplicationEntryPoint
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.
ah, it's in dotnet/xharness. I didn't know about this repo 😸
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.
cc @mandel-macaque ^
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.
also, while you are there could you please add events for "test is started", "test is finished (with result)" ?
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.
@danmosemsft I'm known for my typos, I'll get that fixed! @EgorBo dotnet/xharness#36
@@ -34,7 +34,7 @@ public class AppleAppBuilderTask : Task | |||
/// Path to Mono public headers (*.h) | |||
/// </summary> | |||
[Required] | |||
public string MonoInclude { get; set; } = ""!; | |||
public string MonoRuntimeHeaders { get; set; } = ""!; |
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.
What does ""!
achieve here? Is this the "null-forgiving" operator?
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.
These properties are guaranteed to be non-null always (due to [Required]
attribute) the compiler forces me to do inline init to leave them non-nullable. I wonder how @stephentoub solves such cases.
Co-Authored-By: Dan Moseley <danmose@microsoft.com>
CGRect caretRect = [logLabel caretRectForPosition:logLabel.endOfDocument]; | ||
[logLabel scrollRectToVisible:caretRect animated:NO]; | ||
[logLabel setScrollEnabled:NO]; | ||
[logLabel setScrollEnabled:YES]; | ||
}); |
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.
Some hack to automatically scroll to the bottom in UITextView
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.
why do you need to toggle setScrollEnabled? is the scrollRectToVisible not enough?
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.
@akoeplinger I don't know but without it doesn't work properly, I tried different hacks from SO this one works as expected (https://stackoverflow.com/questions/19124037/scroll-to-bottom-of-uitextview-erratic-in-ios-7/20989956#20989956)
@@ -62,7 +62,7 @@ public static async Task<int> Main(string[] args) | |||
|
|||
protected override IEnumerable<TestAssemblyInfo> GetTestAssemblies() | |||
{ | |||
foreach (string file in testLibs) | |||
foreach (string file in testLibs!) |
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.
Why is the null forgiving operator needed here?
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.
@safern I forgot to remove it
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.
Will fix it in the next PR
|
It's currently WIP, it doesn't use runtime packs yet but it's already possible to run any test suite (well, any without remote-executor based tests inside, see dotnet/arcade#5129 such tests usually have
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
inside csprojs). It uses dotnet/xharness test runner, copies everything including the whole BCL into a test folder and runs the AOT/Bundle generation.Steps to run a test suite:
for x64 it should build everything and launch an iphone simulator with tests. For arm64 devices it needs
xharness
tool (withmlaunch
inside - it's not yet there) and a provisioning profile.If you want to try arm64 now you can run the script above ^ (replace x64 with arm64) and open the xcode project (generated) and deploy to a personal device with auto-generated provisioning. Some tests might crash on arm64, e.g.:
or some missing *aotdata stuff. Arm64 needs some work.
I guess the expected workflow is the following: