Run tests in CI with `runner` #1190

tiziano88 · 2020-06-24T10:21:32Z

Summary of main advantages compared to bash scripts:

more discoverable UI (subcommands and flags have explicit documentation that shows up with --help)
ability to run locally all checks that would be run in CI (with the run-ci command); this gives reasonable confidence that nothing has been missed, which usually otherwise requires waiting to see the outputs of the CI steps, fix the (often trivial) issue locally, and wait for another cycle
clearer output, with logs only showed for failing steps; steps that succeed have a much more concise output
built-in timing for commands and groups of commands
explicit flags for all configurable parameters
easier to support more complex scenarios, such as cross compiling for different platforms
sub-commands to only run specific sets of tests or a single example
ability to rerun tests automatically on file changes (uses inotify, currently a bit flaky)
keeps running steps even if one of them fails (so that the final output is more comprehensive and does not stop at the first error)
written in Rust, and therefore benefiting from tools and knowledge that we use everywhere else in the repository
somehow slightly faster overall (40 mins down to 32 mins)
async structure, can be used to parallelize work even more in the future

Included changes:

support asan tests
add missing example manifest for trusted information retrieval
add support for passing env variables to commands in runner
separate format checks from run-tests command
support building "base" and "logless" versions of oak_loader
support running backend servers as part of examples that need it

tiziano88 · 2020-06-29T21:43:14Z

Still in progress, but I'd like to hear if there are any objections about this.

I think the current version is about feature parity with the bash scripts. Note that the goal is not to replace all the bash scripts, only the ones that currently rely on logic that is spread out over several scripts, have custom flags, and are not easily discoverable / maintainable.

We can (and should) keep bash scripts for simple sequences of commands.

daviddrysdale · 2020-06-30T06:42:10Z

tiziano88 · 2020-06-30T15:00:52Z

Thanks so much for writing down the list, I will let you know when they are all implemented, there are still a few missing (will keep the following checklist updated in the meanwhile):

rbehjati

I think docs/development.md and docs/INSTALL-OSX.md should be updated as part of this change list, since some of the scripts referenced in those docs are removed.

rbehjati · 2020-07-01T10:03:32Z

runner/src/main.rs

+        ],
+    }
+}
+
 fn run_ci() -> Step {


I think it is great that this command runs all the steps even if something fails in between.
I suggest logging a summary of the failures or the list of failed steps in the end, so that one does not have to go through the entire log to locate the problem.

Great idea! Created #1234 to track.

that sweet issue number too

tiziano88

Did another check, it seems everything is there (for real this time?)

I added the docker save step, which I previously missed, and also a few missing license headers to proto files that have been added in the meanwhile (I guess the previous scripts didn't really check for that).

I have attempted to make the binary compatible with mac, but I don't have a way of testing it, so please let me know if it works @daviddrysdale .

Thanks everyone for all your comments and your patience with this, I learned a lot in the process, I hope it was helpful for others too!

tiziano88 · 2020-07-08T18:30:07Z

runner/src/internal.rs

+        // Python variables.
+        if let Ok(v) = std::env::var("PYTHONPATH") {
+            cmd.env("PYTHONPATH", v);
+        }
+
+        // Rust compilation variables.
+        if let Ok(v) = std::env::var("RUSTUP_HOME") {
+            cmd.env("RUSTUP_HOME", v);
+        }
+        if let Ok(v) = std::env::var("CARGO_HOME") {
+            cmd.env("CARGO_HOME", v);
+        }
+
+        // Rust runtime variables.
+        cmd.env(
+            "RUST_LOG",
+            std::env::var("RUST_LOG").unwrap_or_else(|_| "info".to_string()),
+        );
+        cmd.env("RUST_BACKTRACE", "1");
+
+        // Emscripten variables.
+        if let Ok(v) = std::env::var("EMSDK") {
+            cmd.env("EMSDK", v);
+        }
+        if let Ok(v) = std::env::var("EM_CACHE") {
+            cmd.env("EM_CACHE", v);
+        }
+        if let Ok(v) = std::env::var("EM_CONFIG") {
+            cmd.env("EM_CONFIG", v);
+        }
+


It is propagating only some env variables to the child process, yes. I hope to clean it up later on, so that we only pass the relevant ones for each process.

tiziano88 · 2020-07-08T18:32:18Z

runner/src/main.rs

            run_cargo_clippy(),
            run_cargo_test(),
+            run_cargo_doc(),
+            run_cargo_test_tsan(),


I agree, I also usually skip it locally. Made it a separate command, and included it in run-ci. Thanks!

tiziano88 · 2020-07-08T18:41:34Z

runner/src/main.rs

+            build_server(&BuildServer {
+                server_variant: "base".to_string(),
+                server_rust_toolchain: None,
+                server_rust_target: DEFAULT_SERVER_RUST_TARGET.to_string(),


I think this is fixed now, please let me know if it does not work for you.

tiziano88 · 2020-07-08T18:42:21Z

runner/src/main.rs

+/// Return whether the provided path refers to a source file that can be formatted by clang-tidy.
+fn is_clang_format_file(path: &PathBuf) -> bool {
+    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
+    filename.ends_with(".cc") || filename.ends_with(".h") || filename.ends_with(".proto")
+}
+
 /// Return whether the provided path refers to a Bazel file (`BUILD`, `WORKSPACE`, or `*.bzl`)
 fn is_bazel_file(path: &PathBuf) -> bool {
    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
    filename == "BUILD" || filename == "WORKSPACE" || filename.ends_with(".bzl")
 }

+fn is_build_file(path: &PathBuf) -> bool {
+    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
+    filename == "BUILD"
+}
+
 /// Return whether the provided path refers to a markdown file (`*.md`)
 fn is_markdown_file(path: &PathBuf) -> bool {
    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
    filename.ends_with(".md")
 }

+fn is_dockerfile(path: &PathBuf) -> bool {
+    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
+    filename.ends_with("Dockerfile")
+}
+
+fn is_toml_file(path: &PathBuf) -> bool {
+    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
+    filename.ends_with(".toml")
+}
+
+fn is_yaml_file(path: &PathBuf) -> bool {
+    let filename = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
+    filename.ends_with(".yaml")
+}


I definitely thought about higher order functions / combinators too :) but I didn't want to do all the fun stuff myself (hint hint)

tiziano88 · 2020-07-08T18:44:49Z

runner/src/check_todo.rs

+        let file_content = std::fs::read_to_string(&self.path).expect("could not read file");
+        let todo_words = file_content
+            .split_whitespace()
+            .filter(|word| word.contains(&format!("{}{}", "TO", "DO")))


tiziano88 · 2020-07-08T18:47:08Z

runner/src/check_todo.rs

+            .split_whitespace()
+            .filter(|word| word.contains(&format!("{}{}", "TO", "DO")))
+            .filter(|word| {
+                !(word.starts_with(&format!("{}{}(", "TO", "DO"))


Added a TODO, fittingly :)

tiziano88 · 2020-07-08T22:33:00Z

scripts/run_example

-  ps -p ${SERVER_PID} > /dev/null
-  readonly SERVER_GONE=$?
-  if ${SERVER_GONE} ; then
-    echo "Runtime server has terminated early"


Added a TODO, I'd like to find a nicer way for this, and I think the situation is somewhat mitigated in runner by the fact that if the background process exits with a nonzero status, it will already count as failure.

tiziano88 · 2020-07-08T22:33:20Z

scripts/run_example

-  fi
-fi
-
-kill_bg_pids


Added a crude way for now, can be improved upon.

daviddrysdale · 2020-07-09T05:57:17Z

examples/abitest/example.toml

+[application]
+manifest = "examples/abitest/config/config.toml"
+out = "examples/abitest/bin/config.bin"
+
 [modules]
 module_0 = { Cargo = { cargo_manifest = "examples/abitest/module_0/rust/Cargo.toml" } }
 module_1 = { Cargo = { cargo_manifest = "examples/abitest/module_1/rust/Cargo.toml" } }


pre-existing nit below: [clients] cpp is not accurate, it would be nice to sync this with #1170.

I think this is accurate, i.e. it is the cpp client, and it is built via bazel, which is what the details of the entry says.

Ah, my bad, this isn't a fixed schema – this comment belongs elsewhere…

daviddrysdale · 2020-07-09T10:41:22Z

examples/abitest/example.toml

+[application]
+manifest = "examples/abitest/config/config.toml"
+out = "examples/abitest/bin/config.bin"
+
 [modules]
 module_0 = { Cargo = { cargo_manifest = "examples/abitest/module_0/rust/Cargo.toml" } }
 module_1 = { Cargo = { cargo_manifest = "examples/abitest/module_1/rust/Cargo.toml" } }


Ah, my bad, this isn't a fixed schema – this comment belongs elsewhere…

daviddrysdale · 2020-07-09T10:41:42Z

examples/translator/example.toml

+module = { Cargo = { cargo_manifest = "examples/translator/module/rust/Cargo.toml" } }
+
+[clients]
+cpp = { Bazel = { bazel_target = "//examples/translator/client:client" } }


…specifically here. There is no C++ in this example.

Ahh good catch! I totally missed this, renamed it now.

tiziano88 · 2020-07-09T16:30:03Z

cc @wildarch FYI (feel free to look around / add comments if you wish)

rbehjati

Thanks. It was a nice learning experience to review this PR :-)

rbehjati · 2020-07-09T21:49:59Z

runner/src/main.rs

            run_cargo_clippy(),
            run_cargo_test(),
+            run_cargo_doc(),
+            run_cargo_test_tsan(),


Suggested change

run_cargo_test_tsan(),

wildarch · 2020-07-10T08:13:03Z

Since the runner will be heavily dependent on cargo, maybe it would make sense for it to be a subcommand?

docs/INSTALL-OSX.md

jul-sh · 2020-07-10T10:32:11Z

docs/programming-oak.md

 ```

-The Oak Runner will launch an [Oak Runtime](concepts.md#oak-runtime), and this
+The Oak Loader will launch an [Oak Runtime](concepts.md#oak-runtime), and this


Glad we changed the naming, makes things clearer :)

jul-sh

lgtm, nice 2 review

jul-sh · 2020-07-10T10:34:34Z

examples/hello_world/example.toml

 [modules]
 module = { Cargo = { cargo_manifest = "examples/hello_world/module/rust/Cargo.toml" } }
 translator = { Cargo = { cargo_manifest = "examples/translator/module/rust/Cargo.toml" } }
+# This module is only built but not actually run.


because we can only run one application per example, currently. I think if we want to support alternative applications, we may as well turn them into separate examples, as @daviddrysdale suggested.

jul-sh · 2020-07-10T10:42:50Z

runner/src/check_build_licenses.rs

+// limitations under the License.
+//
+
+use super::*;


What does this do?

it just imports everything from the parent mod; see https://doc.rust-lang.org/stable/rust-by-example/mod/super.html

In this specific case, which is the parent mod?

Asking since here it's at the top of the file, whereas the rust doc mentions using it in a nested scope. Is it where this is called from?

This mod is not a top level one. The top-level one is the one in main.rs or lib.rs, then each file creates its own scope based on the file name. So the fully qualified name of a type T in this file is really ::check_build_licenses::T. super:: gets back to the parent scope, which is the on in main.rs in this case.

Makes sense, thanks for explaining :)

tiziano88 · 2020-07-10T13:33:11Z

Since the runner will be heavily dependent on cargo, maybe it would make sense for it to be a subcommand?

I thought about that at some point, but I can see no real benefits to it, and it seems to just confuse things more for people who are not familiar with it.

github-actions · 2020-07-10T18:35:13Z

Reproducibility Index:

815d0815e5ca74b2f26d2d619ce6a85e88e2c7941c7e894390b4ad0a39afe9a9  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
457ba75931f7e22ec7ebcaa6ae05d3e0dd6f11570a08f07c2a6fd0b1b42a8d38  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
5b9dc0b57f1bddb468ded79043984ad42cfa75794b9663c5ce7bf0322c9648ec  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
0dc9e4ee50ce2224e39e354a0cbe0dc337d728b9754fd8ef7ab480c48cc4efbe  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
f48fc1118d367ebb47d2765587622670d1b9a52e9858043173340e228a3a8664  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
443b6c87e761073eafe39913b43cab3103bf790ded8a29ae5f700b4afdd46443  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
411ffccff5538a6541991e6665fb660c90c53348aa2e185b85131c02383ecd99  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
57ea2f959de4f1bbb70cf5acc26969726f78bb90866cc087fa77ec01c29ceb28  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
6b450333f5d36c454a186af9add9f29f9326162cb4b5516b41ff9f29cd9b87e0  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
849708ee562d66ec7c0a8b0f65d509bae4c1e4c7381e72b4b1ae9091be7e3454  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
cd393c0c7a97265fdb8ca514cdfcee7d8379d84c1019262a71267c2c7df12491  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
7f1e1ecff2ab9845dbbe8cf93ede831ec7b5d4e0efebe6718e0c9017ec254a3e  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index e81a0b4..94d0bd1 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -3,8 +3,7 @@
 5b9dc0b57f1bddb468ded79043984ad42cfa75794b9663c5ce7bf0322c9648ec  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
 0dc9e4ee50ce2224e39e354a0cbe0dc337d728b9754fd8ef7ab480c48cc4efbe  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
 f48fc1118d367ebb47d2765587622670d1b9a52e9858043173340e228a3a8664  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
-a0c0e2f762165c0928523fdf3a5b60a83604ac3d09f80b5ed84211428f6e5332  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-df1a5f037bce9ca20b556f50c7cd1bf1780cbf0d378609462f18f5d5b8589ffb  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
+443b6c87e761073eafe39913b43cab3103bf790ded8a29ae5f700b4afdd46443  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
 411ffccff5538a6541991e6665fb660c90c53348aa2e185b85131c02383ecd99  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
 57ea2f959de4f1bbb70cf5acc26969726f78bb90866cc087fa77ec01c29ceb28  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
 6b450333f5d36c454a186af9add9f29f9326162cb4b5516b41ff9f29cd9b87e0  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm

googlebot added the cla: yes label Jun 24, 2020

tiziano88 force-pushed the tzn_runner_ci branch 12 times, most recently from dfa524c to 5b479b8 Compare June 29, 2020 21:35

tiziano88 requested review from daviddrysdale, rbehjati and jul-sh June 29, 2020 21:40

tiziano88 added the WIP Work in progress label Jun 29, 2020

tiziano88 marked this pull request as ready for review June 29, 2020 21:41

tiziano88 force-pushed the tzn_runner_ci branch from 5b479b8 to 8da98b3 Compare June 29, 2020 23:12

tiziano88 force-pushed the tzn_runner_ci branch from 8da98b3 to dcb7f2c Compare June 30, 2020 14:56

tiziano88 force-pushed the tzn_runner_ci branch 2 times, most recently from c83ecfd to fd9af90 Compare June 30, 2020 19:29

rbehjati reviewed Jul 1, 2020

View reviewed changes

tiziano88 force-pushed the tzn_runner_ci branch 4 times, most recently from 02cb03e to 2f9c55f Compare July 3, 2020 21:33

tiziano88 added 4 commits July 8, 2020 21:18

deref

cfd6e6b

cloudbuild

7783ec2

timeout

54d0e0a

save docker image

4e8b55b

tiziano88 commented Jul 8, 2020

View reviewed changes

daviddrysdale reviewed Jul 9, 2020

View reviewed changes

panic

32af173

daviddrysdale reviewed Jul 9, 2020

View reviewed changes

Go

538887d

tiziano88 requested a review from daviddrysdale July 9, 2020 14:43

tiziano88 removed the WIP Work in progress label Jul 9, 2020

tiziano88 requested review from rbehjati and jul-sh July 9, 2020 19:27

rbehjati approved these changes Jul 9, 2020

View reviewed changes

remove extra tsan

798b94e

jul-sh reviewed Jul 10, 2020

View reviewed changes

docs/INSTALL-OSX.md Outdated Show resolved Hide resolved

jul-sh reviewed Jul 10, 2020

View reviewed changes

jul-sh approved these changes Jul 10, 2020

View reviewed changes

osx

88a413b

daviddrysdale removed their request for review July 10, 2020 14:40

tiziano88 mentioned this pull request Jul 10, 2020

allow running the C++ hello world module with runner #1250

Closed

tiziano88 added 3 commits July 10, 2020 16:39

Fix coverage

a0ea3fb

coverage

928617d

coverage

85f1906

tiziano88 merged commit c099895 into project-oak:main Jul 10, 2020

tiziano88 deleted the tzn_runner_ci branch July 10, 2020 17:55

Run tests in CI with runner #1190

Run tests in CI with runner #1190

Conversation

tiziano88 commented Jun 24, 2020 • edited Loading

tiziano88 commented Jun 29, 2020

daviddrysdale commented Jun 30, 2020

tiziano88 commented Jun 30, 2020 • edited Loading

rbehjati left a 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

tiziano88 left a 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

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

tiziano88 commented Jul 9, 2020

rbehjati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildarch commented Jul 10, 2020

Choose a reason for hiding this comment

jul-sh left a 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

tiziano88 commented Jul 10, 2020

github-actions bot commented Jul 10, 2020

Run tests in CI with `runner` #1190

Run tests in CI with `runner` #1190

tiziano88 commented Jun 24, 2020 •

edited

Loading

tiziano88 commented Jun 30, 2020 •

edited

Loading