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.

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

[Android] Add testing support #1714

Merged
merged 1 commit into from
Jun 14, 2016
Merged

Conversation

modocache
Copy link
Contributor

What's in this pull request?

This adds support for running tests for the stdlib built for Android, both on the host machine and on an Android device.

The Android variant of Swift may be built and tested using the following build-script invocation:

$ utils/build-script \
  -R \                                           # Build in ReleaseAssert mode.
  -T \                                           # Run all tests.
  --android \                                    # Build for Android.
  --android-deploy-device-path /data/local/tmp \ # Temporary directory on the device where Android tests are run.
  --android-ndk ~/android-ndk-r10e \             # Path to an Android NDK.
  --android-ndk-version 21 \
  --android-icu-uc ~/libicu-android/armeabi-v7a/libicuuc.so \
  --android-icu-uc-include ~/libicu-android/armeabi-v7a/icu/source/common \
  --android-icu-i18n ~/libicu-android/armeabi-v7a/libicui18n.so \
  --android-icu-i18n-include ~/libicu-android/armeabi-v7a/icu/source/i18n/

See docs/Testing.rst for more details.

Resolved bug number: None


Before merging this pull request to apple/swift repository:

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
OS X platform @swift-ci Please test OS X platform
Linux platform @swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

@modocache modocache mentioned this pull request Mar 16, 2016
@modocache modocache force-pushed the tests branch 3 times, most recently from da6d0e2 to 0d8aa27 Compare April 13, 2016 18:48
@modocache
Copy link
Contributor Author

modocache commented Apr 13, 2016

Now that #1442 has been merged, I've rebased this onto master, which makes the changeset much smaller. Here's the remaining feedback on these changes, from the discussion in #1442:

The tests do currently run with these changes, though (see the instructions added in this commit to docs/Testing.rst). Here's the breakdown as of this comment:

Failing Tests (9):
    Swift :: 1_stdlib/PrintFloat.swift
    Swift :: 1_stdlib/tgmath.swift
    Swift :: 1_stdlib/tgmath_optimized.swift
    Swift :: DebugInfo/ASTSection.swift
    Swift :: Driver/many-inputs.swift
    Swift :: IRGen/condfail.sil
    Swift :: IRGen/objc_simd.sil
    Swift :: Reflection/typeref_decoding.swift
    Swift :: stdlib/RangeReplaceable.swift.gyb

  Expected Passes    : 7166
  Expected Failures  : 76
  Unsupported Tests  : 772
  Unexpected Failures: 9

# FIXME: This value should be read off of config.android_ndk_path,
# but I can't figure out how. The value is set via CMake, and the
# build directory contains a test-android-armv7/lit.site.cfg with
# its @SWIFT_ANDROID_NDK_PATH@ correctly replaced. So what's wrong?
Copy link
Contributor

Choose a reason for hiding this comment

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

What behavior are you observing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I invokve utils/built-script -T, the lit.site.cfg.in is correctly configured and placed in my build directory:

# build/Ninja-ReleaseAssert/swift-linux-x86_64/test-android-armv7/lit.site.cfg
config.android_ndk_path = "/home/modocache/android-ndk-r10e"

But when the tests are run later during that same utils/build-script -T invocation, the attempt to access the value from within the test/lit.cfg fails:

# swift/test/lit.cfg
android_ndk_path = config.android_ndk_path

# lit.py: lit.cfg:738: note: Testing Android armv7-none-linux-androideabi
# lit.py: TestingConfig.py:114: fatal: unable to parse config file '/home/modocache/GitHub/apple/swift/test/lit.cfg', traceback: Traceback (most recent call last):
#   File "/home/modocache/GitHub/apple/llvm/utils/lit/lit/TestingConfig.py", line 101, in load_from_path
#     exec(compile(data, path, 'exec'), cfg_globals, None)
#   File "/home/modocache/GitHub/apple/swift/test/lit.cfg", line 743, in <module>
#     android_ndk_path = config.android_ndk_path
# AttributeError: TestingConfig instance has no attribute 'android_ndk_path'

Alternatively:

# swift/test/lit.cfg
android_ndk_path = getattr(config, 'android_ndk_path')

