-
Notifications
You must be signed in to change notification settings - Fork 273
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
Add doc for android startup scenario local testing. #4664
Conversation
0480e8d
to
131d373
Compare
Lint failures are unrelated to this PR, or markdown we manage. The failures are in eng/common/template-guidance.md. |
131d373
to
081ab90
Compare
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.
Thanks a lot for submitting the doc!
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 are no longer necessary I believe
```sh | ||
python test.py devicestartup --device-type android --package-path <path to apk (e.g. .)>/<apkname>.apk --package-name <apk package name> [--disable-animations] | ||
``` | ||
|
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.
Maybe add information about where the results of the performance test can be found or an example of what the output should look like.
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.
Added section on this.
docs/android-startup-scenarios.md
Outdated
cd helloandroid | ||
``` | ||
|
||
3. Copy the APK into the `helloandroid` directory. |
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.
Is there any correlation between dotnet version used for building the .apk and the one used as part of the environment?
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 script should work with any .apk, cc: @LoopedBard3
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.
There should not be any correlation if using a separately built APK. There are scenarios that this does not currently cover where the dotnet version would matter, e.g. when building Maui apps with pre.py the dotnet version will matter.
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 am a bit confused, could you please clarify:
- Is this doc specific to HelloAndroid (the
dotnet/runtime
) sample, or it refers to any Android app - any .apk?- If it is the former - only HelloAndroid sample, there are couple of issues I can see with it:
- we should have a more general solution and a doc for it, not just HelloAndroid sample, since the sample is not a real representative of an .NET Android application
- it should be pointed that .apks of HelloAndroid sample can produce different results before and after: [Android] Decouple runtime initialization and entry point execution for Android sample runtime#111742 gets merged to main and with it, it will matter which dotnet version is used
- If it is the latter, then we should rename the scenario to avoid confusion (or create a new one which would be just for this purpose). Maybe
androidstartup
or something like that, the point is that it shouldn't refer to HelloAndroid in any way for the above mentioned reasons
- If it is the former - only HelloAndroid sample, there are couple of issues I can see with 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.
The doc is more general for any APK, so I went ahead and added a new scenario folder named genericandroidstartup to make it more clear that it can be used for any APK. The reason we have the specific scenarios for some of the other apps is some of the pre.py files can build the apps (like the mauiandroid), but most of the test.py's for the android testing are interchangeable.
b1abc16
to
d00cefb
Compare
d00cefb
to
c72d44a
Compare
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.
LGTM!
… android startup scenarios doc to use it.
docs/android-startup-scenarios.md
Outdated
```sh | ||
python/(py -3 on Linux) test.py devicestartup --device-type android --package-path <path to apk (e.g. .)>/<apkname>.apk --package-name <apk package name> [--disable-animations] [--use-fully-drawn-time --fully-drawn-extra-delay <delay in sec> (**see below for note**)] | ||
``` |
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 looks cryptic when observed as a shell command.
I suggest splitting it into multiple commands for example:
- non-Linux
python test.py devicestartup --device-type android --package-path <path-to-apk> --package-name <apk-package name> [--disable-animations] [--use-fully-drawn-time --fully-drawn-extra-delay <delay-in-sec>]
- Linux
py -3 test.py devicestartup --device-type android --package-path <path-to-apk> --package-name <apk-package name> [--disable-animations] [--use-fully-drawn-time --fully-drawn-extra-delay <delay-in-sec>]
- Please refer to the Notes below about specifying
--use-fully-drawn-time --fully-drawn-extra-delay
parameters
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.
Seems we can use python for both windows and Linux, so I kept just 1 reference. Also added the reference to the notes separately.
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'm unsure about the py -3
suggestion for Linux... Isn't py -3
specific to Windows Python Launcher? In my experience, I always use python
or python3
on Ubuntu or macOS.
I think we should have example for:
- Windows
- Unix-based (that should work for both Linux and macOS)
…d and updated the python reference to only have python.
Looked again. It seems we should be able to get away with using just python on both Linux and Windows. So I switched to just using python in the doc. |
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.
LGTM, thanks!
Add first iteration of a doc for android startup scenario local testing.