frondeus / test-case

Rust procedural macro attribute for adding test cases easily
MIT License
614 stars 39 forks source link

Regression from 1.2.1 -> 1.2.2: "return-type but no exected clause" failing to compile #86

Closed gh0st42 closed 2 years ago

gh0st42 commented 2 years ago

Tests that were valid in 1.2.1 fail to compile with 1.2.2

an example can be seen here:

#[test_case("dtn:none" => "dtn:none" ; "when using none endpoint")]
#[test_case("dtn:n1/" => panics "" ; "when using dtn endpoint without double slash")]
#[test_case("dtn:none" => EndpointID::none().to_string() ; "when providing node eid and constructed none")]
fn from_str_tests(input_str: &str) -> String {
  EndpointID::try_from(input_str).unwrap().to_string()
}

The compiler gives the following error:

error: Test function from_str_tests has a return-type but no exected clause in the test-case. This is currently unsupported. See test-case documentation for more details.
   --> src/eid.rs:507:43
    |
507 |     fn from_str_tests(input_str: &str) -> String {
    |                                           ^^^^^^

Also, there seems to be a typo in the error message as exected should probably be expected.

Did the syntax change? The changelog does not document any breaking changes regarding this.

frondeus commented 2 years ago

@luke-biel - It's seems that we missed panics scenario.

@gh0st42 - it should not be a breaking change, its more a oversight in that case, therefore a bug. Sorry for that. Please see #50 for more context.

For now, you skip this check by enabling feature allow_result.

luke-biel commented 2 years ago

Mea culpa. I overlooked this. Fix is on it's way, just needs a review.

luke-biel commented 2 years ago

1.2.3 was just published

bspradling commented 2 years ago

I'm still receiving the same error above on 1.2.3 with the following test:

#[test_case("ISBN:1839485743"; "isbn10")]
#[test_case("ISBN:1839485743123"; "isbn13")]
#[tokio::test]
async fn test_bibliography_key_serde(expected: &str) -> Result<(), Box<dyn Error>> {
    let key: BibliographyKey = serde_json::from_str(expected)?;
    let actual = serde_json::to_string(&key)?;
    assert_eq!(expected, actual);
    Ok(())
}
frondeus commented 2 years ago

@bspradling In that case this is expected behavior.

Your tests were passing silently, so even if there was an Err() the test would pass. For now, v1.2.2 introduced a compile-time error so you can review this test.

Currently test_case does not support implicit Result<(), Box<dyn Error>> handling which was introduced in Edition 2018. To solve this problem you should use test case syntax to expect value explicitly.

In your case:

#[test_case("ISBN:1839485743" => Ok(()) ; "isbn10")]
#[test_case("ISBN:1839485743123" => Ok(()) ; "isbn13")]
#[tokio::test]
async fn test_bibliography_key_serde(expected: &str) -> Result<(), Box<dyn Error>> {
    let key: BibliographyKey = serde_json::from_str(expected)?;
    let actual = serde_json::to_string(&key)?;
    assert_eq!(expected, actual);
    Ok(())
}

This should work.

We know that in long term we need to allow implicit Result<(), dyn Err> but our priority was to first prevent any silently failing cases. I hope you understand it.

bspradling commented 2 years ago

Ah! Good to know about the silent failures!

I initially tried your suggestion to resolve the issue but you run into:

30 | #[test_case("ISBN:1839485743" => Ok(()); "isbn10")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no implementation for `dyn StdError == dyn StdError`
   |
   = help: the trait `PartialEq` is not implemented for `dyn StdError`
   = note: required because of the requirements on the impl of `PartialEq` for `Box<dyn StdError>`
frondeus commented 2 years ago

Allright, I think this one deserves its own issue. Let me create one and then we should continue figuring out the right approach for it :)

frondeus commented 2 years ago

Actually it might be just a edge-case of #50

frondeus commented 2 years ago

@gh0st42 - Is your case solved?

luke-biel commented 2 years ago

Ah! Good to know about the silent failures!

I initially tried your suggestion to resolve the issue but you run into:

30 | #[test_case("ISBN:1839485743" => Ok(()); "isbn10")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no implementation for `dyn StdError == dyn StdError`
   |
   = help: the trait `PartialEq` is not implemented for `dyn StdError`
   = note: required because of the requirements on the impl of `PartialEq` for `Box<dyn StdError>`

I just tested locally that

    #[test_case("abc" => matches Ok(_))]
    fn dyn_err(input: &str) -> Result<(), Box<dyn std::error::Error>> {
        Ok(())
    }

works. It may be a solution in your case untill we introduce proper handling of Result.

gh0st42 commented 2 years ago

@gh0st42 - Is your case solved?

yes, version 1.2.3 works again even without the allow_result feature. thank you for the quick response.