fix some tests for Android #693

ndusart · 2017-07-20T12:17:43Z

I tested the crate on an Android emulator and quite all the tests passed. Only 6 were failing.

failures:

---- sys::test_pthread::test_pthread_self stdout ----
thread 'sys::test_pthread::test_pthread_self' panicked at 'assertion failed: tid > 0', test/sys/test_pthread.rs:6
note: Run with RUST_BACKTRACE=1 for a backtrace.

---- sys::test_socket::test_getsockname stdout ----
thread 'sys::test_socket::test_getsockname' panicked at 'bind failed: Sys(EACCES)', /checkout/src/libcore/result.rs:859

---- sys::test_socket::test_unixdomain stdout ----
thread 'sys::test_socket::test_unixdomain' panicked at 'bind failed: Sys(EACCES)', /checkout/src/libcore/result.rs:859

---- sys::test_termios::test_local_flags stdout ----
thread 'sys::test_termios::test_local_flags' panicked at 'called Option::unwrap() on a None value', /checkout/src/libcore/option.rs:329

---- test_net::test_if_nametoindex stdout ----
thread 'test_net::test_if_nametoindex' panicked at 'assertion failed: if_nametoindex(&LOOPBACK[..]).is_ok()', test/test_net.rs:11

---- test_unistd::test_mkstemp stdout ----
thread 'test_unistd::test_mkstemp' panicked at 'mkstemp failed: ENOENT: No such file or directory', test/test_unistd.rs:73

This PR fixes 3 of those:

test_pthread_self: pthread_t is unsigned, so it can be negative (and it is often the case)
test_if_nametoindex: constant LOOPBACK is the same than on Linux
test_mkstemp: directory /tmp does not exist in Android

Two are still failing (test_unixdomain and test_getsockname) because we need some special permissions on Android for playing with sockets. On a rooted physical device, they passed. So, if tests for Android are integrated in CI, we should try to embed the tests to run in an APK with requested permissions or find a way to give the permission to a native program.

test_local_flags is still failing also because O_LARGEFILE is hardcoded and is not the right value in Android. Then fcntl::OFlag::from_bits(flags).unwrap() fails because this flag is then not recognised.
I made a PR in libc so that we can rely on libc definitions for all O_* flags. (rust-lang/libc#683)

Do you prefer to wait for this one to be fixed for this PR to merged as well ?

Susurrus · 2017-07-20T14:00:46Z

test/sys/test_pthread.rs

@@ -3,5 +3,5 @@ use nix::sys::pthread::*;
 #[test]
 fn test_pthread_self() {
    let tid = pthread_self();
-    assert!(tid > 0);
+    assert!(tid != 0);


Actually the man page for pthread_self says we should only use pthread_equals for comparing pthreads. If anything we shouldn't even check this value as it's non-portable.

I was wondering that when I read that test.

So should we remove it ?

Yeah, remove it, but leave it as a separate commit with a description of why it was removed.

0 still isn't valid here, we should really fix this at some point, but this is fine for now.

Do I just remove the test leaving the test_pthread.rs file for a future test to be inserted later or should I completely remove the file ?

Concerning the validity of the test. Maybe we have to wait pthread_create is defined in nix, so that we could make a test where we spin a thread and test that pthread_self returns different pthread_t in the different threads.
And we shouldn't even test for an "invalid" return value as pthread_self is not meant to fail, man page says it always succeed.

Leaving this test as-is is fine.

Susurrus · 2017-07-20T14:03:51Z

test/test_unistd.rs

@@ -64,7 +64,12 @@ fn test_wait() {

 #[test]
 fn test_mkstemp() {
-    let result = mkstemp("/tmp/nix_tempfile.XXXXXX");
+    #[cfg(target_os = "android")]


Shouldn't this use std::env::temp_dir instead?

And actually, this would be cleaner if we separated this into two tests, since they are two separate tests just rolled into one. Could you do that here as well?

Didn't know about std::env::temp_dir, we definitely should use it instead.

The tests are splitted and use std::env::temp_dir now :)

Susurrus · 2017-07-20T14:08:00Z

test/test_net.rs

@@ -1,9 +1,9 @@
 use nix::net::if_::*;

-#[cfg(target_os = "linux")]
+#[cfg(any(target_os = "linux", target_os = "android"))]


The conditions should be alphabetized, so "android" should come first here. Same for the following #cfg attribute.

Susurrus · 2017-07-20T14:08:38Z

This is a great start, thanks @ndusart!

Do you prefer to wait for this one to be fixed for this PR to merged as well?

While fixing all the tests is ideal, I'd like to limit this PR to those tests which can work with the current release of libc and also don't add any new hard-coded values. We'll wait on the rest once 0.9 is released and we're back to using libc-git. You can either raise a new issue or a separate PR to track that work; your call.

Regarding some points you brought up:

Two are still failing (test_unixdomain and test_getsockname) because we need some special permissions on Android for playing with sockets. On a rooted physical device, they passed. So, if tests for Android are integrated in CI, we should try to embed the tests to run in an APK with requested permissions or find a way to give the permission to a native program.

I get the feeling we'll run into tons of permission issues with Android as we continue to develop nix. So I'm fine with needing to figure out a rooted app to run tests on. It'll be up to the users of nix to figure out if Android supports that functionality or not for their own use cases.

Susurrus · 2017-07-20T15:48:50Z

test/test_unistd.rs

-        Err(_) => {}
-    }
+#[test]
+#[should_panic]


Our test shouldn't panic, you should just not use unwrap() here. You should use assertions to check the return value.

Yes, that's right, I tried to tied to the behavior of the test as it was written, it seemed logical at the moment.

asomers · 2017-07-21T00:34:39Z

test/test_unistd.rs

@@ -64,22 +64,27 @@ fn test_wait() {

 #[test]
 fn test_mkstemp() {
-    let result = mkstemp("/tmp/nix_tempfile.XXXXXX");
+    use std::env;


stylistically, I prefer to put all use statements at the top of the file, unless there's a good reason not to.

I agree with this, especially since this is repeated in the following test. My rule is that if there are a lot of little things that need to be imported in a function/module or if they're conditionally imported, then they're fine within the function/module. Otherwise they should be left out.

Susurrus · 2017-07-24T00:56:11Z

test/sys/test_pthread.rs

@@ -3,5 +3,5 @@ use nix::sys::pthread::*;
 #[test]
 fn test_pthread_self() {
    let tid = pthread_self();
-    assert!(tid > 0);
+    assert!(tid != 0);


Leaving this test as-is is fine.

Susurrus · 2017-07-24T00:58:21Z

@asomers Wanna take another look at this? Looks like small solid fixes to me!

asomers · 2017-07-24T01:08:33Z

bors r+

693: fix some tests for Android r=asomers I tested the crate on an Android emulator and quite all the tests passed. Only 6 were failing. > failures: > > ---- sys::test_pthread::test_pthread_self stdout ---- > thread 'sys::test_pthread::test_pthread_self' panicked at 'assertion failed: tid > 0', test/sys/test_pthread.rs:6 > note: Run with `RUST_BACKTRACE=1` for a backtrace. > > ---- sys::test_socket::test_getsockname stdout ---- > thread 'sys::test_socket::test_getsockname' panicked at 'bind failed: Sys(EACCES)', /checkout/src/libcore/result.rs:859 > > ---- sys::test_socket::test_unixdomain stdout ---- > thread 'sys::test_socket::test_unixdomain' panicked at 'bind failed: Sys(EACCES)', /checkout/src/libcore/result.rs:859 > > ---- sys::test_termios::test_local_flags stdout ---- > thread 'sys::test_termios::test_local_flags' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/option.rs:329 > > ---- test_net::test_if_nametoindex stdout ---- > thread 'test_net::test_if_nametoindex' panicked at 'assertion failed: if_nametoindex(&LOOPBACK[..]).is_ok()', test/test_net.rs:11 > > ---- test_unistd::test_mkstemp stdout ---- > thread 'test_unistd::test_mkstemp' panicked at 'mkstemp failed: ENOENT: No such file or directory', test/test_unistd.rs:73 This PR fixes 3 of those: - `test_pthread_self`: pthread_t is unsigned, so it can be negative (and it is often the case) - `test_if_nametoindex`: constant `LOOPBACK` is the same than on Linux - `test_mkstemp`: directory `/tmp` does not exist in Android Two are still failing (`test_unixdomain` and `test_getsockname`) because we need some special permissions on Android for playing with sockets. On a rooted physical device, they passed. So, if tests for Android are integrated in CI, we should try to embed the tests to run in an APK with requested permissions or find a way to give the permission to a native program. `test_local_flags` is still failing also because `O_LARGEFILE` is hardcoded and is not the right value in Android. Then `fcntl::OFlag::from_bits(flags).unwrap()` fails because this flag is then not recognised. I made a PR in `libc` so that we can rely on libc definitions for all `O_*` flags. (rust-lang/libc#683) Do you prefer to wait for this one to be fixed for this PR to merged as well ?

bors · 2017-07-24T02:02:02Z

Build succeeded

ndusart force-pushed the android-test branch from 224f78e to 51b20cf Compare July 20, 2017 13:32

Susurrus suggested changes Jul 20, 2017

View reviewed changes

fix some tests for Android

f72287e

ndusart force-pushed the android-test branch from c73bb5c to 522c714 Compare July 20, 2017 14:48

Susurrus reviewed Jul 20, 2017

View reviewed changes

ndusart force-pushed the android-test branch from 522c714 to 7cba750 Compare July 20, 2017 15:57

asomers reviewed Jul 21, 2017

View reviewed changes

Susurrus mentioned this pull request Jul 21, 2017

Release 0.9.0 #632

Closed

10 tasks

use std::env::temp_dir() to retrieve the temp directory in test_mkstemp

a546ef9

ndusart force-pushed the android-test branch from 7cba750 to a546ef9 Compare July 21, 2017 09:44

Susurrus approved these changes Jul 24, 2017

View reviewed changes

asomers approved these changes Jul 24, 2017

View reviewed changes

bors bot merged commit a546ef9 into nix-rust:master Jul 24, 2017

ndusart deleted the android-test branch July 24, 2017 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix some tests for Android #693

fix some tests for Android #693

ndusart commented Jul 20, 2017 •

edited

Loading

Susurrus Jul 20, 2017

ndusart Jul 20, 2017

Susurrus Jul 20, 2017

Susurrus Jul 21, 2017

ndusart Jul 21, 2017

Susurrus Jul 24, 2017

Susurrus Jul 20, 2017

Susurrus Jul 20, 2017

ndusart Jul 20, 2017

ndusart Jul 20, 2017

Susurrus Jul 20, 2017

Susurrus commented Jul 20, 2017

Susurrus Jul 20, 2017

ndusart Jul 20, 2017

asomers Jul 21, 2017

Susurrus Jul 21, 2017

Susurrus Jul 24, 2017

Susurrus commented Jul 24, 2017

asomers commented Jul 24, 2017

bors bot commented Jul 24, 2017

fix some tests for Android #693

fix some tests for Android #693

Conversation

ndusart commented Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 24, 2017

asomers commented Jul 24, 2017

bors bot commented Jul 24, 2017

Build succeeded

ndusart commented Jul 20, 2017 •

edited

Loading