Open chanced opened 1 month ago
Rather than remove, another option is to change them so they are more useful. I get why having high coverage is appealing, it has good marketing value, and open-source projects subsist on popularity. So rather than reduce coverage we can just increase test quality to achieve the same effect. Some working principles to keep in mind to achieve that:
source()
returns a specific value. The Error
trait very deliberately defines the return type as Option<&(dyn Error + 'static)>
, which has the semantics that no promises are made about the source of the error, only that if one exists it also implements the Error
trait, and that although you're borrowing it, you can hold on to it for as long as you'd like. By checking for a specific value we're constraining the implementation beyond what its contract promises.#[test]
fn into_owned() {
let token = Token::from_encoded("foo~0").unwrap().into_owned();
assert_eq!(token.encoded(), "foo~0");
}
While this checks an important property of Token
, it doesn't check the defining effect of calling into_owned
, which is the change in the lifetime. This is also connected to the next point, as it gives us a false sense of security.
let err = InvalidEncodingError { offset: 3 };
assert_eq!(err.offset(), 3);
Setting aside the fact that maybe we shouldn't implement that method since the field is public anyway (and changing that is a breaking change), its implementation is so trivial we're very unlikely to screw it up so badly this test would catch it. A more interesting test would perhaps be a prop-test (using quickcheck
or some other method) - after all what's so special about 3?
Remove useless tests.