Skip to content

ci: Run all tests in isolation#89

Merged
msiglreith merged 6 commits intorust-mobile:masterfrom
MarijnS95:ndk-test-isolated
Nov 3, 2020
Merged

ci: Run all tests in isolation#89
msiglreith merged 6 commits intorust-mobile:masterfrom
MarijnS95:ndk-test-isolated

Conversation

@MarijnS95
Copy link
Copy Markdown
Member

@MarijnS95 MarijnS95 commented Nov 3, 2020

When using && to cd back out of a subdirectory the error code from cargo test is implicitly consumed instead of being caught by set -e. While this does return a non-zero exit code it does not prevent the rest of the script from running if more lines follow.

The next lines run and, if successful, do not fail the step because GH actions base the result of a multiline shell script solely on the last command 1.

You can try this for yourself by throwing the following three steps in GH actions and observing the result 2:

false
echo "This is not printed"

false && echo "This is not printed"

false && echo "This is not printed"
echo "Oh noes, his is printed, and the script exits successfully"

In addition structuring the tests this way allows working-directory to be used, and shows every test individually in a PR to more quickly identify where things are going wrong.

@msiglreith msiglreith self-requested a review November 3, 2020 16:03
When using && to cd back out of a subdirectory the error code from cargo
test is implicitly consumed instead of being caught by set -e.  While
this does return a non-zero exit code it does not prevent the rest of
the script from running if more lines follow.

The next lines run and, if successful, do not fail the step because GH
actions base the result of a multiline shell script solely on the last
command [1].

You can try this for yourself by throwing the following three steps in
GH actions and observing the result [2]:

    false
    echo "This is not printed"

    false && echo "This is not printed"

    false && echo "This is not printed"
    echo "Oh noes, his is printed, and the script exits successfully"

In addition structuring the tests this way allows working-directory to
be used, and shows every test individually in a PR to more quickly
identify where things are going wrong.

[1]: https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference
[2]: https://github.com/MarijnS95/gh-actions-test/actions/runs/343934038
This feature does not exist anymore and breaks the tests:

    error: Package `ndk v0.2.1 (/home/runner/work/android-ndk-rs/android-ndk-rs/ndk)` does not have these features: `rustdoc`

Fixes: 7d36455 ("Explicitly set docs.rs targets and bump versions to 0.2.1/0.5.3 (rust-mobile#82)")
@MarijnS95
Copy link
Copy Markdown
Member Author

MarijnS95 commented Nov 3, 2020

     Running /home/runner/work/android-ndk-rs/android-ndk-rs/target/debug/deps/ndk-130ea9646c5271e4

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests ndk

running 4 tests
test src/asset.rs - asset::Asset (line 144) ... ok
test src/asset.rs - asset::AssetDir (line 59) ... ok
test src/native_activity.rs - native_activity::NativeActivity::vm (line 99) ... ok
test src/native_activity.rs - native_activity::NativeActivity::vm (line 89) ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

🎉

Looks like all tests actually run now (and succeed!) after introducing a test feature to ndk as well. Note that I've removed --doc which is implied by cargo test (but not by --all-targets!), see https://doc.rust-lang.org/cargo/commands/cargo-test.html#target-selection.

Copy link
Copy Markdown
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@MarijnS95
Copy link
Copy Markdown
Member Author

MarijnS95 commented Nov 3, 2020

@msiglreith I just pushed more changes to test all features :)

@msiglreith
Copy link
Copy Markdown
Contributor

@MarijnS95 yeah github yelled at me when I tried to merge 😃
let me know when it's fine to merge

@MarijnS95
Copy link
Copy Markdown
Member Author

@msiglreith Pushed again and added ndk-glue, if only for checking. Let's see if the tests still pass 😅

Fixes the following failure when compiling on a host that's not android:

    error: android-ndk-sys only supports compiling for Android
      --> ndk-sys/src/lib.rs:18:1
       |
    18 | compile_error!("android-ndk-sys only supports compiling for Android");
       | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In addition jni and jni-glue need to be linked to prevent subsequent
failures when compiling the documentation in native_activity.rs.
This crate was accidentally omitted from the tests, but contains quite a
few #[test] functions that were never ran.
Certain parts of the code are omitted from testing and checking, in
particular the logging feature.
While there are no tests here yet, this at least invokes the checking
stage on ndk-glue.
@MarijnS95
Copy link
Copy Markdown
Member Author

MarijnS95 commented Nov 3, 2020

@msiglreith All tests succeeded, this is ready from my perspective. Feel free to merge it after a second review, I don't think any new commits will show up :D

Someone should perhaps take a look at the deprecation warning in BitmapFormat though, originating from the TryFromPrimitive/IntoPrimitive macros using the deprecated enum variant RGBA_4444. Normally GH picks these up and prints them in the diff (warnings on unchanged lines/files) but it seems to have stopped doing so.

@msiglreith msiglreith merged commit cb0a1b4 into rust-mobile:master Nov 3, 2020
@MarijnS95 MarijnS95 deleted the ndk-test-isolated branch November 3, 2020 19:05
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.

2 participants