From 2942356bf11c82cb793ecb05a68dd4709e4a3ff8 Mon Sep 17 00:00:00 2001 From: Leo Eichhorn Date: Tue, 12 Dec 2023 12:16:52 +0000 Subject: [PATCH] test(http_utils): CON-1183 Add unit tests for `FileDownloader` --- Cargo.Bazel.Fuzzing.json.lock | 93 ++++++++++++- Cargo.Bazel.Fuzzing.toml.lock | 20 +++ Cargo.Bazel.json.lock | 93 ++++++++++++- Cargo.Bazel.toml.lock | 20 +++ Cargo.lock | 23 ++++ Cargo.toml | 1 + bazel/external_crates.bzl | 3 + rs/http_utils/BUILD.bazel | 41 ++++-- rs/http_utils/Cargo.toml | 6 + rs/http_utils/src/file_downloader.rs | 189 +++++++++++++++++++++++++++ 10 files changed, 474 insertions(+), 15 deletions(-) diff --git a/Cargo.Bazel.Fuzzing.json.lock b/Cargo.Bazel.Fuzzing.json.lock index d3048f96fc9..8016730f55c 100644 --- a/Cargo.Bazel.Fuzzing.json.lock +++ b/Cargo.Bazel.Fuzzing.json.lock @@ -1,5 +1,5 @@ { - "checksum": "f6e95aa4588036f20923951375aa3f5ab06a875c95204667a6d093cd0f858bef", + "checksum": "06e49bd4aa9720bd84debde5ada945567976284611aa4d08d6386023fc9d517a", "crates": { "abnf 0.12.0": { "name": "abnf", @@ -14308,6 +14308,10 @@ "id": "mockall 0.11.4", "target": "mockall" }, + { + "id": "mockito 1.2.0", + "target": "mockito" + }, { "id": "moka 0.12.1", "target": "moka" @@ -30970,6 +30974,93 @@ }, "license": "MIT/Apache-2.0" }, + "mockito 1.2.0": { + "name": "mockito", + "version": "1.2.0", + "repository": { + "Http": { + "url": "https://crates.io/api/v1/crates/mockito/1.2.0/download", + "sha256": "f8d3038e23466858569c2d30a537f691fa0d53b51626630ae08262943e3bbb8b" + } + }, + "targets": [ + { + "Library": { + "crate_name": "mockito", + "crate_root": "src/lib.rs", + "srcs": [ + "**/*.rs" + ] + } + } + ], + "library_target_name": "mockito", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "crate_features": { + "common": [ + "color", + "colored", + "default" + ], + "selects": {} + }, + "deps": { + "common": [ + { + "id": "assert-json-diff 2.0.2", + "target": "assert_json_diff" + }, + { + "id": "colored 2.0.4", + "target": "colored" + }, + { + "id": "futures 0.3.28", + "target": "futures" + }, + { + "id": "hyper 0.14.27", + "target": "hyper" + }, + { + "id": "log 0.4.20", + "target": "log" + }, + { + "id": "rand 0.8.5", + "target": "rand" + }, + { + "id": "regex 1.10.2", + "target": "regex" + }, + { + "id": "serde_json 1.0.107", + "target": "serde_json" + }, + { + "id": "serde_urlencoded 0.7.1", + "target": "serde_urlencoded" + }, + { + "id": "similar 2.3.0", + "target": "similar" + }, + { + "id": "tokio 1.33.0", + "target": "tokio" + } + ], + "selects": {} + }, + "edition": "2021", + "version": "1.2.0" + }, + "license": "MIT" + }, "moka 0.12.1": { "name": "moka", "version": "0.12.1", diff --git a/Cargo.Bazel.Fuzzing.toml.lock b/Cargo.Bazel.Fuzzing.toml.lock index ed53d7d0358..1eff95d830d 100644 --- a/Cargo.Bazel.Fuzzing.toml.lock +++ b/Cargo.Bazel.Fuzzing.toml.lock @@ -2823,6 +2823,7 @@ dependencies = [ "minicbor", "minicbor-derive", "mockall", + "mockito", "moka", "nix 0.24.3", "notify", @@ -6092,6 +6093,25 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "mockito" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8d3038e23466858569c2d30a537f691fa0d53b51626630ae08262943e3bbb8b" +dependencies = [ + "assert-json-diff", + "colored", + "futures", + "hyper 0.14.27", + "log", + "rand 0.8.5", + "regex", + "serde_json", + "serde_urlencoded", + "similar", + "tokio", +] + [[package]] name = "moka" version = "0.12.1" diff --git a/Cargo.Bazel.json.lock b/Cargo.Bazel.json.lock index 482d31c3178..db336db33ec 100644 --- a/Cargo.Bazel.json.lock +++ b/Cargo.Bazel.json.lock @@ -1,5 +1,5 @@ { - "checksum": "30bfaa28584585def5ca7e1d062a015f0e5e0856781e405490d4d9b5741529c2", + "checksum": "20b7bc536ff18d36a1e1b1f35b9618cff4a2fef7ecf1f061ab0b004212e2afb0", "crates": { "abnf 0.12.0": { "name": "abnf", @@ -14161,6 +14161,10 @@ "id": "mockall 0.11.4", "target": "mockall" }, + { + "id": "mockito 1.2.0", + "target": "mockito" + }, { "id": "moka 0.12.1", "target": "moka" @@ -30868,6 +30872,93 @@ }, "license": "MIT/Apache-2.0" }, + "mockito 1.2.0": { + "name": "mockito", + "version": "1.2.0", + "repository": { + "Http": { + "url": "https://crates.io/api/v1/crates/mockito/1.2.0/download", + "sha256": "f8d3038e23466858569c2d30a537f691fa0d53b51626630ae08262943e3bbb8b" + } + }, + "targets": [ + { + "Library": { + "crate_name": "mockito", + "crate_root": "src/lib.rs", + "srcs": [ + "**/*.rs" + ] + } + } + ], + "library_target_name": "mockito", + "common_attrs": { + "compile_data_glob": [ + "**" + ], + "crate_features": { + "common": [ + "color", + "colored", + "default" + ], + "selects": {} + }, + "deps": { + "common": [ + { + "id": "assert-json-diff 2.0.2", + "target": "assert_json_diff" + }, + { + "id": "colored 2.0.4", + "target": "colored" + }, + { + "id": "futures 0.3.28", + "target": "futures" + }, + { + "id": "hyper 0.14.27", + "target": "hyper" + }, + { + "id": "log 0.4.20", + "target": "log" + }, + { + "id": "rand 0.8.5", + "target": "rand" + }, + { + "id": "regex 1.9.1", + "target": "regex" + }, + { + "id": "serde_json 1.0.108", + "target": "serde_json" + }, + { + "id": "serde_urlencoded 0.7.1", + "target": "serde_urlencoded" + }, + { + "id": "similar 2.2.1", + "target": "similar" + }, + { + "id": "tokio 1.32.0", + "target": "tokio" + } + ], + "selects": {} + }, + "edition": "2021", + "version": "1.2.0" + }, + "license": "MIT" + }, "moka 0.12.1": { "name": "moka", "version": "0.12.1", diff --git a/Cargo.Bazel.toml.lock b/Cargo.Bazel.toml.lock index 089c61fdd40..8241a7e9596 100644 --- a/Cargo.Bazel.toml.lock +++ b/Cargo.Bazel.toml.lock @@ -2812,6 +2812,7 @@ dependencies = [ "minicbor", "minicbor-derive", "mockall", + "mockito", "moka", "nix 0.24.3", "notify", @@ -6081,6 +6082,25 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "mockito" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8d3038e23466858569c2d30a537f691fa0d53b51626630ae08262943e3bbb8b" +dependencies = [ + "assert-json-diff", + "colored", + "futures", + "hyper 0.14.27", + "log", + "rand 0.8.5", + "regex", + "serde_json", + "serde_urlencoded", + "similar", + "tokio", +] + [[package]] name = "moka" version = "0.12.1" diff --git a/Cargo.lock b/Cargo.lock index 614db35ead5..402c9f8cd88 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7615,14 +7615,18 @@ dependencies = [ name = "ic-http-utils" version = "0.9.0" dependencies = [ + "assert_matches", "flate2", "hex", "http 0.2.9", "ic-crypto-sha2", "ic-logger", + "ic-test-utilities-in-memory-logger", + "mockito", "reqwest", "slog", "tar", + "tempfile", "tokio", ] @@ -13319,6 +13323,25 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "mockito" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8d3038e23466858569c2d30a537f691fa0d53b51626630ae08262943e3bbb8b" +dependencies = [ + "assert-json-diff", + "colored", + "futures", + "hyper 0.14.26", + "log", + "rand 0.8.5", + "regex", + "serde_json", + "serde_urlencoded", + "similar", + "tokio", +] + [[package]] name = "moka" version = "0.12.1" diff --git a/Cargo.toml b/Cargo.toml index 54879796c01..eec1cf72dbc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -460,6 +460,7 @@ k256 = { version = "0.13.2", default_features = false, features = [ "std", ] } mockall = "0.11.4" +mockito = "1.2.0" minicbor = { version = "0.19.1", features = ["alloc", "derive"] } minicbor-derive = "0.13.0" nix = "0.24.3" diff --git a/bazel/external_crates.bzl b/bazel/external_crates.bzl index ded4530daae..a967993658d 100644 --- a/bazel/external_crates.bzl +++ b/bazel/external_crates.bzl @@ -692,6 +692,9 @@ def external_crates_repository(name, cargo_lockfile, lockfile, sanitizers_enable "mockall": crate.spec( version = "^0.11.4", ), + "mockito": crate.spec( + version = "^1.2.0", + ), "moka": crate.spec( version = "^0.12", features = [ diff --git a/rs/http_utils/BUILD.bazel b/rs/http_utils/BUILD.bazel index 032f632e94a..af49a5a7ce8 100644 --- a/rs/http_utils/BUILD.bazel +++ b/rs/http_utils/BUILD.bazel @@ -1,21 +1,36 @@ -load("@rules_rust//rust:defs.bzl", "rust_library") +load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") package(default_visibility = ["//visibility:public"]) +DEPENDENCIES = [ + "//rs/crypto/sha2", + "//rs/monitoring/logger", + "@crate_index//:flate2", + "@crate_index//:hex", + "@crate_index//:http", + "@crate_index//:reqwest", + "@crate_index//:slog", + "@crate_index//:tar", + "@crate_index//:tokio", +] + +DEV_DEPENDENCIES = [ + "//rs/test_utilities/in_memory_logger", + "@crate_index//:assert_matches", + "@crate_index//:mockito", + "@crate_index//:tempfile", +] + rust_library( name = "http_utils", - srcs = glob(["src/**"]), + srcs = glob(["src/**/*.rs"]), crate_name = "ic_http_utils", version = "0.9.0", - deps = [ - "//rs/crypto/sha2", - "//rs/monitoring/logger", - "@crate_index//:flate2", - "@crate_index//:hex", - "@crate_index//:http", - "@crate_index//:reqwest", - "@crate_index//:slog", - "@crate_index//:tar", - "@crate_index//:tokio", - ], + deps = DEPENDENCIES, +) + +rust_test( + name = "http_utils_test", + crate = ":http_utils", + deps = DEPENDENCIES + DEV_DEPENDENCIES, ) diff --git a/rs/http_utils/Cargo.toml b/rs/http_utils/Cargo.toml index 3d63c83a8d7..49ab21f3b49 100644 --- a/rs/http_utils/Cargo.toml +++ b/rs/http_utils/Cargo.toml @@ -16,3 +16,9 @@ ic-logger = { path = "../monitoring/logger" } reqwest = { workspace = true } slog = { workspace = true } tar = "0.4.38" + +[dev-dependencies] +assert_matches = "1.5.0" +ic-test-utilities-in-memory-logger = { path = "../test_utilities/in_memory_logger" } +mockito = { workspace = true } +tempfile = "3.1.0" diff --git a/rs/http_utils/src/file_downloader.rs b/rs/http_utils/src/file_downloader.rs index 93cfb6649e2..6a49482a498 100644 --- a/rs/http_utils/src/file_downloader.rs +++ b/rs/http_utils/src/file_downloader.rs @@ -384,3 +384,192 @@ pub enum HttpError { /// A redirect response without a required header RedirectMissingHeader(http::Method, http::HeaderName, http::StatusCode), } + +#[cfg(test)] +mod tests { + use assert_matches::assert_matches; + use ic_test_utilities_in_memory_logger::{assertions::LogEntriesAssert, InMemoryReplicaLogger}; + use mockito::{Mock, ServerGuard}; + use slog::Level; + use tempfile::{NamedTempFile, TempPath}; + use tokio::test; + + use super::*; + + struct Setup { + pub server: ServerGuard, + pub data: Mock, + pub redirect: Mock, + pub temp: TempPath, + pub logger: InMemoryReplicaLogger, + } + + impl Setup { + fn new(body: &str) -> Self { + let mut server = mockito::Server::new(); + let redirect = server + .mock("GET", "/redirect") + .with_status(301) + .with_header("Location", &server.url()) + .create(); + let data = server + .mock("GET", "/") + .with_status(200) + .with_body(body) + .create(); + + let temp = NamedTempFile::new() + .expect("Failed to create tmp file") + .into_temp_path(); + std::fs::remove_file(&temp).expect("Failed to remove file"); + + Self { + server, + data, + redirect, + temp, + logger: InMemoryReplicaLogger::new(), + } + } + + fn expect_routes(mut self, data_hits: usize, redirect_hits: usize) -> Self { + self.data = self.data.expect(data_hits); + self.redirect = self.redirect.expect(redirect_hits); + self + } + + fn url(&self) -> String { + self.server.url() + } + + fn assert(&self) { + self.data.assert(); + self.redirect.assert(); + } + } + + fn hash(data: &str) -> String { + let mut hasher = Sha256::new(); + hasher + .write_all(data.as_bytes()) + .expect("Failed to write data"); + hex::encode(hasher.finish()) + } + + #[test] + async fn test_file_downloader_handles_redirects() { + let body = String::from("Success"); + let hash = hash(&body); + let setup = Setup::new(&body).expect_routes(1, 1); + + let downloader = FileDownloader::new(None); + downloader + .download_file( + &format!("{}/redirect", setup.url()), + &setup.temp, + Some(hash), + ) + .await + .expect("Download failed"); + + let result = std::fs::read_to_string(&setup.temp).expect("Failed to read file"); + assert_eq!(result, body); + setup.assert(); + } + + #[test] + async fn test_hash_mismatch_returns_error() { + let body = String::from("Success"); + let hash = hash(&body); + let invalid_hash = format!("invalid_{}", hash); + let setup = Setup::new(&body).expect_routes(1, 0); + + let downloader = FileDownloader::new(Some(ReplicaLogger::from(&setup.logger))); + + let result = downloader + .download_file(&setup.url(), &setup.temp, Some(invalid_hash)) + .await; + assert_matches!(result, Err(FileDownloadError::FileHashMismatchError { .. })); + + setup.assert(); + + let logs = setup.logger.drain_logs(); + LogEntriesAssert::assert_that(logs) + .has_only_one_message_containing(&Level::Info, "Response read"); + } + + #[test] + async fn test_download_succeeds_if_file_exists() { + let body = String::from("Success"); + let hash = hash(&body); + + let setup = Setup::new(&body).expect_routes(1, 0); + + // Correct file already exists + std::fs::write(&setup.temp, &body).unwrap(); + + // Download the file without expected hash (it should be overwritten) + let logger = InMemoryReplicaLogger::new(); + let downloader = FileDownloader::new(Some(ReplicaLogger::from(&logger))); + downloader + .download_file(&setup.url(), &setup.temp, None) + .await + .expect("Download failed"); + + let result = std::fs::read_to_string(&setup.temp).expect("Failed to read file"); + assert_eq!(result, body); + + let logs = logger.drain_logs(); + LogEntriesAssert::assert_that(logs) + .has_exactly_n_messages_containing(0, &Level::Info, "File already exists") + .has_only_one_message_containing(&Level::Info, "Response read"); + + // Download it again, this time expecting the correct hash + let logger = InMemoryReplicaLogger::new(); + let downloader = FileDownloader::new(Some(ReplicaLogger::from(&logger))); + downloader + .download_file(&setup.url(), &setup.temp, Some(hash)) + .await + .expect("Download failed"); + + let result = std::fs::read_to_string(&setup.temp).expect("Failed to read file"); + assert_eq!(result, body); + + // We should not download anything, as the file already exists. + let logs = logger.drain_logs(); + LogEntriesAssert::assert_that(logs) + .has_only_one_message_containing(&Level::Info, "File already exists") + .has_exactly_n_messages_containing(0, &Level::Info, "Response read"); + + setup.assert(); + } + + #[test] + async fn test_download_overwrites_existing_file() { + let body = String::from("Success"); + let hash = hash(&body); + + let setup = Setup::new(&body).expect_routes(1, 0); + + // An unexpected file already exists + std::fs::write(&setup.temp, "unexpected content").unwrap(); + + // It should be overwritten with the correct file + let downloader = FileDownloader::new(Some(ReplicaLogger::from(&setup.logger))); + downloader + .download_file(&setup.url(), &setup.temp, Some(hash)) + .await + .expect("Download failed"); + + let result = std::fs::read_to_string(&setup.temp).expect("Failed to read file"); + assert_eq!(result, body); + + setup.assert(); + + let logs = setup.logger.drain_logs(); + LogEntriesAssert::assert_that(logs).has_only_one_message_containing( + &Level::Warning, + "File already exists, but hash check failed", + ); + } +}