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

Cli tool tests #977

Merged
merged 14 commits into from
Jun 14, 2023
Merged

Cli tool tests #977

merged 14 commits into from
Jun 14, 2023

Conversation

tadeohepperle
Copy link
Contributor

partly fixes #946

I added a Writer (&mut impl std::io::Write) to each cli command such that we can write to it instead of stdout.
This makes it easy to test the expected output of each command.

Added tests for some commands.

@tadeohepperle tadeohepperle requested a review from a team as a code owner May 25, 2023 07:39
@tadeohepperle tadeohepperle reopened this May 25, 2023
@tadeohepperle tadeohepperle self-assigned this May 25, 2023
@tadeohepperle tadeohepperle marked this pull request as draft May 25, 2023 07:42
@tadeohepperle tadeohepperle force-pushed the tadeo-hepperle-cli-tool-tests branch from 8dc3421 to df00dd5 Compare June 1, 2023 12:39
@tadeohepperle tadeohepperle marked this pull request as ready for review June 12, 2023 08:49
cli/src/utils.rs Outdated
Comment on lines 98 to 106
/// Given a type definition, return type ID and registry representing it.
#[allow(dead_code)]
pub fn make_type<T: scale_info::TypeInfo + 'static>() -> (u32, scale_info::PortableRegistry) {
let m = scale_info::MetaType::new::<T>();
let mut types = scale_info::Registry::new();
let id = types.register_type(&m);
let portable_registry: scale_info::PortableRegistry = types.into();
(id.id, portable_registry)
}
Copy link
Collaborator

@jsdw jsdw Jun 13, 2023

Choose a reason for hiding this comment

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

move this to the test mod, or mark as #[cfg(test)] if it's used by other tests, to avoid the #[allow(dead_code)] (and make clear it's only used in testing)

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks good; I'll always suggest preferring unit tests over e2e style ones whenever possible, but good to have the option to test the output too for when it's harder to do that (or there's something we want to ensure we are outputting) :)

}

#[tokio::test]
async fn test_commands() {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to split this test into smaller ones because it's much harder to debug if something breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will split it up in another PR today.

let (foo_type_id, foo_registry) = make_type::<Foo>();
let description = print_type_description(&foo_type_id, &foo_registry).unwrap();
let mut output = String::new();
writeln!(output, "struct Foo {{").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

may this works as well:

let s = r#"struct Foo {
        hello: String,
        num: i32
}"#;

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this 💪

@tadeohepperle tadeohepperle merged commit 3cfe91e into master Jun 14, 2023
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-cli-tool-tests branch June 14, 2023 08:52
@jsdw jsdw mentioned this pull request Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: Check the CLI produces the expected output
3 participants