assert-rs / snapbox

Snapshot testing for a herd of CLI tests
docs.rs/trycmd
Apache License 2.0
138 stars 18 forks source link

Easier to diff, prettier rendering for jsonlines #348

Closed epage closed 4 months ago

epage commented 4 months ago

cargo-test-supports built-in assertions for jsonlines would perform the following transformation to the "expected" value

    let expected_objs: Vec<_> = expected
        .split("\n\n")
        .map(|expect| {
            expect
                .parse()
                .with_context(|| format!("failed to parse expected JSON object:\n{}", expect))
        })
        .collect::<Result<_>>()?;

so instead of

{}
{}

you'd get

{
}

{
}

This makes it easier to inspect the jsonlines data and to view the diffs.

For Cargo to adopt snapbox, we need something similar, see rust-lang/cargo#14039

epage commented 4 months ago

I think doing exactly what cargo-test-support wouldn't align well with snapbox.

However, what we could do is allow the test-author to separately specify the expected-format from the actual-format and allow them if the internal structure is similar. This would be leveraging the fact that we store jsonlines as a serde_json::Value::Array.

This means the "expected" would be pretty-printed json, rather than a bespoke format. The result is mostly the same, with the main difference being extra indentation and json's inconsistent comma between entries.

One potential API:

assert_data_eq!(actual.jsonlines(), expected.json());

The problem with trait methods on actual is that it doesn't work with Execs::with_stderr_data as the actual is completely internal to Execs.

So I'd probably go with

assert_data_eq!(actual, expected.is_json().against_jsonlines());

Note I renamed IntoData::json to IntoData::is_json to make the naming clearer. While making "breaking" changes (renames w/ deprecations), I'm tempted to make actual a impl Into<Vec<u8>> and rename Data / IntoData to Snapshot / IntoSnapshot. All of the functionality on Data / IntoData is geared to expected and it would be confusing to offer it to actual and this makes the intent of the API clearer at the type level.

weihanglo commented 4 months ago

assert_data_eq!(actual, expected.is_json().against_jsonlines());

This looks good. So does the rename.

(little concerns around rename as people usually dont like it, but I feel like the major consumers of snapbox should be fine with it)

epage commented 4 months ago

I've implemented #350 but it looks like it won't be sufficient for Cargo as it tries to filter out "invalid" jsonlines

    let actual_objs: Vec<_> = actual
        .lines()
        .filter(|line| line.starts_with('{'))
        .map(|line| {
            line.parse()
                .with_context(|| format!("failed to parse JSON object:\n{}", line))
        })
        .collect::<Result<_>>()?;
epage commented 4 months ago

metabuild tests also don't work with how snapbox finds the end of globs.