Ban public top-level members in a test

[srawlins]

Proposal

Given a library with URI with pattern /test/.*_test.dart, report any public top-level members except main. "Top-level members in a test should be private (or relocated to within main), to prevent unused members."

Secondary proposal

Given a library which does not live in it's declaring package's public API (i.e. files in bin/, test/, and tool/), report any public top-level members except main.

Justification

This lint helps prevent code bloat. Without full-program analysis, we cannot tell if a public member is used anywhere, so it is not reported by the analyzer as unused_variable or something similar. For top-level members, only private ones can be rigorously checked for usage. But in tests (and bin/ and tool/ scripts), which are typically their own entry points and never imported by another library, we want to be able to report unused top-level members.

This includes top-level variables, constants, functions, typedefs, enums, mixins, and classes.

This is a counter-request to #57580. I think this is better choice for a few reasons. #57580 proposes either a change to the analyzer's unused analysis to take into account the library URI ("does it end with _test.dart?"), leading to possible false positives. I don't think that's going to go well. The alternative is to implement a new unused analysis in the linter. Unused analysis is expensive though, and adding a second one in linter (duplicating a ton of code) will make it more expensive.

Downsides

One downside to recommending private top-level members is that it might be counter to what users typically do today. I think a lot of people just write regular, public, top-level consts and helper functions. Personally, I always write private top-level members, so that I know when something becomes unused. Perhaps a study should be done on what most people do. #57580 does not have this downside.

[kevmoo]

Like #57724, right?

[natebosch]

I think it would be mostly covered by #57724 but it's still worth tracking separately.

• Implementing this is likely a lot easier since it isn't blocked by the linter having an understanding of "package level" analysis.
• Even with the package level analysis, defining a public member in a *_test.dart file is something we'd discourage. You don't want to import a test file from another to use a shared utility. That kind of stuff should go in it's own library (though this does mean that the "secondary proposal" above is not feasible, since we explicitly do want to define utility libraries under test/ for shared testing code).

[kevmoo]

We should also be careful, though. Make sure the lib in question is importing package:test/test.dart etc etc

Most folks use pkg:test – but we shouldn't assume

[natebosch]

Most folks use pkg:test – but we shouldn't assume

The folks who don't shouldn't enable this lint...

[goderbauer]

As a data point: Recently did some clean-up in the framework with the help of this tool: https://pub.dev/packages/dart_code_metrics. We had accumulated quite a bit of dead code in tests: flutter/flutter#104550.

This proposal (or #57580 or #58292) probably could have prevented that build-up of dead code.

[pq]

Thanks for the concrete example @goderbauer!

[a14n]

In dart-archive/linter#3453 I implemented such a lint but not specifically for tests. The lint checks every executable libraries (ie. a library containing a main top level function).

On Flutter codebase there are only few cases (among tens of thousands) that are triggered out of tests. Most of them can easily be refactored to prevent the lint (and make the code structured in a better way)

[a14n]

Any opinion about limiting the application of the lint to tests or to extend it to executable libraries (ie. a library containing a main top level function)?

The current PR (that is ready to land) applies on executable libraries but I can change it if we think it will be a too broadened application.

[a14n]

Any opinion about limiting the application of the lint to tests or to extend it to executable libraries (ie. a library containing a main top level function)?

Nobody?