# lit.py: TestingConfig.py:114: fatal: unable to parse config file '/home/modocache/GitHub/apple/swift/test/lit.cfg', traceback: Traceback (most recent call last):
#   File "/home/modocache/GitHub/apple/llvm/utils/lit/lit/TestingConfig.py", line 101, in load_from_path
#     exec(compile(data, path, 'exec'), cfg_globals, None)
#   File "/home/modocache/GitHub/apple/swift/test/lit.cfg", line 743, in <module>
#     android_ndk_path = getattr(config, 'android_ndk_path')
# AttributeError: TestingConfig instance has no attribute 'android_ndk_path'

Bizarrely, running lit directly works:

$ llvm/utils/lit/lit.py -v build/Ninja-ReleaseAssert/swift-linux-x86_64/test-android-armv7

I'm still trying to figure this out, I'm sure it's something silly I'm overlooking, or maybe the result of some build artifact on my host machine. 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess: you have this in test/lit.site.cfg.in, but not validation-test/lit.site.cfg.in. We should probably chain the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was it!! Thanks for the tip. Yes, chaining would be nice. I think this would make a good starter task, so I wrote up a Swift report: https://bugs.swift.org/browse/SR-1266.

@gribozavr
Copy link
Contributor

validation-test/stdlib/CollectionType.swift.gyb should be split up

Please don't :) We already split it on the swift-3-indexing-model branch, and we'd prefer to not have more merge conflicts.

@@ -16,6 +16,9 @@
// RUN: %S/../../utils/line-directive %t/InputStream.swift -- %target-run %t/a.out
// REQUIRES: executable_test

// FIXME: This test takes too long to complete on Android.
// UNSUPPORTED: OS=linux-androideabi
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it just hangs instead. It uses stdin.

@gribozavr
Copy link
Contributor

Sorry for dropping the ball on this! Could you resolve the conflicts?

@modocache
Copy link
Contributor Author

@gribozavr On the contrary, I've dropped the ball! I still haven't addressed most of the concerns you raised. But yes, I'll rebase soon. Thanks!

@modocache
Copy link
Contributor Author

@gribozavr Sorry for the wait! I rebased this. When the Android tests are run with build-script -T, only one test fails: compiler_crashers/28299-swift-lookupvisibledecls.swift.

I still need to address some of your previous suggestions:

I'm hoping to get these done this week. Any advice you have in the meantime would be very helpful!!

set -x
"${SWIFT_SOURCE_DIR}"/utils/android/adb_push_built_products.py --ndk "${ANDROID_NDK}" --destination "${ANDROID_DEPLOY_DEVICE_PATH}" "$(build_directory ${HOST_TARGET} swift)"/lib/swift/android/
{ set +x; } 2>/dev/null
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Uploading should be done in test/CMakeLists.txt -- just set the command_upload_stdlib if SDK is Android. This way, just doing ninja check-swift-android will do the right thing and use the latest library.

@modocache
Copy link
Contributor Author

I've made this pull request a bit smaller by reducing it to just what's necessary to get the test suite to run to completion. I moved fixes for various failing tests to #2939.

@modocache
Copy link
Contributor Author

I think this is ready to go. I'd like to address the following issues in later pull requests. I'll create JIRA tasks to track them:

  • Use the Android linker when running the test suite targeting Android: I can either wait for Added -toolchain option to swiftc #2912 to be merged, or implement a PATH-based solution in this pull request. Thoughts?
  • Limit parallelism when running tests on an Android host device: Originally mentioned here: Port to Android #1442 (comment). I think this would be great, but would make this pull request even larger. I'd love to defer this to a follow-up.
  • The fork/exec behavior for Android StdlibUnittest should be the same as posix_spawn: Originally mentioned in Port to Android #1442 (comment) and Port to Android #1442 (comment). Frankly, I tried my hand at this several times but couldn't get it quite right. I'd love to defer this as well.
  • Other potential improvements: what happens if multiple devices are connected? Do we support running on an Android simulator or emulator? Genymotion? Can we split the tests up into host and non-host tests, like iOS?

@gribozavr
Copy link
Contributor

@swift-ci Please test

@gribozavr
Copy link
Contributor

@swift-ci Please smoke test

@gribozavr
Copy link
Contributor

@swift-ci Please Python lint

@gribozavr
Copy link
Contributor

I think this is ready to go. I'd like to address the following issues in later pull requests. I'll create JIRA tasks to track them:

Thanks!

  • Use the Android linker when running the test suite targeting Android: I can either wait for Added -toolchain option to swiftc #2912 to be merged, or implement a PATH-based solution in this pull request. Thoughts?

I think the -toolchain PR has a good chance to land soon.

  • Limit parallelism when running tests on an Android host device: Originally mentioned here: Port to Android #1442 (comment). I think this would be great, but would make this pull request even larger. I'd love to defer this to a follow-up.
  • The fork/exec behavior for Android StdlibUnittest should be the same as posix_spawn: Originally mentioned in Port to Android #1442 (comment) and Port to Android #1442 (comment). Frankly, I tried my hand at this several times but couldn't get it quite right. I'd love to defer this as well.

Makes sense.

  • Other potential improvements: what happens if multiple devices are connected?

This would help with test latency! Usually the host has more cores than the device, so you can usually connect multiple devices "for free" without saturating the host CPU.

Do we support running on an Android simulator or emulator? Genymotion?

That would be great!

Can we split the tests up into host and non-host tests, like iOS?

The test annotations (executable_test) should be the same, and the CMake logic for generating targets is also shared. I'm wondering if it just works already! Try running the check-swift-validation-only_non_executable-android-armv7 target.

@modocache
Copy link
Contributor Author

@gribozavr Looks like everything passed! Is this OK to merge?

@modocache
Copy link
Contributor Author

Rebased on top of #2919.

@modocache
Copy link
Contributor Author

modocache commented Jun 9, 2016

@swift-ci Please Python lint

@modocache
Copy link
Contributor Author

@swift-ci please test

@gribozavr
Copy link
Contributor

Re-testing after #2880 was merged.

@swift-ci Please test

@gribozavr
Copy link
Contributor

@swift-ci Please smoke test

# FIXME: Allow Android host tests to be enabled/disabled by the
# build script.
test_this_target=
test_this_target=$(not ${SKIP_TEST_ANDROID_HOST})
Copy link
Contributor

Choose a reason for hiding this comment

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

Since #2880 just landed, I believe there will need to be an update to the matching logic in build-script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up, @ddunbar! Rebasing now.

This adds support for running tests for the stdlib built
for Android, both on the host machine and on an Android device.

The Android variant of Swift may be built and tested using
the following `build-script` invocation:

```
$ utils/build-script \
  -R \                                           # Build in ReleaseAssert mode.
  -T \                                           # Run all tests.
  --android \                                    # Build for Android.
  --android-deploy-device-path /data/local/tmp \ # Temporary directory on the device where Android tests are run.
  --android-ndk ~/android-ndk-r10e \             # Path to an Android NDK.
  --android-ndk-version 21 \
  --android-icu-uc ~/libicu-android/armeabi-v7a/libicuuc.so \
  --android-icu-uc-include ~/libicu-android/armeabi-v7a/icu/source/common \
  --android-icu-i18n ~/libicu-android/armeabi-v7a/libicui18n.so \
  --android-icu-i18n-include ~/libicu-android/armeabi-v7a/icu/source/i18n/
```

See docs/Testing.rst for more details.
@modocache
Copy link
Contributor Author

Rebased and presumably ready to go. Going to kick off some tests.

@swift-ci Please test

@modocache
Copy link
Contributor Author

@swift-ci Please smoke test

@modocache
Copy link
Contributor Author

@swift-ci Please Python lint

@modocache
Copy link
Contributor Author

modocache commented Jun 9, 2016

@gribozavr All green! Confirmed that Android tests run on my local machine as well. Is this good to go?

@modocache
Copy link
Contributor Author

@gribozavr Friendly ping! :)

@gribozavr
Copy link
Contributor

Sorry for the delay, your PR LGTM!

@swift-ci Please smoke test and merge

@swift-ci swift-ci merged commit a1f1976 into swiftlang:master Jun 14, 2016
@modocache modocache deleted the tests branch June 14, 2016 04:49
@modocache
Copy link
Contributor Author

Excellent, thanks! Tomorrow I'll create a bunch of tasks for the failing tests, and for the improvements we discussed that didn't make it into this pull request.

@J-Bobb
Copy link

J-Bobb commented Jun 20, 2016

Sent from my Samsung Galaxy smartphone.-------- Original message --------From: Brian Gesiak notifications@github.com Date: 06/08/2016 10:06 PM (GMT-05:00) To: apple/swift swift@noreply.github.com Subject: Re: [apple/swift] [Android] Add testing support (#1714)
@swift-ci please test

@swift-ci Please Python lint


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants