Skip to content
This repository was archived by the owner on May 4, 2024. It is now read-only.

[Movey] Call Movey API to increase download count #318

Closed
wants to merge 9 commits into from
Closed

[Movey] Call Movey API to increase download count #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
73d3eb4
Call to Movey API to increase download count
ea-open-source Jul 28, 2022
cf607f5
Change `skip_movey_call` to false in test sources, added unit tests t…
ea-open-source Aug 2, 2022
aa62911
Separate `increase_movey_download_count` from `download_and_update_if…
ea-open-source Aug 3, 2022
cd2f041
Refactor `increase_movey_download_count` unit tests
ea-open-source Aug 4, 2022
7a297be
Refactor sleep logic of `increase_movey_download_count` unit tests
ea-open-source Aug 5, 2022
2197bee
Refactor comments of the `wait` function
ea-open-source Aug 8, 2022
5f0300d
Refactor mock movey server function, rename `wait` function
ea-open-source Aug 10, 2022
4289293
Rebase and rename `download_and_update_if_repo` to `download_and_upda…
ea-open-source Aug 16, 2022
cbe1000
Refactor according to review
ea-open-source Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion devtools/x/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions language/move-command-line-common/src/movey_constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
1 change: 1 addition & 0 deletions language/tools/move-package/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ whoami = { version = "1.2.1" }

[dev-dependencies]
datatest-stable = "0.1.1"
httpmock = "0.6.6"

[[test]]
name = "test_runner"
Expand Down
4 changes: 4 additions & 0 deletions language/tools/move-package/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", global = true)]
pub skip_movey: bool,
}

#[derive(Debug, Clone, Eq, PartialEq, PartialOrd)]
Expand Down
121 changes: 118 additions & 3 deletions language/tools/move-package/src/resolution/resolution_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@ 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,
},
},
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};
Expand All @@ -28,6 +31,7 @@ use std::{
path::{Path, PathBuf},
process::Command,
rc::Rc,
thread,
};

pub type ResolvedTable = ResolutionTable<AccountAddress>;
Expand Down Expand Up @@ -388,6 +392,18 @@ impl ResolvingGraph {
dep: Dependency,
root_path: PathBuf,
) -> Result<(Renaming, ResolvingTable)> {
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)?;
let (dep_package, dep_package_dir) =
Self::parse_package_manifest(&dep, &dep_name_in_pkg, root_path)
Expand Down Expand Up @@ -526,8 +542,19 @@ impl ResolvingGraph {
};

for (dep_name, dep) in manifest.dependencies.iter().chain(additional_deps.iter()) {
Self::download_and_update_if_remote(*dep_name, dep)?;
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)?;
Copy link
Member

@tnowacki tnowacki Sep 9, 2022

Choose a reason for hiding this comment

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

I feel like it would make more sense to include this logic in download_and_update_if_remote. Is there some reason not to?

Copy link
Contributor Author

@ea-open-source ea-open-source Sep 12, 2022

Choose a reason for hiding this comment

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

This was our solution originally, however, we decided to move the call to update_download_count outside of the download_and_update_if_remote. We do this in order to write tests and separate Movey logic and download_and_update_if_remote logic as much as possible.

We can move update_download_count logic inside download_and_update_if_remote but doing that would complicate the testing logic.

let (dep_manifest, _) =
Self::parse_package_manifest(dep, dep_name, root_path.to_path_buf())
.with_context(|| format!("While processing dependency '{}'", *dep_name))?;
Expand Down Expand Up @@ -572,6 +599,20 @@ impl ResolvingGraph {
}
Ok(())
}

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();
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(&params)
.send();
});
}
}

impl ResolvingPackage {
Expand Down Expand Up @@ -813,3 +854,77 @@ impl ResolvedPackage {
}
}
}

#[cfg(test)]
mod tests {
use crate::{
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};

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 test_subdir = "test_subdir";
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)
}

fn test_thread_wait(wait_time: Option<u64>) {
// make sure the spawn thread has enough time to run
let thread_sleep_time =
time::Duration::from_millis(wait_time.unwrap_or(THREAD_WAIT_INTERVAL));
thread::sleep(thread_sleep_time);
}

#[test]
fn increase_movey_download_count_calls_movey_api() {
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);
test_thread_wait(None);
server_mock.assert_hits(1);
}

#[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);
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) =
mock_movey_count_request_with_response_status_code(&server, 500);
ResolvingGraph::increase_movey_download_count(server.base_url(), &git_info);
test_thread_wait(None);
server_mock.assert_hits(1);
}
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions ...tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions .../test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions .../move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions ...ge/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/one_dep_assigned_address/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/one_dep_renamed/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/one_dep_with_scripts/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/compilation/test_symlinks/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ CompiledPackageInfo {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
}
1 change: 1 addition & 0 deletions ...ge/tools/move-package/tests/test_sources/parsing/invalid_identifier_package_name/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/parsing/minimal_manifest/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/resolution/basic_no_deps/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions .../tools/move-package/tests/test_sources/resolution/basic_no_deps_address_assigned/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions ...s/test_sources/resolution/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/resolution/dep_good_digest/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions ...s/move-package/tests/test_sources/resolution/diamond_problem_backflow_resolution/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions ...age/tools/move-package/tests/test_sources/resolution/diamond_problem_no_conflict/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/resolution/multiple_deps_rename/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/resolution/one_dep/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions language/tools/move-package/tests/test_sources/resolution/one_dep_assigned_address/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
1 change: 1 addition & 0 deletions ...e/tools/move-package/tests/test_sources/resolution/one_dep_multiple_of_same_name/Move.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ResolutionGraph {
additional_named_addresses: {},
architecture: None,
fetch_deps_only: false,
skip_movey: false,
},
root_package: SourceManifest {
package: PackageInfo {
Expand Down
Loading