`unreachable_from_main` false positives w/ reflective tests

[pq]

https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8784818267426989249/+/u/analyze_pkg_/stdout

info - analysis_server/test/services/snippets/snippet_request_test.dart:22:3 - Unreachable member 'SnippetRequestTest' in an executable library. Try referencing the member or removing it. - unreachable_from_main

/cc @srawlins

[srawlins]

Yeah I'll // ignore: those.

[pq]

Alternatively, maybe the lint should know about reflective tests? Or maybe the constructor code code be moved to setUp methods?

[srawlins]

I don't think it would be worth it to make the linter rule aware of test_reflective_loader. It is not a popular package (5 packages on pub use it), and I imagine we will replace the mirrors implementation with a macros one, which wouldn't fool unreachable_from_main.

[srawlins]

I am curious about why the constructor is used instead of setUp. Maybe it is accidental.

[pq]

It is not a popular package (5 packages on pub use it)

Good point. (We do use it pretty heavily internally though.) That said, I'm guessing we don't have many (more) constructors in reflective test classes?

I imagine we will replace the mirrors implementation with a macros one

It's a good point (@scheglov?). That'd be quite a while though and it'd be nice to not have a bunch of ignores pile up in the meantime.

Personally I'd be inclined to add a special-case for reflective tests since they really are false-positives. Alternatively, I'd be curious to get more feedback from @bwilkerson on https://dart-review.googlesource.com/c/sdk/+/293006. If there's a more general way to mark apparently unused code as used, maybe we could do the same for reflective tests?

[bwilkerson]

I have no problem converting the constructor into an override of setUp.

I have no opinion about the migrator cases because I'm not familiar enough with the code to know what those classes are for,

[scheglov]

Yes, I think we will be able to use macros to generate code to run test_X methods in a class. After that the lint will able to see that the class is instantiated with a specific constructor.

We check for annotations presence in lints all the time, it is not something unique. So, why not do it here too, and mark the default constructor of such classes as used?

In a few tests a factory constructor is used to avoid making fields a nullable or late.

[srawlins]

We check for annotations presence in lints all the time, it is not something unique. So, why not do it here too, and mark the default constructor of such classes as used?

It seemed more work than writing three // ignore: comments. There is not a high value in implementing such a special case. But we can do it.

[bwilkerson]

Seems to me that initializing test state in setUp is a better style that initializing it in the constructor, so I also don't see a lot of value in allowing reflective tests to have a constructor. Am I missing something?

[srawlins]

The factory constructors in nnbd_migration, seen in this CL: https://dart-review.googlesource.com/c/sdk/+/293006 are a much more nuanced case of initialization, where the factory constructors do some pre-processing, which allows some fields to be final and non-nullable, and not late. I think refactoring the logic back to setUp would require making the fields late. Which is unfortunate but not a huge expense. 🤷

[bwilkerson]

I'd leave the ignores in the nnbd_migration package. It isn't worth the effort IMO.

[scheglov]

Honest question - why is is a better style for use setUp()? I don't remember how it is done in Java, but for Dart reflective tests we create a new instance of the test class, and call setUp(). So they are mostly equivalent. A subtle difference is that for setUp() the subclass can choose when to call super.setUp(), e.g. after setting some configuration. Is this what you refer to, or something else? I'd like to know.

A wider question, is whether we will eventually report unused public methods in executable files. After all, what is a difference between a function that creates an instance of a class, and a method that does something with a class? If we do, then a solution to use setUp() will not help, we will report the hint of these methods and all test_X methods, and will have to make the lint to understand test_reflective_loader.