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.

Sign up for GitHub

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

Jump to bottom

Add sui-move CLI #444

Merged
merged 1 commit into from
Feb 12, 2022
Merged

Add sui-move CLI #444

merged 1 commit into from
Feb 12, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 12, 2022

Add sui-move CLI that allows a Move application developer to build and test their code.
I could also merge this into sui CLI.
Open to feedback / suggestions.

patrickkuo
patrickkuo approved these changes Feb 12, 2022
View reviewed changes
Copy link
Contributor

@patrickkuo patrickkuo left a comment

Choose a reason for hiding this comment

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

Some comments on unwraps, otherwise LGTM.

I think we need a UX expert to tell us which is better, 1 binary vs many binaries

sui/src/sui-move.rs Outdated
}

#[tokio::main]
async fn main() -> Result<(), anyhow::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: async not needed here?

sui_programmability/framework/src/lib.rs
@@ -36,7 +35,7 @@ pub enum EventType {

pub fn get_sui_framework_modules() -> Vec<CompiledModule> {
let modules = build_framework(".");
verify_modules(&modules);
verify_modules(&modules).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

will this panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would. This logic doesn't change (it would panic before). Since it's part of loading genesis, it should be ok. (we could certainly consider improve it)

sui_programmability/framework/src/lib.rs
@@ -50,7 +49,7 @@ pub fn get_move_stdlib_modules() -> Vec<CompiledModule> {
.into_iter()
.filter(|m| !denylist.contains(&m.self_id().name().to_owned()))
.collect();
verify_modules(&modules);
verify_modules(&modules).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@oxade
Copy link
Contributor

oxade commented Feb 12, 2022

I'm a fan of one CLI but it might make sense to keep this separate since for rapid dev and migrate stable features to the main stable CLI as needed.

@lxfind lxfind force-pushed the unit-test-framework branch from a4adf92 to e6900f1 Compare February 12, 2022 07:16
@lxfind lxfind changed the base branch from add-tictactoe to main February 12, 2022 07:18
@lxfind lxfind force-pushed the unit-test-framework branch from e6900f1 to f45e5b2 Compare February 12, 2022 07:20
@sblackshear
Copy link
Collaborator

On one CLI vs multiple: I think because the main Sui CLI is an interactive shell, it probably makes sense to have a separate CLI for Move dev (which doesn't require the context/state that the interactive shell buys you). However, I think we should make most (or all) of the Move CLI commands usable from the main Sui CLI as well so it can be a one stop shop. This is easy to do in Rust with structopt subcommands + flatten.

I often look at dapptools for inspiration on how to design these separate CLI's that interact with each other. They seem to have a pretty clean division of local dev, network interaction, and signing CLI's https://github.com/dapphub/dapptools based on a lot of experience with Eth. I'm sure there will be some differences because of Move vs EVM and Sui vs Eth, but still useful.

sblackshear
sblackshear approved these changes Feb 12, 2022
View reviewed changes
sui_programmability/framework/src/lib.rs
.map_err(|err| SuiError::MoveUnitTestFailure {
error: err.to_string(),
})?;
if result == UnitTestResult::Failure {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused about when we will take this branch vs returning a result via the error + ? at 190--do you have a good understanding of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that it's similar to how we use Result. If the test didn't even run due to issues like invalid test graph/dependency, it fails with Err. Otherwise if it's test failure it's Ok(Fail).

@lxfind lxfind merged commit 6b690b8 into main Feb 12, 2022
@lxfind lxfind deleted the unit-test-framework branch February 12, 2022 21:56
@lxfind lxfind linked an issue Feb 15, 2022 that may be closed by this pull request
Design a testing framework for Move code #408
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickkuo patrickkuo patrickkuo approved these changes

@sblackshear sblackshear sblackshear approved these changes

@huitseeker huitseeker Awaiting requested review from huitseeker

@awelc awelc Awaiting requested review from awelc

@gdanezis gdanezis Awaiting requested review from gdanezis

@stella3d stella3d Awaiting requested review from stella3d

@oxade oxade Awaiting requested review from oxade

@arun-koshy arun-koshy Awaiting requested review from arun-koshy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Design a testing framework for Move code
4 participants
@lxfind @oxade @sblackshear @patrickkuo