dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
631 stars 170 forks source link

proposal: `avoid_visible_for_testing_annotation` as a new lint rule #3262

Open jorgecoca opened 2 years ago

jorgecoca commented 2 years ago

avoid_visible_for_testing_annotation

As a developer, I'd like to have the option of flagging the usage of the annotation visibleForTesting in projects in which I think its presence is not recommended.

Description

Dart uses the annotation visibleForTesting as a way to tell the analyzer that, while an API was marked as public, it is not intended truly to be public, but instead only be accessible in testing.

While I see the benefits of using this annotation in many different projects, there are also many other cases in which it might lead to poor API design decisions; therefore, given developers the ability to flag its usage might allow us to make better API design choices based on our project needs.

Details

Imagine a class like this:

class MyClass {
 @visibleForTesting
 static const definedSize = 50;

  Square createSquare() => Square(side: definedSize);
}

Someone might write a test like this:

test('createSquare returns a 50x50 square', () {
  final c = MyClass();
  expect(c.createSquare(), Square(side: definedSize));
});

This test would pass without a problem. Furthermore, if we change the value of definedSize inside MyClass, the test would still pass without modifying the tests, leading to a consistency problem.

In an ideal world, definedSize would be private and it would force us to write our expectation like this:

expect(c.createSquare(), Square(side: 50));

But in this case, the usage of @visibleForTesting led us to a poorly implemented API that affects also the robustness of our tests.

If we had a lint rule that allows us to detect its usage in projects where we do not want to use that annotation, it would help our teams to re-think our API design an provide a more robust solution.

Kind

Good Examples

N/A

Bad Examples

N/A

Discussion

This discussion is motivated by a separate chat we've had on our team, in which we realized that we've never had the need for @visibleForTesting in Flutter products, but there has been some undesired attempts to introduce it in a few codebases leading us to poorly implemented features.

Discussion checklist

KyleFin commented 2 years ago

I agree this would be a helpful linting option to give more control and warnings about patterns you've decided to follow or avoid in your codebase.

In your example, I might argue that the inconsistency is caused by the name of the test. Instead of returns a 50x50 square it could be named something like uses default value (since that's the behavior that's actually being asserted).

My biggest concern is that with visibleForTesting there is no way for the compiler to disallow inappropriate usage of the annotated member. "Enforcement" is only done with analyzer warnings. So if you use visibleForTesting, the member is actually public and can be accessed anywhere.

For internal packages this may be a bit sloppy or inconvenient, but if I'm developing a package or library to be consumed externally I would absolutely want a way to guarantee users of my library don't have full access to members I thought were only exposed to tests. (I believe this isn't a problem for Java's visibleForTesting because you can make members package protected.)