frondeus / test-case

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

Bad custom test names should not fail silently #72

Open GabrielSimonetto opened 3 years ago

GabrielSimonetto commented 3 years ago

I will be borrowing the example from #19, since it has some relation to how I found this error on my own code.

So, we know from that issue that the minus will be abstracted from the automatic name creation process, BUT, we have a mechanism for custom names, and that could help the developer to understand that the problem with his code is that he is lacking a valid test name:

#[test_case(15, 15; "Abs_15_15")]
#[test_case(-15, 15; "Abs_-15_-15")]
fn test_crazy_stuff(value: i32, expected: i32) {
    assert_eq!(value.abs(), expected)
}

And this will raise a familiar error, but the interesting part is that it tried to use my custom name, since it begins with abs, and not test_crazy_stuff:

error[E0428]: the name `abs_15_15` is defined multiple times
   --> src/lib.rs:162:5
    |
162 |     #[test_case(15, 15; "Abs_15_15")]
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |     |
    |     `abs_15_15` redefined here
    |     previous definition of the value `abs_15_15` here

This leads me to believe that we have a process that tries to make the name valid, since it erased the hyphen from my custom test, applied, and then we had a name collision.

I haven't tried to analyze the code to understand how the hyphen is erased in both cases, but, i believe that if a developer instantiates a custom name, it shouldn't be safeguarded, if the name is invalid it should break catastrophically warning the developer of what the problem is.

frondeus commented 3 years ago

That's an exciting idea. However, the intention was to provide a human-readable test description, not necessarily a compiler-readable one. For example:

    #[test_case(4,  2  ; "when operands are swapped")]
    #[test_case(-2, -4 ; "when both operands are negative")]
    #[test_case(2,  4  ; "when both operands are positive")]

As you can see, in this example, we have to sanitize input.

Anyway, you have a valid point. @luke-biel - perhaps we should introduce some kind of feature flag or syntax (like different quotes used) to enable "strict mode"?

GabrielSimonetto commented 3 years ago

the intention was to provide a human-readable test description, not necessarily a compiler-readable one.

Hmm, yes, that makes sense

luke-biel commented 3 years ago

perhaps we should introduce some kind of feature flag or syntax (like different quotes used) to enable "strict mode"? @frondeus I'd see extra arg to test case rather than changing syntax of the description.

#[test_case(4, 2 ; "when operands are swapped", fn_name = my_very_custom_name)]

I'm just not sure if this doesn't introduce bloat within test-case - how many people are gonna use it?

AugustoFKL commented 9 months ago

IMHO, a project using test_case should almost always add a name to the test, for good practices. But I think refining the opposite is still something good.

The solution provided on #19 would solve this problem, but the question is: what defined a "bad test name"? Anything besides alphanumeric, spaces, and underscores?