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

dev(sozo): improve test coverage and other few changes #1462

Merged
merged 13 commits into from
Jan 23, 2024
1 change: 0 additions & 1 deletion crates/sozo/src/commands/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ impl AuthArgs {
let env_metadata = if config.manifest_path().exists() {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

// TODO: Check the updated scarb way to read profile specific values
dojo_metadata_from_workspace(&ws).and_then(|inner| inner.env().cloned())
} else {
None
Expand Down
18 changes: 18 additions & 0 deletions crates/sozo/src/commands/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,21 @@ impl BuildArgs {
Ok(())
}
}

#[cfg(test)]
mod tests {
use dojo_test_utils::compiler::build_test_config;

use super::BuildArgs;

// this adds considerable time to test suite, should this be enabled by default?
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be done as part of the dojo-test-utils build.rs already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

build.rs has it but it doesn't generate bindings. since build is done this shouldn't be expensive to do assuming cairo compiler does incremental builds

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the plugin system is something that is done post-compilation, what's generated by build.rs from dojo-test-utils is enough to run tests. But this is done inside the dojo-bindgen crate.

Inside sozo, it's currently not possible to decouple the build from the plugin system being invoked after compilation. For this reason, if the time to execute the test suite must remain reasonable, we can remove this test for now.

However, when the dry-run option will be available, we could imagine sozo outputting the migration strategy + the plugins that are expected to run. Like this, sozo tests cover the plugin integration, but not the actual bindings generation (which is expected to be tested in dojo-bindgen).

Sounds reasonable to you guys?

#[ignore]
#[test]
fn build_example() {
let config = build_test_config("../../examples/spawn-and-move/Scarb.toml").unwrap();

let build_args = BuildArgs { typescript: false, unity: false };
let result = build_args.run(&config);
assert!(result.is_ok());
}
}
10 changes: 9 additions & 1 deletion crates/sozo/src/commands/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ impl EventsArgs {
let env_metadata = if config.manifest_path().exists() {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

// TODO: Check the updated scarb way to read profile specific values
dojo_metadata_from_workspace(&ws).and_then(|inner| inner.env().cloned())
} else {
None
Expand Down Expand Up @@ -132,4 +131,13 @@ mod test {
assert!(arg.to_block.is_none());
assert!(arg.chunk_size == 1);
}

#[test]
fn extract_events_work_as_expected() {
let manifest = Manifest::load_from_path("./tests/test_data/manifest.json").unwrap();
let result = extract_events(&manifest);

// we are just collection all events from manifest file so just verifying count should work
assert!(result.len() == 13);
}
}
1 change: 0 additions & 1 deletion crates/sozo/src/commands/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ impl ExecuteArgs {
let env_metadata = if config.manifest_path().exists() {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

// TODO: Check the updated scarb way to read profile specific values
dojo_metadata_from_workspace(&ws).and_then(|inner| inner.env().cloned())
} else {
None
Expand Down
3 changes: 1 addition & 2 deletions crates/sozo/src/commands/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::ops::migration;
pub struct MigrateArgs {
#[arg(short, long)]
#[arg(help = "Perform a dry run and outputs the plan to be executed.")]
// TODO: i think dry_run would be more descriptive
lambda-0x marked this conversation as resolved.
Show resolved Hide resolved
pub plan: bool,

#[arg(long)]
Expand Down Expand Up @@ -56,8 +57,6 @@ impl MigrateArgs {
)?;
}

// TODO: Check the updated scarb way to read profile specific values

ws.config().tokio_handle().block_on(migration::execute(&ws, self, target_dir))?;

Ok(())
Expand Down
1 change: 0 additions & 1 deletion crates/sozo/src/commands/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ impl ModelArgs {
let env_metadata = if config.manifest_path().exists() {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

// TODO: Check the updated scarb way to read profile specific values
dojo_metadata_from_workspace(&ws).and_then(|inner| inner.env().cloned())
} else {
None
Expand Down
33 changes: 23 additions & 10 deletions crates/sozo/src/commands/options/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub struct AccountOptions {
#[arg(value_name = "PATH")]
#[arg(help_heading = "Signer options - KEYSTORE")]
#[arg(help = "Use the keystore in the given folder or file.")]
// TODO: Should we add `DOJO_KEYSTORE_PATH_ENV_VAR`?
lambda-0x marked this conversation as resolved.
Show resolved Hide resolved
pub keystore_path: Option<String>,

#[arg(long = "password", env = DOJO_KEYSTORE_PASSWORD_ENV_VAR)]
Expand Down Expand Up @@ -160,11 +161,9 @@ mod tests {

#[test]
fn account_address_from_args() {
let env_metadata = dojo_world::metadata::Environment::default();

let cmd = Command::parse_from(["sozo", "--account-address", "0x0"]);
assert_eq!(
cmd.account.account_address(Some(&env_metadata)).unwrap(),
cmd.account.account_address(None).unwrap(),
FieldElement::from_hex_be("0x0").unwrap()
);
}
Expand Down Expand Up @@ -199,20 +198,17 @@ mod tests {

#[test]
fn account_address_from_neither() {
let env_metadata = dojo_world::metadata::Environment::default();

let cmd = Command::parse_from([""]);
assert!(cmd.account.account_address(Some(&env_metadata)).is_err());
assert!(cmd.account.account_address(None).is_err());
}

#[tokio::test]
async fn private_key_from_args() {
let env_metadata = dojo_world::metadata::Environment::default();
let private_key = "0x1";

let cmd =
Command::parse_from(["sozo", "--account-address", "0x0", "--private-key", private_key]);
let result_wallet = cmd.account.signer(Some(&env_metadata)).unwrap();
let result_wallet = cmd.account.signer(None).unwrap();
let expected_wallet = LocalWallet::from_signing_key(SigningKey::from_secret_scalar(
FieldElement::from_str(private_key).unwrap(),
));
Expand Down Expand Up @@ -246,7 +242,6 @@ mod tests {
let keystore_path = "./tests/test_data/keystore/test.json";
let keystore_password = "dojoftw";
let private_key = "0x1";
let env_metadata = dojo_world::metadata::Environment::default();

let cmd = Command::parse_from([
"sozo",
Expand All @@ -255,7 +250,7 @@ mod tests {
"--password",
keystore_password,
]);
let result_wallet = cmd.account.signer(Some(&env_metadata)).unwrap();
let result_wallet = cmd.account.signer(None).unwrap();
let expected_wallet = LocalWallet::from_signing_key(SigningKey::from_secret_scalar(
FieldElement::from_str(private_key).unwrap(),
));
Expand Down Expand Up @@ -373,4 +368,22 @@ mod tests {
// 0x2 is the Calldata len.
assert!(*result.get(3).unwrap() == FieldElement::from_hex_be("0x2").unwrap());
}

#[test]
fn keystore_path_without_keystore_password() {
let keystore_path = "./tests/test_data/keystore/test.json";

let cmd = Command::parse_from(["sozo", "--keystore", keystore_path]);
let result = cmd.account.signer(None);

assert!(result.is_err());
}

#[test]
fn signer_without_pk_or_keystore() {
let cmd = Command::parse_from(["sozo"]);
let result = cmd.account.signer(None);

assert!(result.is_err());
}
}
1 change: 1 addition & 0 deletions crates/sozo/src/commands/options/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ const STARKNET_RPC_URL_ENV_VAR: &str = "STARKNET_RPC_URL";
const DOJO_PRIVATE_KEY_ENV_VAR: &str = "DOJO_PRIVATE_KEY";
const DOJO_KEYSTORE_PASSWORD_ENV_VAR: &str = "DOJO_KEYSTORE_PASSWORD";
const DOJO_ACCOUNT_ADDRESS_ENV_VAR: &str = "DOJO_ACCOUNT_ADDRESS";
const DOJO_WORLD_ADDRESS_ENV_VAR: &str = "DOJO_WORLD_ADDRESS";
76 changes: 69 additions & 7 deletions crates/sozo/src/commands/options/world.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,90 @@ use clap::Args;
use dojo_world::metadata::Environment;
use starknet::core::types::FieldElement;

use super::DOJO_WORLD_ADDRESS_ENV_VAR;

#[derive(Debug, Args)]
#[command(next_help_heading = "World options")]
pub struct WorldOptions {
#[arg(long = "world")]
#[arg(help = "The address of the World contract.")]
#[arg(long = "world", env = DOJO_WORLD_ADDRESS_ENV_VAR)]
pub world_address: Option<FieldElement>,
}

impl WorldOptions {
pub fn address(&self, env_metadata: Option<&Environment>) -> Result<FieldElement> {
if let Some(world_address) = self.world_address {
Ok(world_address)
} else if let Some(world_address) = env_metadata
.and_then(|env| env.world_address())
.or(std::env::var("DOJO_WORLD_ADDRESS").ok().as_deref())
{
} else if let Some(world_address) = env_metadata.and_then(|env| env.world_address()) {
Ok(FieldElement::from_str(world_address)?)
} else {
Err(anyhow!(
"Could not find World address. Please specify it with --world or in the world \
config."
"Could not find World address. Please specify it with --world, environment \
variable or in the world config."
))
}
}
}

#[cfg(test)]
mod tests {

use clap::Parser;
use starknet_crypto::FieldElement;

use super::{WorldOptions, DOJO_WORLD_ADDRESS_ENV_VAR};

#[derive(clap::Parser, Debug)]
struct Command {
#[clap(flatten)]
pub inner: WorldOptions,
}
#[test]

fn world_address_read_from_env_variable() {
std::env::set_var(DOJO_WORLD_ADDRESS_ENV_VAR, "0x0");

let cmd = Command::parse_from([""]);
assert_eq!(cmd.inner.world_address, Some(FieldElement::from_hex_be("0x0").unwrap()));
}

#[test]
fn world_address_from_args() {
let cmd = Command::parse_from(["sozo", "--world", "0x0"]);
assert_eq!(cmd.inner.address(None).unwrap(), FieldElement::from_hex_be("0x0").unwrap());
}

#[test]
fn world_address_from_env_metadata() {
let env_metadata = dojo_world::metadata::Environment {
world_address: Some("0x0".to_owned()),
..Default::default()
};

let cmd = Command::parse_from([""]);
assert_eq!(
cmd.inner.address(Some(&env_metadata)).unwrap(),
FieldElement::from_hex_be("0x0").unwrap()
);
}

#[test]
fn world_address_from_both() {
let env_metadata = dojo_world::metadata::Environment {
world_address: Some("0x0".to_owned()),
..Default::default()
};

let cmd = Command::parse_from(["sozo", "--world", "0x1"]);
assert_eq!(
cmd.inner.address(Some(&env_metadata)).unwrap(),
FieldElement::from_hex_be("0x1").unwrap()
);
}

#[test]
fn world_address_from_neither() {
let cmd = Command::parse_from([""]);
assert!(cmd.inner.address(None).is_err());
}
}
1 change: 0 additions & 1 deletion crates/sozo/src/commands/register.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ impl RegisterArgs {
let env_metadata = if config.manifest_path().exists() {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

// TODO: Check the updated scarb way to read profile specific values
dojo_metadata_from_workspace(&ws).and_then(|inner| inner.env().cloned())
} else {
None
Expand Down
Loading
Loading