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

Don't panic in serde_test #2435

Merged
merged 1 commit into from May 4, 2023
Merged

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Apr 29, 2023

Panics lead to reporting errors in tests inside of serde_test internals, returning errors moves the report location to the corresponding assert_tokens expression

Panics lead to reporting errors in tests inside of serde_test internals,
returning errors moves the report location to the corresponding assert_tokens
expression
@oli-obk
Copy link
Member

oli-obk commented May 2, 2023

How do test failures look now compared to before? It could be less obvious that the error is a test failure and not a failure from serde. If the errors are indeed misleading, we could also use #[track_caller] liberally instead

@Mingun
Copy link
Contributor Author

Mingun commented May 3, 2023

Take the first commit of #2437. Without this change the output is:

failures:

---- regression::issue2432::deny_unknown_fields::tuple stdout ----
thread 'regression::issue2432::deny_unknown_fields::tuple' panicked at 'expected Token::F32(345.234) but deserialization wants Token::TupleEnd', D:\Projects\serde\serde_test\src\de.rs:97:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- regression::issue2432::deny_unknown_fields::seq stdout ----
thread 'regression::issue2432::deny_unknown_fields::seq' panicked at 'expected Token::F32(345.234) but deserialization wants Token::SeqEnd', D:\Projects\serde\serde_test\src\de.rs:97:9

---- regression::issue2432::deny_unknown_fields::tuple_struct stdout ----
thread 'regression::issue2432::deny_unknown_fields::tuple_struct' panicked at 'expected Token::F32(345.234) but deserialization wants Token::TupleStructEnd', D:\Projects\serde\serde_test\src\de.rs:97:9

---- regression::issue2432::no_deny_unknown_fields::seq stdout ----
thread 'regression::issue2432::no_deny_unknown_fields::seq' panicked at 'expected Token::F32(345.234) but deserialization wants Token::SeqEnd', D:\Projects\serde\serde_test\src\de.rs:97:9

---- regression::issue2432::no_deny_unknown_fields::tuple stdout ----
thread 'regression::issue2432::no_deny_unknown_fields::tuple' panicked at 'expected Token::F32(345.234) but deserialization wants Token::TupleEnd', D:\Projects\serde\serde_test\src\de.rs:97:9

---- regression::issue2432::no_deny_unknown_fields::tuple_struct stdout ----
thread 'regression::issue2432::no_deny_unknown_fields::tuple_struct' panicked at 'expected Token::F32(345.234) but deserialization wants Token::TupleStructEnd', D:\Projects\serde\serde_test\src\de.rs:97:9


failures:
    regression::issue2432::deny_unknown_fields::seq
    regression::issue2432::deny_unknown_fields::tuple
    regression::issue2432::deny_unknown_fields::tuple_struct
    regression::issue2432::no_deny_unknown_fields::seq
    regression::issue2432::no_deny_unknown_fields::tuple
    regression::issue2432::no_deny_unknown_fields::tuple_struct

test result: FAILED. 4 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

error: test failed, to rerun pass `--test regression`
PS D:\Projects\serde\test_suite>

Not much helpful. All tests points here:

{
let value = visitor.visit_seq(DeserializerSeqVisitor {
de: self,
len: len,
end: end,
})?;
assert_next_token!(self, end);
Ok(value)
}

With this change the output is:

failures:

---- regression::issue2432::deny_unknown_fields::seq stdout ----
thread 'regression::issue2432::deny_unknown_fields::seq' panicked at 'assertion failed: `(left == right)`
  left: `Error { msg: "expected Token::F32(345.234) but deserialization wants Token::SeqEnd" }`,
 right: `"invalid length 2, expected struct Struct with 2 elements"`', test_suite\tests\regression\issue2432.rs:315:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- regression::issue2432::deny_unknown_fields::tuple stdout ----
thread 'regression::issue2432::deny_unknown_fields::tuple' panicked at 'assertion failed: `(left == right)`
  left: `Error { msg: "expected Token::F32(345.234) but deserialization wants Token::TupleEnd" }`,
 right: `"invalid length 2, expected struct Struct with 2 elements"`', test_suite\tests\regression\issue2432.rs:346:9

