Skip to content

[android] Enable Thread tests were possible. #2181

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

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

drodriguez
Copy link
Contributor

Enable most of the Thread test for Android. The only tests that cannot
pass right now are the ones related to backtraces, which do not use the
Linux API backtrace and would need an specific implementation.

And while Android did not get pthread_getname_np until API 26, the low
level prctl(PR_GET_NAME) works with the same restriction as in Linux
(only 15 characters). With that change, the test about thread names also
passes in Android.

@spevans
Copy link
Contributor

spevans commented Apr 26, 2019

@swift-ci test

@drodriguez drodriguez force-pushed the android-enable-thread-tests branch from 3246b60 to 2efa9e9 Compare April 29, 2019 21:19
@drodriguez
Copy link
Contributor Author

Removed a preprocessor flag that shouldn’t have been there.

@swift-ci please test

@spevans
Copy link
Contributor

spevans commented Apr 30, 2019

@swift-ci test

@drodriguez
Copy link
Contributor Author

@millenomi: I think I have seen TestFileHandle.test_readWriteHandlers apparently lock up in several CI runs already. I wonder if the semaphore.wait(timeout: .distantFuture) can be reduced to a couple of seconds, to avoid locking up forever (probably breaking when result is not .success). Do you mind me doing that change? It will still fail, but at least the rest of the test suite will run, and the CI machine will not be hold for that long. Do you have a good timeout? 10 seconds?

@drodriguez
Copy link
Contributor Author

@swift-ci please test linux platform

@millenomi
Copy link
Contributor

There is an issue in libdispatch that is being tracked, but the wait kinda needs to match Darwin's.

@spevans
Copy link
Contributor

spevans commented Apr 30, 2019

Would an alarm(600) in Testfoundation/main.swift act as a simple timout in that SIGALRM should terminate the tests after 10 minutes?

@drodriguez
Copy link
Contributor Author

Is the libdispatch issue in a public tracker that I can follow? I don’t know what you refer with “match Darwin's”, but my proposal is changing the timeout for every platform. If I read the code correctly, this is writing and reading from a pipe, so the delays from reading/writing should be minimal, whatever the platform. I think the change will give us the same signal as we have now, but clearer (it will be the assert about the timeout), instead of a CI build killed because of such timeout. In any case, whatever you think is best.

@drodriguez
Copy link
Contributor Author

@swift-ci please test linux platform

@drodriguez drodriguez requested a review from compnerd May 1, 2019 22:40
@drodriguez drodriguez force-pushed the android-enable-thread-tests branch from 2efa9e9 to e84d955 Compare May 13, 2019 20:38
@drodriguez
Copy link
Contributor Author

Rebased and applied the changes from DEPLOYMENT_TARGET_* to TARGET_OS_*.

@swift-ci please test

@drodriguez
Copy link
Contributor Author

@swift-ci please test

@drodriguez drodriguez force-pushed the android-enable-thread-tests branch from e84d955 to 682e069 Compare May 29, 2019 20:29
@drodriguez
Copy link
Contributor Author

Rebased to fix the conflicts in the test file.

@swift-ci please test

@drodriguez drodriguez force-pushed the android-enable-thread-tests branch from 682e069 to 830d552 Compare July 9, 2019 00:41
@drodriguez
Copy link
Contributor Author

Rebasing to a recent master. Still passing tests in Linux and Android.

@spevans, @millenomi, @compnerd: some feedback? positive or negative? I'm trying to clean up my old “stuck” PRs.

@swift-ci please test Linux platform

Enable most of the Thread test for Android. The only tests that cannot
pass right now are the ones related to backtraces, which do not use the
Linux API backtrace and would need an specific implementation.

And while Android did not get pthread_getname_np until API 26, the low
level prctl(PR_GET_NAME) works with the same restriction as in Linux
(only 15 characters). With that change, the test about thread names also
passes in Android.
@drodriguez drodriguez force-pushed the android-enable-thread-tests branch from 830d552 to 138e586 Compare July 9, 2019 20:19
@drodriguez
Copy link
Contributor Author

@swift-ci please test Linux platform

@spevans
Copy link
Contributor

spevans commented Jul 10, 2019

LGTM

@drodriguez drodriguez merged commit 6cbc4bb into swiftlang:master Jul 10, 2019
@drodriguez drodriguez deleted the android-enable-thread-tests branch July 10, 2019 16:18
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.

3 participants