[Move] Allow Bytecode Dependencies for Unit Testing (#15252)

* fix bytecode deps for unit tests * add tests * linter & remove debug print * reset Cargo.lock * add bytecode files * reset Cargo.lock * add .gitkeep for empty dir * address comments * resolve merge conflicts --------- Co-authored-by: Zekun Wang <[email protected]>
aptos-labs · Jan 7, 2025 · aaa3514 · aaa3514
1 parent 0318475
commit aaa3514
Show file tree

Hide file tree

Showing 16 changed files with 202 additions and 24 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/aptos-move/framework/src/built_package.rs b/aptos-move/framework/src/built_package.rs
@@ -18,7 +18,10 @@ use codespan_reporting::{
 use itertools::Itertools;
 use move_binary_format::{file_format_common::VERSION_7, CompiledModule};
 use move_command_line_common::files::MOVE_COMPILED_EXTENSION;
-use move_compiler::compiled_unit::{CompiledUnit, NamedCompiledModule};
+use move_compiler::{
+    compiled_unit::{CompiledUnit, NamedCompiledModule},
+    shared::NumericalAddress,
+};
 use move_compiler_v2::{external_checks::ExternalChecks, options::Options, Experiment};
 use move_core_types::{language_storage::ModuleId, metadata::Metadata};
 use move_model::{
@@ -507,8 +510,9 @@ impl BuiltPackage {
                 self.package
                     .bytecode_deps
                     .iter()
-                    .map(|(name, address)| PackageDep {
-                        account: address.into_inner(),
+                    .map(|(name, module)| PackageDep {
+                        account: NumericalAddress::from_account_address(*module.self_addr())
+                            .into_inner(),
                         package_name: name.as_str().to_string(),
                     }),
             )

diff --git a/third_party/move/move-compiler/src/unit_test/mod.rs b/third_party/move/move-compiler/src/unit_test/mod.rs
@@ -7,6 +7,7 @@ use crate::{
     diagnostics::FilesSourceText,
     shared::NumericalAddress,
 };
+use move_binary_format::CompiledModule;
 use move_core_types::{
     account_address::AccountAddress, identifier::Identifier, language_storage::ModuleId,
     value::MoveValue, vm_status::StatusCode,
@@ -18,11 +19,21 @@ pub mod plan_builder;
 
 pub type TestName = String;
 
+#[derive(Debug, Clone)]
+pub enum NamedOrBytecodeModule {
+    // Compiled from source
+    Named(NamedCompiledModule),
+    // Bytecode dependency
+    Bytecode(CompiledModule),
+}
+
 #[derive(Debug, Clone)]
 pub struct TestPlan {
     pub files: FilesSourceText,
     pub module_tests: BTreeMap<ModuleId, ModuleTestPlan>,
-    pub module_info: BTreeMap<ModuleId, NamedCompiledModule>,
+    // `NamedCompiledModule` for compiled modules with source,
+    // `CompiledModule` for modules with bytecode only
+    pub module_info: BTreeMap<ModuleId, NamedOrBytecodeModule>,
 }
 
 #[derive(Debug, Clone)]
@@ -85,6 +96,7 @@ impl TestPlan {
         tests: Vec<ModuleTestPlan>,
         files: FilesSourceText,
         units: Vec<AnnotatedCompiledUnit>,
+        bytecode_modules: Vec<CompiledModule>,
     ) -> Self {
         let module_tests: BTreeMap<_, _> = tests
             .into_iter()
@@ -97,12 +109,17 @@ impl TestPlan {
                 if let AnnotatedCompiledUnit::Module(annot_module) = unit {
                     Some((
                         annot_module.named_module.module.self_id(),
-                        annot_module.named_module,
+                        NamedOrBytecodeModule::Named(annot_module.named_module),
                     ))
                 } else {
                     None
                 }
             })
+            .chain(
+                bytecode_modules
+                    .into_iter()
+                    .map(|module| (module.self_id(), NamedOrBytecodeModule::Bytecode(module))),
+            )
             .collect();
 
         Self {

diff --git a/third_party/move/tools/move-cli/src/base/test.rs b/third_party/move/tools/move-cli/src/base/test.rs
@@ -241,7 +241,7 @@ pub fn run_move_unit_tests_with_factory<W: Write + Send, F: UnitTestFactory + Se
     // Move package system, to first grab the compilation env, construct the test plan from it, and
     // then save it, before resuming the rest of the compilation and returning the results and
     // control back to the Move package system.
-    let (_compiled_package, model_opt) = build_plan.compile_with_driver(
+    let (compiled_package, model_opt) = build_plan.compile_with_driver(
         writer,
         &build_config.compiler_config,
         vec![],
@@ -306,7 +306,12 @@ pub fn run_move_unit_tests_with_factory<W: Write + Send, F: UnitTestFactory + Se
     files.extend(dep_file_map);
     let test_plan = test_plan.unwrap();
     let no_tests = test_plan.is_empty();
-    let test_plan = TestPlan::new(test_plan, files, units);
+    let test_plan = TestPlan::new(
+        test_plan,
+        files,
+        units,
+        compiled_package.bytecode_deps.into_values().collect(),
+    );
 
     let trace_path = pkg_path.join(".trace");
     let coverage_map_path = pkg_path

diff --git a/third_party/move/tools/move-package/src/compilation/compiled_package.rs b/third_party/move/tools/move-package/src/compilation/compiled_package.rs
@@ -83,7 +83,7 @@ pub struct CompiledPackage {
     /// The output compiled bytecode for dependencies
     pub deps_compiled_units: Vec<(PackageName, CompiledUnitWithSource)>,
     /// Bytecode dependencies of this compiled package
-    pub bytecode_deps: BTreeMap<PackageName, NumericalAddress>,
+    pub bytecode_deps: BTreeMap<PackageName, CompiledModule>,
 
     // Optional artifacts from compilation
     //
@@ -809,8 +809,7 @@ impl CompiledPackage {
                 .flat_map(|package| {
                     let name = package.name.unwrap();
                     package.paths.iter().map(move |pkg_path| {
-                        get_addr_from_module_in_package(name, pkg_path.as_str())
-                            .map(|addr| (name, addr))
+                        get_module_in_package(name, pkg_path.as_str()).map(|module| (name, module))
                     })
                 })
                 .try_collect()?,
@@ -1166,8 +1165,8 @@ pub fn build_and_report_no_exit_v2_driver(
     ))
 }
 
-/// Returns the address of the module
-fn get_addr_from_module_in_package(pkg_name: Symbol, pkg_path: &str) -> Result<NumericalAddress> {
+/// Returns the deserialized module from the bytecode file
+fn get_module_in_package(pkg_name: Symbol, pkg_path: &str) -> Result<CompiledModule> {
     // Read the bytecode file
     let mut bytecode = Vec::new();
     std::fs::File::open(pkg_path)
@@ -1183,5 +1182,4 @@ fn get_addr_from_module_in_package(pkg_name: Symbol, pkg_path: &str) -> Result<N
                 pkg_name
             ))
         })
-        .map(|module| NumericalAddress::from_account_address(*module.self_addr()))
 }
diff --git a/third_party/move/tools/move-unit-test/Cargo.toml b/third_party/move/tools/move-unit-test/Cargo.toml
@@ -43,6 +43,10 @@ regex = { workspace = true }
 [dev-dependencies]
 datatest-stable = { workspace = true }
 difference = { workspace = true }
+move-cli = { path = "../move-cli" }
+move-model = { path = "../../move-model" }
+move-package = { path = "../move-package" }
+tempfile = { workspace = true }
 
 [[bin]]
 name = "move-unit-test"

diff --git a/third_party/move/tools/move-unit-test/src/lib.rs b/third_party/move/tools/move-unit-test/src/lib.rs
@@ -214,7 +214,7 @@ impl UnitTestingConfig {
             diagnostics::report_warnings(&files, warnings);
             (test_plan, files, units)
         };
-        test_plan.map(|tests| TestPlan::new(tests, files, units))
+        test_plan.map(|tests| TestPlan::new(tests, files, units, vec![]))
     }
 
     /// Build a test plan from a unit test config

diff --git a/third_party/move/tools/move-unit-test/src/test_reporter.rs b/third_party/move/tools/move-unit-test/src/test_reporter.rs
@@ -13,7 +13,7 @@ use move_command_line_common::{env::read_bool_env_var, files::FileHash};
 pub use move_compiler::unit_test::ExpectedMoveError as MoveError;
 use move_compiler::{
     diagnostics::{self, Diagnostic, Diagnostics},
-    unit_test::{ModuleTestPlan, TestName, TestPlan},
+    unit_test::{ModuleTestPlan, NamedOrBytecodeModule, TestName, TestPlan},
 };
 use move_core_types::{effects::ChangeSet, language_storage::ModuleId, vm_status::StatusType};
 use move_ir_types::location::Loc;
@@ -357,7 +357,10 @@ impl TestFailure {
                     None => return "\tmalformed stack trace (no module ID)".to_string(),
                 };
                 let named_module = match test_plan.module_info.get(module_id) {
-                    Some(v) => v,
+                    Some(NamedOrBytecodeModule::Named(v)) => v,
+                    Some(NamedOrBytecodeModule::Bytecode(_)) => {
+                        return "\tno source map for bytecode module".to_string()
+                    },
                     None => return "\tmalformed stack trace (no module)".to_string(),
                 };
                 let function_source_map =
@@ -408,12 +411,15 @@ impl TestFailure {
         let mut diags = match vm_error.location() {
             Location::Module(module_id) => {
                 let diag_opt = vm_error.offsets().first().and_then(|(fdef_idx, offset)| {
-                    let function_source_map = test_plan
-                        .module_info
-                        .get(module_id)?
-                        .source_map
-                        .get_function_source_map(*fdef_idx)
-                        .ok()?;
+                    let function_source_map = match test_plan.module_info.get(module_id)? {
+                        NamedOrBytecodeModule::Named(named_compiled_module) => {
+                            named_compiled_module
+                                .source_map
+                                .get_function_source_map(*fdef_idx)
+                                .ok()?
+                        },
+                        NamedOrBytecodeModule::Bytecode(_compiled_module) => return None,
+                    };
                     let loc = function_source_map.get_code_location(*offset).unwrap();
                     let msg = format!("In this function in {}", format_module_id(module_id));
                     // TODO(tzakian) maybe migrate off of move-langs diagnostics?

diff --git a/third_party/move/tools/move-unit-test/src/test_runner.rs b/third_party/move/tools/move-unit-test/src/test_runner.rs
@@ -13,7 +13,9 @@ use anyhow::Result;
 use colored::*;
 use move_binary_format::{errors::VMResult, file_format::CompiledModule};
 use move_bytecode_utils::Modules;
-use move_compiler::unit_test::{ExpectedFailure, ModuleTestPlan, TestCase, TestPlan};
+use move_compiler::unit_test::{
+    ExpectedFailure, ModuleTestPlan, NamedOrBytecodeModule, TestCase, TestPlan,
+};
 use move_core_types::{
     account_address::AccountAddress,
     effects::{ChangeSet, Op},
@@ -131,7 +133,10 @@ impl TestRunner {
             .values()
             .map(|(filepath, _)| filepath.to_string())
             .collect();
-        let modules = tests.module_info.values().map(|info| &info.module);
+        let modules = tests.module_info.values().map(|info| match info {
+            NamedOrBytecodeModule::Named(named_compiled_module) => &named_compiled_module.module,
+            NamedOrBytecodeModule::Bytecode(compiled_module) => compiled_module,
+        });
         let mut starting_storage_state = setup_test_storage(modules)?;
         if let Some(genesis_state) = genesis_state {
             starting_storage_state.apply(genesis_state)?;

diff --git a/third_party/move/tools/move-unit-test/tests/packages/dep/Move.toml b/third_party/move/tools/move-unit-test/tests/packages/dep/Move.toml
@@ -0,0 +1,4 @@
+[package]
+name = "Dep"
+version = "1.0.0"
+authors = []
diff --git a/third_party/move/tools/move-unit-test/tests/packages/dep/build/Dep/BuildInfo.yaml b/third_party/move/tools/move-unit-test/tests/packages/dep/build/Dep/BuildInfo.yaml
@@ -0,0 +1,46 @@
+---
+compiled_package_info:
+  package_name: Dep
+  address_alias_instantiation: {}
+  source_digest: B01B575F8492F72C72DBF502E1F788F49A9CA8AC335FB9E52B0455ACA35677E0
+  build_flags:
+    dev_mode: false
+    test_mode: false
+    override_std: ~
+    generate_docs: false
+    generate_abis: false
+    generate_move_model: true
+    full_model_generation: false
+    install_dir: ~
+    force_recompilation: false
+    additional_named_addresses: {}
+    architecture: ~
+    fetch_deps_only: false
+    skip_fetch_latest_git_deps: false
+    compiler_config:
+      bytecode_version: 7
+      known_attributes:
+        - bytecode_instruction
+        - deprecated
+        - event
+        - expected_failure
+        - "fmt::skip"
+        - legacy_entry_fun
+        - "lint::allow_unsafe_randomness"
+        - "lint::skip"
+        - "mutation::skip"
+        - native_interface
+        - randomness
+        - resource_group
+        - resource_group_member
+        - test
+        - test_only
+        - verify_only
+        - view
+      skip_attribute_checks: false
+      compiler_version: V2_0
+      language_version: V2_1
+      experiments:
+        - optimize=on
+dependencies: []
+bytecode_deps: []
diff --git a/third_party/move/tools/move-unit-test/tests/packages/dep/build/Dep/bytecode_modules/foo.mv b/third_party/move/tools/move-unit-test/tests/packages/dep/build/Dep/bytecode_modules/foo.mv
diff --git a/third_party/move/tools/move-unit-test/tests/packages/dep/sources/.gitkeep b/third_party/move/tools/move-unit-test/tests/packages/dep/sources/.gitkeep
diff --git a/third_party/move/tools/move-unit-test/tests/packages/one-bytecode-dep/Move.toml b/third_party/move/tools/move-unit-test/tests/packages/one-bytecode-dep/Move.toml
@@ -0,0 +1,14 @@
+[package]
+name = "unit-test"
+version = "1.0.0"
+authors = []
+
+[addresses]
+
+[dev-addresses]
+
+[dependencies]
+Dep = { local = "../dep" }
+MoveStdlib = { local = "../../../../../../../aptos-move/framework/move-stdlib" }
+
+[dev-dependencies]
diff --git a/third_party/move/tools/move-unit-test/tests/packages/one-bytecode-dep/sources/test.move b/third_party/move/tools/move-unit-test/tests/packages/one-bytecode-dep/sources/test.move
@@ -0,0 +1,9 @@
+module 0x42::test {
+	#[test_only]
+	use 0x42::foo;
+
+	#[test]
+	fun test() {
+		assert!(foo::foo() == 42, 0);
+	}
+}
diff --git a/third_party/move/tools/move-unit-test/tests/pkg_tests.rs b/third_party/move/tools/move-unit-test/tests/pkg_tests.rs
@@ -0,0 +1,64 @@
+// Copyright (c) Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+use move_cli::base::test::{run_move_unit_tests, UnitTestResult};
+use move_core_types::{account_address::AccountAddress, effects::ChangeSet};
+use move_model::metadata::CompilerVersion;
+use move_package::CompilerConfig;
+use move_stdlib::natives::{all_natives, GasParameters};
+use move_unit_test::UnitTestingConfig;
+use std::path::PathBuf;
+use tempfile::tempdir;
+
+pub fn path_in_crate<S>(relative: S) -> PathBuf
+where
+    S: Into<String>,
+{
+    let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    path.push(relative.into());
+    path
+}
+
+fn run_tests_for_pkg(path_to_pkg: impl Into<String>, v2: bool) {
+    let pkg_path = path_in_crate(path_to_pkg);
+
+    let natives = all_natives(
+        AccountAddress::from_hex_literal("0x1").unwrap(),
+        GasParameters::zeros(),
+    );
+
+    let result = run_move_unit_tests(
+        &pkg_path,
+        move_package::BuildConfig {
+            test_mode: true,
+            install_dir: Some(tempdir().unwrap().path().to_path_buf()),
+            compiler_config: CompilerConfig {
+                compiler_version: if v2 {
+                    Some(CompilerVersion::latest())
+                } else {
+                    None
+                },
+                ..Default::default()
+            },
+            ..Default::default()
+        },
+        UnitTestingConfig::default(),
+        natives,
+        ChangeSet::new(),
+        /* gas_limit */ Some(100_000),
+        /* cost_table */ None,
+        /* compute_coverage */ false,
+        &mut std::io::stdout(),
+    )
+    .unwrap();
+    if result != UnitTestResult::Success {
+        panic!("aborting because of Move unit test failures");
+    }
+}
+
+#[test]
+fn one_bytecode_dep() {
+    // TODO: automatically discovers all Move packages under a package directory and runs unit tests for them
+    run_tests_for_pkg("tests/packages/one-bytecode-dep", true);
+    run_tests_for_pkg("tests/packages/one-bytecode-dep", false);
+}