---- regression::issue2432::deny_unknown_fields::tuple_struct stdout ----
thread 'regression::issue2432::deny_unknown_fields::tuple_struct' panicked at 'assertion failed: `(left == right)`
  left: `Error { msg: "expected Token::F32(345.234) but deserialization wants Token::TupleStructEnd" }`,
 right: `"invalid length 2, expected struct Struct with 2 elements"`', test_suite\tests\regression\issue2432.rs:377:9

---- regression::issue2432::no_deny_unknown_fields::seq stdout ----
thread 'regression::issue2432::no_deny_unknown_fields::seq' panicked at 'tokens failed to deserialize: expected Token::F32(345.234) but deserialization wants Token::SeqEnd', test_suite\tests\regression\issue2432.rs:132:9

---- regression::issue2432::no_deny_unknown_fields::tuple stdout ----
thread 'regression::issue2432::no_deny_unknown_fields::tuple' panicked at 'tokens failed to deserialize: expected Token::F32(345.234) but deserialization wants Token::TupleEnd', test_suite\tests\regression\issue2432.rs:164:9

---- regression::issue2432::no_deny_unknown_fields::tuple_struct stdout ----
thread 'regression::issue2432::no_deny_unknown_fields::tuple_struct' panicked at 'tokens failed to deserialize: expected Token::F32(345.234) but deserialization wants Token::TupleStructEnd', test_suite\tests\regression\issue2432.rs:196:9


failures:
    regression::issue2432::deny_unknown_fields::seq
    regression::issue2432::deny_unknown_fields::tuple
    regression::issue2432::deny_unknown_fields::tuple_struct
    regression::issue2432::no_deny_unknown_fields::seq
    regression::issue2432::no_deny_unknown_fields::tuple
    regression::issue2432::no_deny_unknown_fields::tuple_struct

test result: FAILED. 4 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `--test regression`
PS D:\Projects\serde\test_suite>

Now locations points to the failed test.

@oli-obk
Copy link
Member

oli-obk commented May 4, 2023

Thanks! While we could have gotten the panic locations fixed by sprinkling track_caller everywhere, the fact that it is a very localized change and gives us the assert_eq message, is very useful imo.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay dtolnay merged commit 48193fb into serde-rs:master May 4, 2023
19 checks passed
@Mingun Mingun deleted the hitchhiker-guide branch May 5, 2023 05:14
crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request May 9, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | dependencies | patch | `1.0.160` -> `1.0.162` |

---

### Release Notes

<details>
<summary>serde-rs/serde</summary>

### [`v1.0.162`](https://github.com/serde-rs/serde/releases/tag/1.0.162)

[Compare Source](serde-rs/serde@v1.0.161...1.0.162)

-   Support deserializing flattened adjacently tagged enums from data formats which represent fields as bytes, such as the `csv` crate ([#&#8203;2377](serde-rs/serde#2377), thanks [@&#8203;mfro](https://github.com/mfro))

    ```rust
    #[derive(Deserialize)]
    pub struct Record {
        common: u64,
        #[serde(flatten)]
        kind: Kind,
    }

    #[derive(Deserialize)]
    #[serde(tag = "kind", content = "parameter", rename_all = "lowercase")]
    enum Kind {
        Foo(u64),
        Bar(bool),
    }
    ```

    ```csv
    common,kind,parameter
    1,foo,42
    2,bar,true
    ```

### [`v1.0.161`](https://github.com/serde-rs/serde/releases/tag/v1.0.161)

[Compare Source](serde-rs/serde@v1.0.160...v1.0.161)

-   Improve error messages produced by serde_test on test failure ([#&#8203;2435](serde-rs/serde#2435), thanks [@&#8203;Mingun](https://github.com/Mingun))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS43Mi4xIiwidXBkYXRlZEluVmVyIjoiMzUuNzIuMSIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1891
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants