Refactor the Reporter API

[natebosch]

The API is currently "backwards". The Reporter class is designed around how the Runner class wants to use it, it doesn't really say anything about what the reporter should do, or how to implement one. The details for what the reporter should do are communicated, although not well at all, through the ReporterFactory typedef. What really needs to happen is for the reporter to listen on various streams from the Engine, but to know the specific ones you really need to just copy existing reporter behavior. This also means that lots of other irrelevant details from Engine leak.

We should consider refactoring to a new pattern. Some options that have been discussed:

• Write an interface in terms of the events that the reporter handles. Very roughly something like:

  class Reporter {
    void onTestStarted(...);
    void onTestSuccess(...);
    void onTestFailure(...);
    void onSuiteDone(...);
    ...
  }

• Remove Engine and create an API in terms of one or more Streams that emit values that might be LiveTest, or perhaps a new and more limited interface.

Current reporters also read other details from the engine, like the list of "active" tests so some consideration will bee needed about how to expose what is necessary and leave room for enhancement without leaking too many unnecessary details.

[jakemac53]

We could also consider just a single Stream of objects, testStateChanges. The objects in the stream would contain all the information for the state change, the Test that changed state, the new state, possibly the old state, etc.

[natebosch]

I spent some time digging on this. One bit that has me worried about making a change here is this snippet:

test/pkgs/test_core/lib/src/runner/reporter/json.dart

Line 257 in 62f7408

'hidden': !_engine.liveTests.contains(liveTest)

This appears to rely on certain tests not showing up in Engine.liveTests which is a combination of passed, skipped, failed, and active.

There is some logic that prevents tests which have a .suite which is a LoadSuite from getting put into active, but that is sometimes violated if there are no other tests in active when a LoadSuite test is added.

test/pkgs/test_core/lib/src/runner/engine.dart

Lines 449 to 450 in 62f7408

	// Only surface the load test if there are no other tests currently running.
	if (_active.isEmpty) _active.add(liveTest);

This is done because of a UX concern and isn't related to the logic of running tests. That mixing of responsibilities makes this tricky. I think it's maybe relying on liveTest.onComplete firing after liveTest.onStateChange to deliver the State.success, this is a stated guarantee but the reliance on it is not visible in the code.

I think that it's only these LoadSuite tests that are ever hidden. I don't know why we mark them as hidden in the 'testDone' and not the 'testStart' events.

[jakemac53]

I think that it's only these LoadSuite tests that are ever hidden. I don't know why we mark them as hidden in the 'testDone' and not the 'testStart' events.

Probably so that the loading message shows when tests start running - so basically just a UX thing... 😞

[jakemac53]

We could still give the json reporter access to the engine if we needed to right? Without actually making that a part of the public contract?

Not sure if I am totally following but that seems like it could help resolve the scenario we are talking about. I don't know how that would work out exactly in the code, the built in reporters might end up being a bit special in that way.

Or, can we communicate this same information through the new apis such that we could mimic the old behavior but without exposing the engine?

[natebosch]

Or, can we communicate this same information through the new apis such that we could mimic the old behavior but without exposing the engine?

That's what I'm attempting to figure out out. If I can be reasonably sure that it's only the LiveTest with a suite which is a LoadSuite I can keep this behavior with a new interface, but if there are other subtle differences it may end up breaking someone.

We could also consider dropping the 'hidden' field and see if anyone notices. 😈
The most prominent use of the json API is the VSCode integration and that doesn't read it: #1321 (comment)

I do need to dig around some more though. I think that setUpAll and tearDownAll are also "synthetic" tests like the load tests, but I'm not sure which touch points in the code deal with that.