feat(try_from_into): Add tests

[fiplox]

Hello everyone 😄. I was trying to resolve the try_from_into exercise and found out that I passed all the tests if I sum up for example all elements in array and test if it is <= 765 or >= 0. But if we sum up (255 + 255 + (-1)) it gives us a good result but the third value is negative, so we should not pass it.

I added 3 tests for each type to prevent users pass this type of errors.

Example for array:

// Array implementation
impl TryFrom<[i16; 3]> for Color {
    type Error = String;
    fn try_from(arr: [i16; 3]) -> Result<Self, Self::Error> {
        if arr[0] + arr[1] + arr[2] <= 765 && arr[0] + arr[1] + arr[2] >= 0 {
            let c = [arr[0] as u8, arr[1] as u8, arr[2] as u8];
            Ok(Color {
                red: c[0],
                green: c[1],
                blue: c[2],
            })
        } else {
            Err("NOP".to_string())
        }
    }
}

This code passes all the tests, but if we add test with [-1, 255, 255] it does not pass 👍.
I'm open for discussion if it is not merge-able to correct ! 😃

[darnuria]

Otherwise I love the idea, because it help to avoid sneaking around the intent of the exercise. When I "cheese" an exercise I always personally feel bad. ;P

[darnuria]

Disclamer: I am not maintainer of Rustlings just a modest suggestion:

Would have done something
assert!(Color::try_from((-1, 255, 255)).is_err())
playground link

I try to avoid unwrap in test because I hate the stack-trace it generate would prefers a expect or if the error is precise even an assert_eq! or a assert!(expr.is_ok) or assert!(expr.is_err()) if it's hard to match value.

[fiplox]

Then we would need to rewrite every test with is_err() because every test uses unwrap().
If there is demand to rewrite every test without unwrap() I'm in!

[AbdouSeck]

Hi y'all, this is a great addition. The desire to move from .unwrap() has been brought up in #549. If you're up for it, please feel free to rewrite the tests by substituting .unwrap() with .is_err() and removing the #should_panic directives.

[AbdouSeck]

Just going to link this PR with issue #549. If the unit tests can be rewritten to replace .unwrap() with .is_err(), then this PR could close the linked issue.

Thanks,
Abdou

[fiplox]

I doubt if I should replace unwrap() in correct tests too

[AbdouSeck]

@fiplox It should be fine to do that, as long as the #[should_panic] directive is removed from the target tests.

Thanks,
Abdou

[fiplox]

@AbdouSeck Would it be ok to replace this test :

#[test]
    fn test_slice_correct() {
        let v = vec![183, 65, 14];
        let c = Color::try_from(&v[..]).unwrap();
        assert_eq!(c.red, 183);
        assert_eq!(c.green, 65);
        assert_eq!(c.blue, 14);
    }

with this :

    #[test]
    fn test_slice_correct() {
        let v = vec![183, 65, 14];
        let c = match Color::try_from(&v[..]) {
            Ok(color) => color,
            Err(_) => Color {
                red: 0,
                green: 0,
                blue: 0,
            },
        };
        assert_eq!(c.red, 183);
        assert_eq!(c.green, 65);
        assert_eq!(c.blue, 14);
    }

and other correct tests too the same way?

[AbdouSeck]

@fiplox That would be fine with me.

[darnuria]

Sorrryyyy i know i look like the bikesheding strange guy who comes up after the discussion settled.

But I think it's better to rethink the way of checking error, beginners look in rustlings tests to find out a solution or to learn idiomatic testing. It's like correction of homework, giving the best of ourself help beginners to tackle Rust difficulties. :D Just IMHO.

[AbdouSeck]

I am not a huge fan of the additional PartialEq derive, but I hope it does not add any confusion to beginners. @fmoko this should be good to go, unless something else comes up.

Thanks,
Abdou

[fiplox]

It should not add any confusion as it is one of the last exercises. 😉

[fiplox]

Did force push to leave only useful information.