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

Remove unnecessary tests #57

Open
chanced opened this issue Jul 9, 2024 · 2 comments
Open

Remove unnecessary tests #57

chanced opened this issue Jul 9, 2024 · 2 comments

Comments

@chanced
Copy link
Owner

chanced commented Jul 9, 2024

Remove useless tests.

@asmello
Copy link
Collaborator

asmello commented Jul 9, 2024

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:

  • We shouldn't enforce aspects of the current implementation that have no impact on users
    • This makes evolution more difficult, as contributors will be afraid to make changes that invalidate tests. And if tests need to be updated all the time, how do we know when they are failing due to an actual regression? An example is checking 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.
  • Tests should be targeted
    • Some tests may seem to test for one aspect of the behavior when they actually test another. A good example:
     #[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.
  • We should strive for tests to look for bugs that have a realistic chance of happening (i.e. tests should be high-signal)
    • This is mostly to avoid gaining a false sense of security. There is an implicit contract of trust that if a test exists (any test) for a piece of logic, and the test passes, we expect the logic to be correct. This makes changes more likely to receive less scrutiny during reviews, which makes regressions more likely to creep in. I don't think there's any super bad example of this currently, but one I can give that maybe falls under the category of "how likely is this to catch any bugs, really?" is:
          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?

@chanced
Copy link
Owner Author

chanced commented Oct 21, 2024

Yea, I really need to clean these up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants