Expand "scopeLog" to make testing *way* easier

[matanlurey]

Today I need the following boilerplate (from an AngularDart test):

    test('a class with an invalid field formal parameter', () async {
      final logs = <String>[];
      final logger = new Logger.detached('test')..onRecord.listen((r) {
        print(r);
        logs.add(r.message);
      });
      scopeLog(() {
        reader.parseDependencies(classNamed('BadField'));
      }, logger);
      await new Future.value();
      expect(logs, ['Bad thing happened']);
    });

It would be nice to reduce this, somehow, to:

    test('a class with an invalid field formal parameter', () async {
      final logs = await scopeTestLogger(() {
        reader.parseDependencies(classNamed('BadField'));
      });
      expect(logs, ['Bad thing happened']);
    });

[matanlurey]

As usual, happy to make change myself, just want input from @jakemac53 / @natebosch.

[matanlurey]

Semi-related: dart-lang/core#434.

[natebosch]

I'm on board with it. I think we should consider using LogRecord instead of String to make more complex matching possible.

Maybe

Future<List<LogRecord>> recordLogRecords(Future action())
Future<List<String>> recordLogMessages(Future action())

[matanlurey]

Either of those is fine with me.

I'll give @jakemac53 some time to weigh in, and if we agree I'll work on it later this week.

[jakemac53]

Seems fine to me, +1 for LogRecord. Could also consider just returning the Logger.

[matanlurey]

@jakemac53:

Could also consider just returning the Logger.

Unfortunately Logger isn't very testable, since onRecord drops on the floor (no buffering).

[jakemac53]

Ah sure good point.

We could also consider returning a Stream<LogRecord> but it would have to be a custom one not the default onRecord stream I think since that one probably never closes?

[matanlurey]

I'm OK with that too. But at that point might as well return Future<List<LogRecord>>.

[jakemac53]

In general if I see a Future<List> my first question is are you sure you dont really want a Stream? If the items can be delivered incrementally then a Stream represents that better. If none are available till the end anyways then a Future<List> might make sense.

In this case logs do come through incrementally so I would lean towards Stream. Also the Stream matches are nicer in some ways, specifically if you care about the ordering items you are matching against.

[natebosch]

I do usually think "Stream" when I see Future<List> - and it's pretty easy to get from a Stream to a List by awaiting .toList()...

However I do think 99.9% of cases are going to want the List anyway so I'm torn on whether I want to impose that extra noise of .toList() on every usage...

[matanlurey]

Definitely my thought too. If we provide a Matcher, I'm OK with it:

final logs = recordLogs(() { ... });
await expectLater(logs, containsLogs([
  '...',
  '...',
]);

[jakemac53]

I am curious why you think most people want a List? The primary use case I would think is with matchers.

I don't know all the matchers but I think the StreamMatchers cover some use cases better. Specifically if you want to check that certain logs occur in a specific order but with arbitrary other logs in between.

I am about to lose cell service tho so I won't be able to chime in again for a while, feel free to make a decision either way, both sound fine.

[natebosch]

Specifically if you want to check that certain logs occur in a specific order but with arbitrary other logs in between.

Sounds a lot like containsAllInOrder which operates on Iterable. I know I've seen similar matchers on Stream but I think they might all be private in pub or test. Test does expose emitsInOrder but that doesn't allow extra events in between.

In any case I don't think we need to bikeshed too much on this. Let's aim for emitting a Stream<LogRecord> and adding Matchers for LogRecords and see how it feels in usage, and if whoever implements this thinks it feels clumsy we can try with List<LogRecord> and see if that looks nicer.