From 73d3eb4ae3bfde3d3bfc4784ac22496d798fd37c Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Thu, 28 Jul 2022 17:25:38 +0700 Subject: [PATCH 1/9] Call to Movey API to increase download count --- Cargo.lock | 6 +++ language/tools/move-package/src/lib.rs | 4 ++ .../src/resolution/resolution_graph.rs | 37 +++++++++++++++++-- .../compilation/basic_no_deps/Move.exp | 1 + .../basic_no_deps_address_assigned/Move.exp | 1 + .../Move.exp | 1 + .../basic_no_deps_test_mode/Move.exp | 1 + .../Move.exp | 1 + .../diamond_problem_no_conflict/Move.exp | 1 + .../compilation/multiple_deps_rename/Move.exp | 1 + .../multiple_deps_rename_one/Move.exp | 1 + .../test_sources/compilation/one_dep/Move.exp | 1 + .../one_dep_assigned_address/Move.exp | 1 + .../compilation/one_dep_renamed/Move.exp | 1 + .../compilation/one_dep_with_scripts/Move.exp | 1 + .../compilation/test_symlinks/Move.exp | 1 + 16 files changed, 56 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ddbe544fa1..a921ef984b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6058,6 +6058,12 @@ dependencies = [ "serde 1.0.143", ] +[[package]] +name = "tower-service" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6bc1c9ce2b5135ac7f93c72918fc37feb872bdc6a5533a8b85eb4b86bfdae52" + [[package]] name = "toml_edit" version = "0.14.4" diff --git a/language/tools/move-package/src/lib.rs b/language/tools/move-package/src/lib.rs index bba42a818b..1ef66c3220 100644 --- a/language/tools/move-package/src/lib.rs +++ b/language/tools/move-package/src/lib.rs @@ -130,6 +130,10 @@ pub struct BuildConfig { /// Only fetch dependency repos to MOVE_HOME #[clap(long = "fetch-deps-only", global = true)] pub fetch_deps_only: bool, + + /// Skip the call to Movey API to increase download count + #[clap(long = "skip-movey-call", global = true)] + pub skip_movey_call: bool, } #[derive(Debug, Clone, Eq, PartialEq, PartialOrd)] diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index 19059009aa..f53f48f599 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -16,7 +16,10 @@ use crate::{ BuildConfig, }; use anyhow::{bail, Context, Result}; -use move_command_line_common::files::{find_move_filenames, FileHash}; +use move_command_line_common::{ + files::{find_move_filenames, FileHash}, + movey_constants, +}; use move_core_types::account_address::AccountAddress; use move_symbol_pool::Symbol; use petgraph::{algo, graphmap::DiGraphMap, Outgoing}; @@ -28,6 +31,7 @@ use std::{ path::{Path, PathBuf}, process::Command, rc::Rc, + thread }; pub type ResolvedTable = ResolutionTable; @@ -388,7 +392,11 @@ impl ResolvingGraph { dep: Dependency, root_path: PathBuf, ) -> Result<(Renaming, ResolvingTable)> { - Self::download_and_update_if_remote(dep_name_in_pkg, &dep)?; + Self::download_and_update_if_repo( + dep_name_in_pkg, + &dep, + self.build_options.skip_movey_call + )?; let (dep_package, dep_package_dir) = Self::parse_package_manifest(&dep, &dep_name_in_pkg, root_path) .with_context(|| format!("While processing dependency '{}'", dep_name_in_pkg))?; @@ -526,7 +534,7 @@ impl ResolvingGraph { }; for (dep_name, dep) in manifest.dependencies.iter().chain(additional_deps.iter()) { - Self::download_and_update_if_remote(*dep_name, dep)?; + Self::download_and_update_if_repo(*dep_name, dep, build_options.skip_movey_call)?; let (dep_manifest, _) = Self::parse_package_manifest(dep, dep_name, root_path.to_path_buf()) @@ -537,9 +545,19 @@ impl ResolvingGraph { Ok(()) } - fn download_and_update_if_remote(dep_name: PackageName, dep: &Dependency) -> Result<()> { + fn download_and_update_if_repo( + dep_name: PackageName, + dep: &Dependency, + skip_movey_call: bool + ) -> Result<()> { if let Some(git_info) = &dep.git_info { if !git_info.download_to.exists() { + if !skip_movey_call { + let git_url = git_info.git_url.as_str().to_string(); + let git_rev = git_info.git_rev.as_str().to_string(); + let subdir = git_info.subdir.to_string_lossy().to_string(); + Self::increase_movey_download_count(git_url, git_rev, subdir); + } Command::new("git") .args([ "clone", @@ -572,6 +590,17 @@ impl ResolvingGraph { } Ok(()) } + + fn increase_movey_download_count(git_url: String, git_rev: String, subdir: String) { + thread::spawn(move || { + let params = [("url", git_url), ("rev", git_rev), ("subdir", subdir)]; + let client = reqwest::blocking::Client::new(); + let _ = client + .post(&format!("{}/api/v1/download", movey_constants::MOVEY_URL)) + .form(¶ms) + .send(); + }); + } } impl ResolvingPackage { diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp index 9974fc0647..8978ff225c 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp @@ -16,5 +16,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp index 3862f486ac..28d879cd70 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp index f8c5b1d503..b7c28daffb 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp index af0fe4123f..95986cc560 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp index 9d61982e1b..8856634613 100644 --- a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp @@ -19,5 +19,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp index 9d61982e1b..8856634613 100644 --- a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp @@ -19,5 +19,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp index c95634f0ae..2dbadb0fc0 100644 --- a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp @@ -20,5 +20,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp index 81b4a35692..6a5a5f6e1e 100644 --- a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp @@ -20,5 +20,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp index baed78ba25..f4fa70cf7e 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp index c21c42001e..eaeeb59ff8 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp index baed78ba25..f4fa70cf7e 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp index baed78ba25..f4fa70cf7e 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp b/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp index 3862f486ac..28d879cd70 100644 --- a/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp @@ -18,5 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: true, }, } From cf607f50bbe351bdf0eac93c01d88cc0e7568639 Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Tue, 2 Aug 2022 18:04:54 +0700 Subject: [PATCH 2/9] Change `skip_movey_call` to false in test sources, added unit tests to `increase_movey_download_count` --- Cargo.lock | 10 ++ devtools/x/Cargo.toml | 2 +- language/tools/move-package/Cargo.toml | 1 + .../src/resolution/resolution_graph.rs | 153 +++++++++++++++--- .../compilation/basic_no_deps/Move.exp | 2 +- .../basic_no_deps_address_assigned/Move.exp | 2 +- .../Move.exp | 2 +- .../basic_no_deps_test_mode/Move.exp | 2 +- .../Move.exp | 2 +- .../diamond_problem_no_conflict/Move.exp | 2 +- .../compilation/multiple_deps_rename/Move.exp | 2 +- .../multiple_deps_rename_one/Move.exp | 2 +- .../test_sources/compilation/one_dep/Move.exp | 2 +- .../one_dep_assigned_address/Move.exp | 2 +- .../compilation/one_dep_renamed/Move.exp | 2 +- .../compilation/one_dep_with_scripts/Move.exp | 2 +- .../compilation/test_symlinks/Move.exp | 2 +- .../invalid_identifier_package_name/Move.exp | 1 + .../parsing/minimal_manifest/Move.exp | 1 + .../resolution/basic_no_deps/Move.exp | 1 + .../basic_no_deps_address_assigned/Move.exp | 1 + .../Move.exp | 1 + .../resolution/dep_good_digest/Move.exp | 1 + .../Move.exp | 1 + .../diamond_problem_no_conflict/Move.exp | 1 + .../resolution/multiple_deps_rename/Move.exp | 1 + .../test_sources/resolution/one_dep/Move.exp | 1 + .../one_dep_assigned_address/Move.exp | 1 + .../one_dep_multiple_of_same_name/Move.exp | 1 + .../one_dep_reassigned_address/Move.exp | 1 + .../Move.exp | 1 + 31 files changed, 171 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a921ef984b..f64d6662aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -824,6 +824,15 @@ dependencies = [ "itertools 0.10.1", ] +[[package]] +name = "concurrent-queue" +version = "1.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af4780a44ab5696ea9e28294517f1fffb421a83a25af521333c838635509db9c" +dependencies = [ + "cache-padded", +] + [[package]] name = "combine" version = "4.6.4" @@ -3313,6 +3322,7 @@ dependencies = [ "dirs-next", "evm-exec-utils", "hex", + "httpmock", "itertools 0.10.1", "move-abigen", "move-binary-format", diff --git a/devtools/x/Cargo.toml b/devtools/x/Cargo.toml index f4a0ab058f..07b13c98d1 100644 --- a/devtools/x/Cargo.toml +++ b/devtools/x/Cargo.toml @@ -20,7 +20,7 @@ guppy = { version = "0.12.3", features = ["summaries"] } indoc = "1.0.3" toml = "0.5.8" env_logger = "0.8.3" -log = "0.4.14" +log = "0.4.17" chrono = "0.4.19" globset = "0.4.6" regex = "1.5.5" diff --git a/language/tools/move-package/Cargo.toml b/language/tools/move-package/Cargo.toml index ddc3f42580..e46aecb88e 100644 --- a/language/tools/move-package/Cargo.toml +++ b/language/tools/move-package/Cargo.toml @@ -47,6 +47,7 @@ whoami = { version = "1.2.1" } [dev-dependencies] datatest-stable = "0.1.1" +httpmock = "0.6.6" [[test]] name = "test_runner" diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index f53f48f599..c5761bd904 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -9,7 +9,7 @@ use crate::{ layout::SourcePackageLayout, manifest_parser::{parse_move_manifest_string, parse_source_manifest}, parsed_manifest::{ - Dependencies, Dependency, FileName, NamedAddress, PackageDigest, PackageName, + Dependencies, Dependency, FileName, GitInfo, NamedAddress, PackageDigest, PackageName, SourceManifest, SubstOrRename, }, }, @@ -31,7 +31,7 @@ use std::{ path::{Path, PathBuf}, process::Command, rc::Rc, - thread + thread, }; pub type ResolvedTable = ResolutionTable; @@ -395,7 +395,7 @@ impl ResolvingGraph { Self::download_and_update_if_repo( dep_name_in_pkg, &dep, - self.build_options.skip_movey_call + self.build_options.skip_movey_call, )?; let (dep_package, dep_package_dir) = Self::parse_package_manifest(&dep, &dep_name_in_pkg, root_path) @@ -548,16 +548,15 @@ impl ResolvingGraph { fn download_and_update_if_repo( dep_name: PackageName, dep: &Dependency, - skip_movey_call: bool + skip_movey_call: bool, ) -> Result<()> { if let Some(git_info) = &dep.git_info { if !git_info.download_to.exists() { - if !skip_movey_call { - let git_url = git_info.git_url.as_str().to_string(); - let git_rev = git_info.git_rev.as_str().to_string(); - let subdir = git_info.subdir.to_string_lossy().to_string(); - Self::increase_movey_download_count(git_url, git_rev, subdir); - } + increase_movey_download_count( + movey_constants::MOVEY_URL.to_string(), + git_info, + skip_movey_call, + ); Command::new("git") .args([ "clone", @@ -590,17 +589,6 @@ impl ResolvingGraph { } Ok(()) } - - fn increase_movey_download_count(git_url: String, git_rev: String, subdir: String) { - thread::spawn(move || { - let params = [("url", git_url), ("rev", git_rev), ("subdir", subdir)]; - let client = reqwest::blocking::Client::new(); - let _ = client - .post(&format!("{}/api/v1/download", movey_constants::MOVEY_URL)) - .form(¶ms) - .send(); - }); - } } impl ResolvingPackage { @@ -842,3 +830,126 @@ impl ResolvedPackage { } } } + +fn increase_movey_download_count(movey_url: String, git_info: &GitInfo, skip_movey_call: bool) { + if skip_movey_call { + return; + } + let git_url = git_info.git_url.as_str().to_string(); + let git_rev = git_info.git_rev.as_str().to_string(); + let subdir = git_info.subdir.to_string_lossy().to_string(); + thread::spawn(move || { + let params = [("url", git_url), ("rev", git_rev), ("subdir", subdir)]; + let client = reqwest::blocking::Client::new(); + let _ = client + .post(&format!("{}/api/v1/packages/count", movey_url)) + .form(¶ms) + .send(); + }); +} + +#[cfg(test)] +mod tests { + use crate::{ + resolution::resolution_graph::increase_movey_download_count, + source_package::parsed_manifest::GitInfo, + }; + use httpmock::{prelude::*, Mock}; + use move_symbol_pool::Symbol; + use std::{path::PathBuf, thread, time}; + + fn init_mock_server<'a>( + server: &'a MockServer, + git_info: &'a GitInfo, + status_code: u16, + ) -> Mock<'a> { + server.mock(|when, then| { + when.method(POST) + .path("/api/v1/packages/count") + .x_www_form_urlencoded_tuple("url", git_info.git_url.as_str()) + .x_www_form_urlencoded_tuple("rev", git_info.git_rev.as_str()) + .x_www_form_urlencoded_tuple("subdir", "test subdir"); + then.status(status_code); + }) + } + + #[test] + fn increase_movey_download_count_calls_movey_api() { + let git_url = Symbol::from("test git url"); + let git_rev = Symbol::from("test git rev"); + let subdir = PathBuf::from("test subdir"); + let git_info = GitInfo { + git_url, + git_rev, + subdir, + download_to: Default::default(), + }; + let server = MockServer::start(); + let server_mock = init_mock_server(&server, &git_info, 200); + increase_movey_download_count(server.base_url(), &git_info, false); + // make sure the spawn thread has enough time to run + let twenty_millis = time::Duration::from_millis(20); + thread::sleep(twenty_millis); + server_mock.assert(); + } + + #[test] + fn increase_movey_download_count_not_calls_movey_api_if_skip_movey_call_flag_is_true() { + let git_url = Symbol::from("test git url"); + let git_rev = Symbol::from("test git rev"); + let subdir = PathBuf::from("test subdir"); + let git_info = GitInfo { + git_url, + git_rev, + subdir, + download_to: Default::default(), + }; + let server = MockServer::start(); + let server_mock = init_mock_server(&server, &git_info, 200); + increase_movey_download_count(server.base_url(), &git_info, true); + // make sure the spawn thread has enough time to run + let twenty_millis = time::Duration::from_millis(20); + thread::sleep(twenty_millis); + server_mock.assert_hits(0); + } + + #[test] + fn increase_movey_download_count_not_throw_error_if_movey_returns_4xx() { + let git_url = Symbol::from("test git url"); + let git_rev = Symbol::from("test git rev"); + let subdir = PathBuf::from("test subdir"); + let git_info = GitInfo { + git_url, + git_rev, + subdir, + download_to: Default::default(), + }; + let server = MockServer::start(); + let server_mock = init_mock_server(&server, &git_info, 400); + increase_movey_download_count(server.base_url(), &git_info, false); + // make sure the spawn thread has enough time to run + let twenty_millis = time::Duration::from_millis(20); + thread::sleep(twenty_millis); + server_mock.assert(); + } + + #[test] + fn increase_movey_download_count_not_throw_error_if_movey_returns_5xx() { + let git_url = Symbol::from("test git url"); + let git_rev = Symbol::from("test git rev"); + let subdir = PathBuf::from("test subdir"); + let git_info = GitInfo { + git_url, + git_rev, + subdir, + download_to: Default::default(), + }; + let server = MockServer::start(); + let server_mock = init_mock_server(&server, &git_info, 500); + increase_movey_download_count(server.base_url(), &git_info, false); + // make sure the spawn thread has enough time to run + let twenty_millis = time::Duration::from_millis(20); + thread::sleep(twenty_millis); + server_mock.assert(); + } +} diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp index 8978ff225c..ac7b8df5c0 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp @@ -16,6 +16,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp index 28d879cd70..dc126f9ca5 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp index b7c28daffb..6ee624b7ab 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp index 95986cc560..8ddab0b3dd 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp index 8856634613..4287b719bf 100644 --- a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp @@ -19,6 +19,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp index 8856634613..4287b719bf 100644 --- a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp @@ -19,6 +19,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp index 2dbadb0fc0..8f01a5f518 100644 --- a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp @@ -20,6 +20,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp index 6a5a5f6e1e..3f04c1cb47 100644 --- a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp @@ -20,6 +20,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp index f4fa70cf7e..aff465b1fc 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp index eaeeb59ff8..650f9bf5ef 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp index f4fa70cf7e..aff465b1fc 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp index f4fa70cf7e..aff465b1fc 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp b/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp index 28d879cd70..dc126f9ca5 100644 --- a/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: true, + skip_movey_call: false, }, } diff --git a/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp b/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp index 65763af335..bd289e3c02 100644 --- a/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp +++ b/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp b/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp index 6906f4935b..b1d6c04041 100644 --- a/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp +++ b/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp index 88ecefeb75..8572e6fdf6 100644 --- a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp index 7cd6b1f870..183a9da230 100644 --- a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp index 694993bb91..adcf2af231 100644 --- a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp b/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp index c599d4e6d6..ec346563e1 100644 --- a/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp index cd5711fa69..eac5fdf975 100644 --- a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp index f74f6a87ce..1f4ef11fec 100644 --- a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp b/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp index 9e6a787ade..fb19810497 100644 --- a/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp index 0a225bf404..aaac273e6e 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp index f3b6a1384f..5c9e2f88bd 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp index d073d1d5f3..f91e364258 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp index b0fd165cd8..e0d181e9c6 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp index de214ddc88..44d38027fa 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp @@ -12,6 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, + skip_movey_call: false, }, root_package: SourceManifest { package: PackageInfo { From aa62911656b7dc52866b742b79f21167017d11ee Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Wed, 3 Aug 2022 15:25:34 +0700 Subject: [PATCH 3/9] Separate `increase_movey_download_count` from `download_and_update_if_repo`, rename `skip_movey_flag` to `skip_movey` --- language/tools/move-package/src/lib.rs | 4 +- .../src/resolution/resolution_graph.rs | 85 ++++++++++--------- .../compilation/basic_no_deps/Move.exp | 2 +- .../basic_no_deps_address_assigned/Move.exp | 2 +- .../Move.exp | 2 +- .../basic_no_deps_test_mode/Move.exp | 2 +- .../Move.exp | 2 +- .../diamond_problem_no_conflict/Move.exp | 2 +- .../compilation/multiple_deps_rename/Move.exp | 2 +- .../multiple_deps_rename_one/Move.exp | 2 +- .../test_sources/compilation/one_dep/Move.exp | 2 +- .../one_dep_assigned_address/Move.exp | 2 +- .../compilation/one_dep_renamed/Move.exp | 2 +- .../compilation/one_dep_with_scripts/Move.exp | 2 +- .../compilation/test_symlinks/Move.exp | 2 +- .../invalid_identifier_package_name/Move.exp | 2 +- .../parsing/minimal_manifest/Move.exp | 2 +- .../resolution/basic_no_deps/Move.exp | 2 +- .../basic_no_deps_address_assigned/Move.exp | 2 +- .../Move.exp | 2 +- .../resolution/dep_good_digest/Move.exp | 2 +- .../Move.exp | 2 +- .../diamond_problem_no_conflict/Move.exp | 2 +- .../resolution/multiple_deps_rename/Move.exp | 2 +- .../test_sources/resolution/one_dep/Move.exp | 2 +- .../one_dep_assigned_address/Move.exp | 2 +- .../one_dep_multiple_of_same_name/Move.exp | 2 +- .../one_dep_reassigned_address/Move.exp | 2 +- .../Move.exp | 2 +- 29 files changed, 73 insertions(+), 70 deletions(-) diff --git a/language/tools/move-package/src/lib.rs b/language/tools/move-package/src/lib.rs index 1ef66c3220..c1577187a8 100644 --- a/language/tools/move-package/src/lib.rs +++ b/language/tools/move-package/src/lib.rs @@ -132,8 +132,8 @@ pub struct BuildConfig { pub fetch_deps_only: bool, /// Skip the call to Movey API to increase download count - #[clap(long = "skip-movey-call", global = true)] - pub skip_movey_call: bool, + #[clap(long = "skip-movey", global = true)] + pub skip_movey: bool, } #[derive(Debug, Clone, Eq, PartialEq, PartialOrd)] diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index c5761bd904..9d73feb082 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -392,11 +392,16 @@ impl ResolvingGraph { dep: Dependency, root_path: PathBuf, ) -> Result<(Renaming, ResolvingTable)> { - Self::download_and_update_if_repo( - dep_name_in_pkg, - &dep, - self.build_options.skip_movey_call, - )?; + if let Some(git_info) = &dep.git_info { + if !git_info.download_to.exists() { + Self::increase_movey_download_count( + movey_constants::MOVEY_URL.to_string(), + git_info, + self.build_options.skip_movey, + ); + } + } + Self::download_and_update_if_repo(dep_name_in_pkg, &dep)?; let (dep_package, dep_package_dir) = Self::parse_package_manifest(&dep, &dep_name_in_pkg, root_path) .with_context(|| format!("While processing dependency '{}'", dep_name_in_pkg))?; @@ -534,8 +539,16 @@ impl ResolvingGraph { }; for (dep_name, dep) in manifest.dependencies.iter().chain(additional_deps.iter()) { - Self::download_and_update_if_repo(*dep_name, dep, build_options.skip_movey_call)?; - + if let Some(git_info) = &dep.git_info { + if !git_info.download_to.exists() { + Self::increase_movey_download_count( + movey_constants::MOVEY_URL.to_string(), + git_info, + build_options.skip_movey, + ); + } + } + Self::download_and_update_if_repo(*dep_name, dep)?; let (dep_manifest, _) = Self::parse_package_manifest(dep, dep_name, root_path.to_path_buf()) .with_context(|| format!("While processing dependency '{}'", *dep_name))?; @@ -545,18 +558,9 @@ impl ResolvingGraph { Ok(()) } - fn download_and_update_if_repo( - dep_name: PackageName, - dep: &Dependency, - skip_movey_call: bool, - ) -> Result<()> { + fn download_and_update_if_repo(dep_name: PackageName, dep: &Dependency) -> Result<()> { if let Some(git_info) = &dep.git_info { if !git_info.download_to.exists() { - increase_movey_download_count( - movey_constants::MOVEY_URL.to_string(), - git_info, - skip_movey_call, - ); Command::new("git") .args([ "clone", @@ -589,6 +593,23 @@ impl ResolvingGraph { } Ok(()) } + + fn increase_movey_download_count(movey_url: String, git_info: &GitInfo, skip_movey: bool) { + if skip_movey { + return; + } + let git_url = git_info.git_url.as_str().to_string(); + let git_rev = git_info.git_rev.as_str().to_string(); + let subdir = git_info.subdir.to_string_lossy().to_string(); + thread::spawn(move || { + let params = [("url", git_url), ("rev", git_rev), ("subdir", subdir)]; + let client = reqwest::blocking::Client::new(); + let _ = client + .post(&format!("{}/api/v1/packages/count", movey_url)) + .form(¶ms) + .send(); + }); + } } impl ResolvingPackage { @@ -831,28 +852,10 @@ impl ResolvedPackage { } } -fn increase_movey_download_count(movey_url: String, git_info: &GitInfo, skip_movey_call: bool) { - if skip_movey_call { - return; - } - let git_url = git_info.git_url.as_str().to_string(); - let git_rev = git_info.git_rev.as_str().to_string(); - let subdir = git_info.subdir.to_string_lossy().to_string(); - thread::spawn(move || { - let params = [("url", git_url), ("rev", git_rev), ("subdir", subdir)]; - let client = reqwest::blocking::Client::new(); - let _ = client - .post(&format!("{}/api/v1/packages/count", movey_url)) - .form(¶ms) - .send(); - }); -} - #[cfg(test)] mod tests { use crate::{ - resolution::resolution_graph::increase_movey_download_count, - source_package::parsed_manifest::GitInfo, + resolution::resolution_graph::ResolvingGraph, source_package::parsed_manifest::GitInfo, }; use httpmock::{prelude::*, Mock}; use move_symbol_pool::Symbol; @@ -886,7 +889,7 @@ mod tests { }; let server = MockServer::start(); let server_mock = init_mock_server(&server, &git_info, 200); - increase_movey_download_count(server.base_url(), &git_info, false); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); thread::sleep(twenty_millis); @@ -894,7 +897,7 @@ mod tests { } #[test] - fn increase_movey_download_count_not_calls_movey_api_if_skip_movey_call_flag_is_true() { + fn increase_movey_download_count_not_calls_movey_api_if_skip_movey_flag_is_true() { let git_url = Symbol::from("test git url"); let git_rev = Symbol::from("test git rev"); let subdir = PathBuf::from("test subdir"); @@ -906,7 +909,7 @@ mod tests { }; let server = MockServer::start(); let server_mock = init_mock_server(&server, &git_info, 200); - increase_movey_download_count(server.base_url(), &git_info, true); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, true); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); thread::sleep(twenty_millis); @@ -926,7 +929,7 @@ mod tests { }; let server = MockServer::start(); let server_mock = init_mock_server(&server, &git_info, 400); - increase_movey_download_count(server.base_url(), &git_info, false); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); thread::sleep(twenty_millis); @@ -946,7 +949,7 @@ mod tests { }; let server = MockServer::start(); let server_mock = init_mock_server(&server, &git_info, 500); - increase_movey_download_count(server.base_url(), &git_info, false); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); thread::sleep(twenty_millis); diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp index ac7b8df5c0..fb12c0b008 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp @@ -16,6 +16,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp index dc126f9ca5..238f97a737 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp index 6ee624b7ab..e039462ed2 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp index 8ddab0b3dd..393c22feae 100644 --- a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp index 4287b719bf..f46db592c9 100644 --- a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp @@ -19,6 +19,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp index 4287b719bf..f46db592c9 100644 --- a/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp @@ -19,6 +19,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp index 8f01a5f518..5e1250982e 100644 --- a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp @@ -20,6 +20,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp index 3f04c1cb47..6947e90afc 100644 --- a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp @@ -20,6 +20,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp index aff465b1fc..383db4b9de 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp index 650f9bf5ef..dabb7392bf 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp index aff465b1fc..383db4b9de 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp index aff465b1fc..383db4b9de 100644 --- a/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp b/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp index dc126f9ca5..238f97a737 100644 --- a/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp +++ b/language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp @@ -18,6 +18,6 @@ CompiledPackageInfo { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, } diff --git a/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp b/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp index bd289e3c02..849233df06 100644 --- a/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp +++ b/language/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp b/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp index b1d6c04041..0977edae2e 100644 --- a/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp +++ b/language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp index 8572e6fdf6..d0ceb2574c 100644 --- a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp index 183a9da230..bcc606c1fa 100644 --- a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp index adcf2af231..23d1ab8c70 100644 --- a/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp b/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp index ec346563e1..7d2437b6b6 100644 --- a/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp index eac5fdf975..f9f08ed00f 100644 --- a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp index 1f4ef11fec..5c7c100553 100644 --- a/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp b/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp index fb19810497..a24c8bfe9c 100644 --- a/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp index aaac273e6e..7ffd405802 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp index 5c9e2f88bd..633afd308e 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp index f91e364258..d892bbd1e5 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp index e0d181e9c6..12b40821ce 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_reassigned_address/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { diff --git a/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp b/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp index 44d38027fa..5c68b110d1 100644 --- a/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp +++ b/language/tools/move-package/tests/test_sources/resolution/one_dep_unification_across_local_renamings/Move.exp @@ -12,7 +12,7 @@ ResolutionGraph { additional_named_addresses: {}, architecture: None, fetch_deps_only: false, - skip_movey_call: false, + skip_movey: false, }, root_package: SourceManifest { package: PackageInfo { From cd2f041f99c513d69bcfc4021c78b3e06c7da75b Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Thu, 4 Aug 2022 16:44:52 +0700 Subject: [PATCH 4/9] Refactor `increase_movey_download_count` unit tests --- .../src/resolution/resolution_graph.rs | 65 +++++-------------- 1 file changed, 18 insertions(+), 47 deletions(-) diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index 9d73feb082..4aaca6d602 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -861,34 +861,32 @@ mod tests { use move_symbol_pool::Symbol; use std::{path::PathBuf, thread, time}; - fn init_mock_server<'a>( - server: &'a MockServer, - git_info: &'a GitInfo, - status_code: u16, - ) -> Mock<'a> { - server.mock(|when, then| { + fn init_mock_server(server: &MockServer, status_code: u16) -> (Mock, GitInfo) { + let git_url = Symbol::from("test git url"); + let git_rev = Symbol::from("test git rev"); + let subdir = PathBuf::from("test subdir"); + let git_info = GitInfo { + git_url, + git_rev, + subdir, + download_to: Default::default(), + }; + let server_mock = server.mock(|when, then| { when.method(POST) .path("/api/v1/packages/count") .x_www_form_urlencoded_tuple("url", git_info.git_url.as_str()) .x_www_form_urlencoded_tuple("rev", git_info.git_rev.as_str()) .x_www_form_urlencoded_tuple("subdir", "test subdir"); then.status(status_code); - }) + }); + + (server_mock, git_info) } #[test] fn increase_movey_download_count_calls_movey_api() { - let git_url = Symbol::from("test git url"); - let git_rev = Symbol::from("test git rev"); - let subdir = PathBuf::from("test subdir"); - let git_info = GitInfo { - git_url, - git_rev, - subdir, - download_to: Default::default(), - }; let server = MockServer::start(); - let server_mock = init_mock_server(&server, &git_info, 200); + let (server_mock, git_info) = init_mock_server(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); @@ -898,17 +896,8 @@ mod tests { #[test] fn increase_movey_download_count_not_calls_movey_api_if_skip_movey_flag_is_true() { - let git_url = Symbol::from("test git url"); - let git_rev = Symbol::from("test git rev"); - let subdir = PathBuf::from("test subdir"); - let git_info = GitInfo { - git_url, - git_rev, - subdir, - download_to: Default::default(), - }; let server = MockServer::start(); - let server_mock = init_mock_server(&server, &git_info, 200); + let (server_mock, git_info) = init_mock_server(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, true); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); @@ -918,17 +907,8 @@ mod tests { #[test] fn increase_movey_download_count_not_throw_error_if_movey_returns_4xx() { - let git_url = Symbol::from("test git url"); - let git_rev = Symbol::from("test git rev"); - let subdir = PathBuf::from("test subdir"); - let git_info = GitInfo { - git_url, - git_rev, - subdir, - download_to: Default::default(), - }; let server = MockServer::start(); - let server_mock = init_mock_server(&server, &git_info, 400); + let (server_mock, git_info) = init_mock_server(&server, 400); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); @@ -938,17 +918,8 @@ mod tests { #[test] fn increase_movey_download_count_not_throw_error_if_movey_returns_5xx() { - let git_url = Symbol::from("test git url"); - let git_rev = Symbol::from("test git rev"); - let subdir = PathBuf::from("test subdir"); - let git_info = GitInfo { - git_url, - git_rev, - subdir, - download_to: Default::default(), - }; let server = MockServer::start(); - let server_mock = init_mock_server(&server, &git_info, 500); + let (server_mock, git_info) = init_mock_server(&server, 500); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run let twenty_millis = time::Duration::from_millis(20); From 7a297be4badd05aac6d72590b542766ca823bcde Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Fri, 5 Aug 2022 16:43:09 +0700 Subject: [PATCH 5/9] Refactor sleep logic of `increase_movey_download_count` unit tests --- .../src/resolution/resolution_graph.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index 4aaca6d602..6396c8c05f 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -883,14 +883,18 @@ mod tests { (server_mock, git_info) } + fn wait() { + let twenty_millis = time::Duration::from_millis(20); + thread::sleep(twenty_millis); + } + #[test] fn increase_movey_download_count_calls_movey_api() { let server = MockServer::start(); let (server_mock, git_info) = init_mock_server(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run - let twenty_millis = time::Duration::from_millis(20); - thread::sleep(twenty_millis); + wait(); server_mock.assert(); } @@ -900,8 +904,7 @@ mod tests { let (server_mock, git_info) = init_mock_server(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, true); // make sure the spawn thread has enough time to run - let twenty_millis = time::Duration::from_millis(20); - thread::sleep(twenty_millis); + wait(); server_mock.assert_hits(0); } @@ -911,8 +914,7 @@ mod tests { let (server_mock, git_info) = init_mock_server(&server, 400); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run - let twenty_millis = time::Duration::from_millis(20); - thread::sleep(twenty_millis); + wait(); server_mock.assert(); } @@ -922,8 +924,7 @@ mod tests { let (server_mock, git_info) = init_mock_server(&server, 500); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); // make sure the spawn thread has enough time to run - let twenty_millis = time::Duration::from_millis(20); - thread::sleep(twenty_millis); + wait(); server_mock.assert(); } } From 2197beee35dcfc64c8fa39753c0c853164f9ea3c Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Mon, 8 Aug 2022 14:31:45 +0700 Subject: [PATCH 6/9] Refactor comments of the `wait` function --- Cargo.lock | 16 +++++++----- .../src/resolution/resolution_graph.rs | 25 ++++++++----------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f64d6662aa..f2d6fd7cc8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -824,6 +824,16 @@ dependencies = [ "itertools 0.10.1", ] +[[package]] +name = "combine" +version = "4.6.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2a604e93b79d1808327a6fca85a6f2d69de66461e7620f5a4cbf5fb4d1d7c948" +dependencies = [ + "bytes 1.0.1", + "memchr", +] + [[package]] name = "concurrent-queue" version = "1.2.4" @@ -6068,12 +6078,6 @@ dependencies = [ "serde 1.0.143", ] -[[package]] -name = "tower-service" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b6bc1c9ce2b5135ac7f93c72918fc37feb872bdc6a5533a8b85eb4b86bfdae52" - [[package]] name = "toml_edit" version = "0.14.4" diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index 6396c8c05f..c99bed0306 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -883,9 +883,10 @@ mod tests { (server_mock, git_info) } - fn wait() { - let twenty_millis = time::Duration::from_millis(20); - thread::sleep(twenty_millis); + fn wait(wait_time: Option) { + // make sure the spawn thread has enough time to run + let millis = time::Duration::from_millis(wait_time.unwrap_or(20)); + thread::sleep(millis); } #[test] @@ -893,9 +894,8 @@ mod tests { let server = MockServer::start(); let (server_mock, git_info) = init_mock_server(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); - // make sure the spawn thread has enough time to run - wait(); - server_mock.assert(); + wait(None); + server_mock.assert_hits(1); } #[test] @@ -903,8 +903,7 @@ mod tests { let server = MockServer::start(); let (server_mock, git_info) = init_mock_server(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, true); - // make sure the spawn thread has enough time to run - wait(); + wait(None); server_mock.assert_hits(0); } @@ -913,9 +912,8 @@ mod tests { let server = MockServer::start(); let (server_mock, git_info) = init_mock_server(&server, 400); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); - // make sure the spawn thread has enough time to run - wait(); - server_mock.assert(); + wait(None); + server_mock.assert_hits(1); } #[test] @@ -923,8 +921,7 @@ mod tests { let server = MockServer::start(); let (server_mock, git_info) = init_mock_server(&server, 500); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); - // make sure the spawn thread has enough time to run - wait(); - server_mock.assert(); + wait(None); + server_mock.assert_hits(1); } } From 5f0300dcff418ad257c3b72f9a7484c63086b7f4 Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Wed, 10 Aug 2022 14:53:49 +0700 Subject: [PATCH 7/9] Refactor mock movey server function, rename `wait` function - change `init_mock_server` to `mock_movey_count_request_with_response_status_code` - change `wait` to `test_thread_wait` --- .../src/resolution/resolution_graph.rs | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index c99bed0306..3d60917c94 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -861,7 +861,10 @@ mod tests { use move_symbol_pool::Symbol; use std::{path::PathBuf, thread, time}; - fn init_mock_server(server: &MockServer, status_code: u16) -> (Mock, GitInfo) { + fn mock_movey_count_request_with_response_status_code( + server: &MockServer, + status_code: u16, + ) -> (Mock, GitInfo) { let git_url = Symbol::from("test git url"); let git_rev = Symbol::from("test git rev"); let subdir = PathBuf::from("test subdir"); @@ -883,45 +886,49 @@ mod tests { (server_mock, git_info) } - fn wait(wait_time: Option) { + fn test_thread_wait(wait_time: Option) { // make sure the spawn thread has enough time to run - let millis = time::Duration::from_millis(wait_time.unwrap_or(20)); - thread::sleep(millis); + let thread_sleep_time = time::Duration::from_millis(wait_time.unwrap_or(20)); + thread::sleep(thread_sleep_time); } #[test] fn increase_movey_download_count_calls_movey_api() { let server = MockServer::start(); - let (server_mock, git_info) = init_mock_server(&server, 200); + let (server_mock, git_info) = + mock_movey_count_request_with_response_status_code(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); - wait(None); + test_thread_wait(None); server_mock.assert_hits(1); } #[test] fn increase_movey_download_count_not_calls_movey_api_if_skip_movey_flag_is_true() { let server = MockServer::start(); - let (server_mock, git_info) = init_mock_server(&server, 200); + let (server_mock, git_info) = + mock_movey_count_request_with_response_status_code(&server, 200); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, true); - wait(None); + test_thread_wait(None); server_mock.assert_hits(0); } #[test] fn increase_movey_download_count_not_throw_error_if_movey_returns_4xx() { let server = MockServer::start(); - let (server_mock, git_info) = init_mock_server(&server, 400); + let (server_mock, git_info) = + mock_movey_count_request_with_response_status_code(&server, 400); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); - wait(None); + test_thread_wait(None); server_mock.assert_hits(1); } #[test] fn increase_movey_download_count_not_throw_error_if_movey_returns_5xx() { let server = MockServer::start(); - let (server_mock, git_info) = init_mock_server(&server, 500); + let (server_mock, git_info) = + mock_movey_count_request_with_response_status_code(&server, 500); ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); - wait(None); + test_thread_wait(None); server_mock.assert_hits(1); } } From 42892938051ade53a059e6285ef91ed146029f8c Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Tue, 16 Aug 2022 16:25:09 +0700 Subject: [PATCH 8/9] Rebase and rename `download_and_update_if_repo` to `download_and_update_if_remote` --- Cargo.lock | 19 ------------------- .../src/resolution/resolution_graph.rs | 14 +++++++++----- 2 files changed, 9 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f2d6fd7cc8..6fb99cb0c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -824,25 +824,6 @@ dependencies = [ "itertools 0.10.1", ] -[[package]] -name = "combine" -version = "4.6.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2a604e93b79d1808327a6fca85a6f2d69de66461e7620f5a4cbf5fb4d1d7c948" -dependencies = [ - "bytes 1.0.1", - "memchr", -] - -[[package]] -name = "concurrent-queue" -version = "1.2.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af4780a44ab5696ea9e28294517f1fffb421a83a25af521333c838635509db9c" -dependencies = [ - "cache-padded", -] - [[package]] name = "combine" version = "4.6.4" diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index 3d60917c94..07540942ce 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -401,7 +401,8 @@ impl ResolvingGraph { ); } } - Self::download_and_update_if_repo(dep_name_in_pkg, &dep)?; + + Self::download_and_update_if_remote(dep_name_in_pkg, &dep)?; let (dep_package, dep_package_dir) = Self::parse_package_manifest(&dep, &dep_name_in_pkg, root_path) .with_context(|| format!("While processing dependency '{}'", dep_name_in_pkg))?; @@ -548,7 +549,8 @@ impl ResolvingGraph { ); } } - Self::download_and_update_if_repo(*dep_name, dep)?; + + Self::download_and_update_if_remote(*dep_name, dep)?; let (dep_manifest, _) = Self::parse_package_manifest(dep, dep_name, root_path.to_path_buf()) .with_context(|| format!("While processing dependency '{}'", *dep_name))?; @@ -558,7 +560,7 @@ impl ResolvingGraph { Ok(()) } - fn download_and_update_if_repo(dep_name: PackageName, dep: &Dependency) -> Result<()> { + fn download_and_update_if_remote(dep_name: PackageName, dep: &Dependency) -> Result<()> { if let Some(git_info) = &dep.git_info { if !git_info.download_to.exists() { Command::new("git") @@ -598,6 +600,7 @@ impl ResolvingGraph { if skip_movey { return; } + let git_url = git_info.git_url.as_str().to_string(); let git_rev = git_info.git_rev.as_str().to_string(); let subdir = git_info.subdir.to_string_lossy().to_string(); @@ -867,7 +870,8 @@ mod tests { ) -> (Mock, GitInfo) { let git_url = Symbol::from("test git url"); let git_rev = Symbol::from("test git rev"); - let subdir = PathBuf::from("test subdir"); + let test_subdir = "test_subdir"; + let subdir = PathBuf::from(test_subdir); let git_info = GitInfo { git_url, git_rev, @@ -879,7 +883,7 @@ mod tests { .path("/api/v1/packages/count") .x_www_form_urlencoded_tuple("url", git_info.git_url.as_str()) .x_www_form_urlencoded_tuple("rev", git_info.git_rev.as_str()) - .x_www_form_urlencoded_tuple("subdir", "test subdir"); + .x_www_form_urlencoded_tuple("subdir", test_subdir); then.status(status_code); }); From cbe1000af020a3815024a016fd230468ab917ff0 Mon Sep 17 00:00:00 2001 From: ea-open-source Date: Tue, 13 Sep 2022 15:11:14 +0700 Subject: [PATCH 9/9] Refactor according to review --- .../src/movey_constants.rs | 1 + .../src/resolution/resolution_graph.rs | 62 ++++++++----------- 2 files changed, 28 insertions(+), 35 deletions(-) diff --git a/language/move-command-line-common/src/movey_constants.rs b/language/move-command-line-common/src/movey_constants.rs index de98638301..178f3032f8 100644 --- a/language/move-command-line-common/src/movey_constants.rs +++ b/language/move-command-line-common/src/movey_constants.rs @@ -6,3 +6,4 @@ pub const MOVEY_URL: &str = "https://movey-app-staging.herokuapp.com"; #[cfg(not(debug_assertions))] pub const MOVEY_URL: &str = "https://www.movey.net"; pub const MOVEY_CREDENTIAL_PATH: &str = "/movey_credential.toml"; +pub const THREAD_WAIT_INTERVAL: u64 = 20; diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs index 07540942ce..362fadc297 100644 --- a/language/tools/move-package/src/resolution/resolution_graph.rs +++ b/language/tools/move-package/src/resolution/resolution_graph.rs @@ -392,14 +392,16 @@ impl ResolvingGraph { dep: Dependency, root_path: PathBuf, ) -> Result<(Renaming, ResolvingTable)> { - if let Some(git_info) = &dep.git_info { - if !git_info.download_to.exists() { - Self::increase_movey_download_count( - movey_constants::MOVEY_URL.to_string(), - git_info, - self.build_options.skip_movey, - ); - } + let already_downloaded = dep + .git_info + .as_ref() + .map(|gi| gi.download_to.exists()) + .unwrap_or(true); + if !already_downloaded && !self.build_options.skip_movey { + Self::increase_movey_download_count( + movey_constants::MOVEY_URL.to_string(), + dep.git_info.as_ref().unwrap(), + ); } Self::download_and_update_if_remote(dep_name_in_pkg, &dep)?; @@ -540,14 +542,16 @@ impl ResolvingGraph { }; for (dep_name, dep) in manifest.dependencies.iter().chain(additional_deps.iter()) { - if let Some(git_info) = &dep.git_info { - if !git_info.download_to.exists() { - Self::increase_movey_download_count( - movey_constants::MOVEY_URL.to_string(), - git_info, - build_options.skip_movey, - ); - } + let already_downloaded = dep + .git_info + .as_ref() + .map(|gi| gi.download_to.exists()) + .unwrap_or(true); + if !already_downloaded && !build_options.skip_movey { + Self::increase_movey_download_count( + movey_constants::MOVEY_URL.to_string(), + dep.git_info.as_ref().unwrap(), + ); } Self::download_and_update_if_remote(*dep_name, dep)?; @@ -596,11 +600,7 @@ impl ResolvingGraph { Ok(()) } - fn increase_movey_download_count(movey_url: String, git_info: &GitInfo, skip_movey: bool) { - if skip_movey { - return; - } - + fn increase_movey_download_count(movey_url: String, git_info: &GitInfo) { let git_url = git_info.git_url.as_str().to_string(); let git_rev = git_info.git_rev.as_str().to_string(); let subdir = git_info.subdir.to_string_lossy().to_string(); @@ -861,6 +861,7 @@ mod tests { resolution::resolution_graph::ResolvingGraph, source_package::parsed_manifest::GitInfo, }; use httpmock::{prelude::*, Mock}; + use move_command_line_common::movey_constants::THREAD_WAIT_INTERVAL; use move_symbol_pool::Symbol; use std::{path::PathBuf, thread, time}; @@ -892,7 +893,8 @@ mod tests { fn test_thread_wait(wait_time: Option) { // make sure the spawn thread has enough time to run - let thread_sleep_time = time::Duration::from_millis(wait_time.unwrap_or(20)); + let thread_sleep_time = + time::Duration::from_millis(wait_time.unwrap_or(THREAD_WAIT_INTERVAL)); thread::sleep(thread_sleep_time); } @@ -901,27 +903,17 @@ mod tests { let server = MockServer::start(); let (server_mock, git_info) = mock_movey_count_request_with_response_status_code(&server, 200); - ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info); test_thread_wait(None); server_mock.assert_hits(1); } - #[test] - fn increase_movey_download_count_not_calls_movey_api_if_skip_movey_flag_is_true() { - let server = MockServer::start(); - let (server_mock, git_info) = - mock_movey_count_request_with_response_status_code(&server, 200); - ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, true); - test_thread_wait(None); - server_mock.assert_hits(0); - } - #[test] fn increase_movey_download_count_not_throw_error_if_movey_returns_4xx() { let server = MockServer::start(); let (server_mock, git_info) = mock_movey_count_request_with_response_status_code(&server, 400); - ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info); test_thread_wait(None); server_mock.assert_hits(1); } @@ -931,7 +923,7 @@ mod tests { let server = MockServer::start(); let (server_mock, git_info) = mock_movey_count_request_with_response_status_code(&server, 500); - ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info, false); + ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info); test_thread_wait(None); server_mock.assert_hits(1); }