Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Move] Allow Bytecode Dependencies for Unit Testing #15252

Merged
merged 11 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions Cargo.lock

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

10 changes: 7 additions & 3 deletions aptos-move/framework/src/built_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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(),
}),
)
Expand Down
21 changes: 19 additions & 2 deletions third_party/move/move-compiler/src/unit_test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)]
Expand Down Expand Up @@ -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()
Expand All @@ -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 {
Expand Down
9 changes: 7 additions & 2 deletions third_party/move/tools/move-cli/src/base/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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![],
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand Down Expand Up @@ -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()?,
Expand Down Expand Up @@ -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)
Expand All @@ -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()))
}
4 changes: 4 additions & 0 deletions third_party/move/tools/move-unit-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion third_party/move/tools/move-unit-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 14 additions & 8 deletions third_party/move/tools/move-unit-test/src/test_reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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?
Expand Down
9 changes: 7 additions & 2 deletions third_party/move/tools/move-unit-test/src/test_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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)?;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[package]
name = "Dep"
version = "1.0.0"
authors = []
Original file line number Diff line number Diff line change
@@ -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: []
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -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" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not to make third_party to depend on aptos-move. Note that you don't know whether framework/move-stdlib has been compiled at this point or not because there may or may not be an earlier build of framework/move_stdlib or not. Can you create a smaller local test library to third_party to use here?

Copy link
Contributor

Choose a reason for hiding this comment

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

For a simple test like this that doesn't depend on any real Move logic, I think we can manage to not use the stdlib at all.


[dev-dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module 0x42::test {
#[test_only]
use 0x42::foo;

#[test]
fun test() {
assert!(foo::foo() == 42, 0);
}
}
64 changes: 64 additions & 0 deletions third_party/move/tools/move-unit-test/tests/pkg_tests.rs
Original file line number Diff line number Diff line change
@@ -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);
}
Loading