Check for calls to `test` in `setUp` and `tearDown` callbacks

[hellyhansen]

Describe the rule you'd like to see implemented

Warn when a test is defined inside a setUp or tearDown callback.

Examples

void main() {
  group('Good tests', () {
    setUp(() {
      doSomeSetup();
    });

    test('This is fine, () {
    });
  });
  group('Bad tests', () {
    setUp(() {
      doSomeSetup();

      // It would be nice if the linter flagged this test.
      test('whoops', () {
      });
    });
  });
}

In this example, the whoops test doesn't run because it's been accidentally defined inside the setUp callback. The setup doesn't fail because there are no tests in the group to trigger it. In a larger test it's easy to miss the indentation and assume the tests are passing.

[pq]

This is really interesting. Similar enforcement has come up in the context of flutter where we want to ensure that certain functions aren't called, for example, within build(). The solution we kicked around was a @DoNotCallWithin annotation that allows API designers to disallow function calls in protected contexts. This is an interesting variation -- I guess the inverse. So something like @OnlyCallWithin. Anyway, that's a generalized solution. I wonder if @natebosch has any thoughts about enforcing test API use in particular?

[natebosch]

Another option here could be to report a warning if there is a setUp that won't be run because there are no test in that group. It might get a little complicated but I think would be easier than a lint like this.

cc @grouma @jakemac53

[bwilkerson]

Would we just want to produce a lint if there's a group that doesn't contain at least one group or test, or is there a valid use case for that?

[natebosch]

Would we just want to produce a lint if there's a group that doesn't contain at least one group or test, or is there a valid use case for that?

I can't think of a valid use case for that.

Any lint we write likely can't be 100% foolproof, but that might not be a problem. For instance it might be tougher to support a pattern like:

import 'something.dart';

void main() {
  group('something', somethingTests);
}

However those patterns are rare, and it might be sufficient to only handle cases where the second argument to group is a function literal.

[jakemac53]

I think this is probably something we could just flag in the test runner itself when collecting tests? If we get an empty group fail the run with an error saying you can't do that. I can't think of any valid reason for an empty group.

If we similarly throw if test is called within setUp or tearDown (or any time after we start actually running tests), then I think that covers everything? It seems overly specific to me to have a lint for this.

[spencercornish-wk]

@pq @jakemac53 Have there been any plans on adding this lint, making test throw in those situations, adding an annotation, etc? I've seen this done a couple of times, which resulted in tests in some files not being run for months+, which can hard to catch manually in repos with thousands of tests.

[jakemac53]

It looks like we will already throw today if you call test while a test is running (this includes inside of setUp). But we will never run setUp if there are no tests in the group. So I think it would be sufficient to fail if we see a group with no tests at all.

That is probably pretty easy I can take a look.

[jakemac53]

cc @natebosch how worried would you be about the breaking nature of this? I can test it internally, but curious what your general thoughts are.

[natebosch]

Yeah I think this is worth rolling out even if it's breaking.

[jakemac53]

So an additional thing I realized when looking at this - a naive approach also forces the root group to have tests. This is probably desirable (otherwise a root setUp could violate the rules still), but it means that all test files must have tests now. Are we ok with that change? It seems beneficial generally, maybe a bit annoying when scaffolding out a project.

I could also only throw if there is a setUp or tearDown for the root group, but allow the root group to be completely empty.

[pq]

I'm definitely in favor. This is a common source of confusion.