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

Easier to diff, prettier rendering for jsonlines #348

Closed
epage opened this issue Jul 19, 2024 · 4 comments · Fixed by #350
Closed

Easier to diff, prettier rendering for jsonlines #348

epage opened this issue Jul 19, 2024 · 4 comments · Fixed by #350
Labels
A-snapbox Area: snapbox package enhancement Improve the expected

Comments

@epage
Copy link
Contributor

epage commented Jul 19, 2024

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 epage added enhancement Improve the expected A-snapbox Area: snapbox package labels Jul 19, 2024
@epage
Copy link
Contributor Author

epage commented Jul 19, 2024

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
Copy link

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
Copy link
Contributor Author

epage commented Jul 23, 2024

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
Copy link
Contributor Author

epage commented Jul 23, 2024

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

@epage epage closed this as completed in 273025e Jul 23, 2024
bors added a commit to rust-lang/cargo that referenced this issue Jul 24, 2024
test: Migrate some json tests to snapbox

### What does this PR try to resolve?

This builds on assert-rs/snapbox#348 and is part of #14039.

Note: this also updates existing `.is_jsonlines()` usage to `.is_json().against_jsonlines()`.

### How should we test and review this PR?

### Additional information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-snapbox Area: snapbox package enhancement Improve the expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants