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

Add option for JSON formatted output #324

Merged
merged 20 commits into from
Aug 12, 2021
Merged

Add option for JSON formatted output #324

merged 20 commits into from
Aug 12, 2021

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Aug 6, 2021

Adds a new --output-json flag which will output build results in JSON instead of the
normal human readable format. This is useful for using cargo-contract in part of data
pipelines.

@HCastano HCastano added the enhancement New feature or request label Aug 6, 2021
@HCastano HCastano marked this pull request as ready for review August 9, 2021 21:04
@HCastano HCastano requested a review from cmichi August 9, 2021 21:04
src/cmd/build.rs Outdated

// we specified that debug symbols should be kept
assert!(has_debug_symbols(&res.dest_wasm.unwrap()));

Ok(())
})
}

#[test]
fn build_result_seralization_sanity_check() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh I couldn't come up with good tests to figure out whether or not this flag worked as expected. They basically ended up boiling down to whether or not BuildResult had the right fields.

Open to suggestions though.

CHANGELOG.md Outdated Show resolved Hide resolved
src/cmd/build.rs Outdated Show resolved Hide resolved
let args = crate::cmd::build::ExecuteArgs {
manifest_path,
build_artifact: BuildArtifacts::CodeOnly,
..Default::default()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍!


let res = super::execute(args).expect("build failed");

assert!(res.serialize_json().is_ok());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert!(res.serialize_json().is_ok());
// then
assert!(res.serialize_json().is_ok());

Copy link
Collaborator

@cmichi cmichi Aug 11, 2021

Choose a reason for hiding this comment

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

Please add a

let expected output = /* Insert a multi-line string of the whole fixture */;
assert_eq!(serialized_json, expected_output);

You will have to replace some part of the absolute artifact paths in the serialized json (or use a regex).

Having a fixture for this will enable us to notice if the output format changes and communicate it in the changelog.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add this test back in? It ran the whole execution pipeline with OutputType::Json, which is not tested otherwise.

src/main.rs Outdated Show resolved Hide resolved
@HCastano
Copy link
Contributor Author

I've pulled the CI Docker image locally and all the tests pass. Could the issues be caching related? cc @TriplEight

@HCastano HCastano requested a review from cmichi August 11, 2021 15:53
@TriplEight
Copy link
Contributor

@HCastano if you run everything with cargo test --verbose --workspace --all-features and there's no special logic that would avoid running

failures:
---- cmd::build::tests_ci_only::contract_lib_name_different_from_package_name_must_build stdout ----
--------------------^^^^^^^^^^^^

this, then it might be caching related. You can compare the executed tests by cargo test -- --list

@TriplEight
Copy link
Contributor

Just in case I've purged the caches and relaunched, now it's green, weird.

Copy link
Collaborator

@cmichi cmichi left a comment

Choose a reason for hiding this comment

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

Please add the execute test from 730d14a#r687601916 back in, looks good otherwise!

@HCastano HCastano merged commit 259b716 into master Aug 12, 2021
@HCastano HCastano deleted the hc-json-output branch August 12, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants