Consider reporting skipped tests in the json reporter

[natebosch]

We currently report skipped tests as success for backwards compatibility. I would imagine than consumers of this data can do something smarter if we report skipped tests correctly.

Rolling out a change here might be a little tricky since the breaking change in semver doesn't inform things like IDE plugin compatibility.

cc @DanTup for thoughts. Would this be useful?

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

Lines 253 to 256 in 223665c

	// For backwards-compatibility, report skipped tests as successes.
	'result': liveTest.state.result == Result.skipped
	? 'success'
	: liveTest.state.result.toString(),

[natebosch]

Oh, we do report them as skipped, just in a separate field right below this...

maybe it's not worth changing if the only real benefit is that we can send 1 less field

[DanTup]

Yeah, in Dart-Code we have this:

		if (evt.skipped) {
			testNode.status = TestStatus.Skipped;
		} else if (evt.result === "success") {
			testNode.status = TestStatus.Passed;
		}

If you're looking for things to improve though, those fake "Loading" tests that come through like real tests are kinda annoying :-)

[natebosch]

If you're looking for things to improve though, those fake "Loading" tests that come through like real tests are kinda annoying :-)

We have a hidden field on testDone events. I don't know why we have one there but not on testStart events.

From the docs:

test/pkgs/test/doc/json_reporter.md

Lines 302 to 305 in fbbd3b2

	The `hidden` attribute indicates that the test's result should be hidden and not
	counted towards the total number of tests run for the suite. This is true for
	virtual tests created for loading test suites, `setUpAll()`, and
	`tearDownAll()`. Only successful tests will be hidden.

The implementation doesn't make a lot of sense to me:

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

Line 258 in fbbd3b2

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

Are you currently reading that hidden field? Is it useful for you?

Would it be better to suppress "Loading" tests always, or would it be better to be more precise about what hidden means and add that field to testStart events?

[jakemac53]

In general we do want to report the loading status of tests, etc. That is useful information for tools to get.

The way that works is pretty bizarre currently to be sure though.

[DanTup]

Are you currently reading that hidden field? Is it useful for you?

I don't think so. I might not have seen it (since it was only on the end one). Currently we check testNode.test.name.startsWith("loading ") to try and identify them and hide them.

Would it be better to suppress "Loading" tests always, or would it be better to be more precise about what hidden means and add that field to testStart events?

I think either of these could simplify things a fair bit, though having some indication of loading would definitely be better (so if we can come up with a nice way to show it in the test tree, we could).

The way that works is pretty bizarre currently to be sure though.

Yeah. I assumed that pre-JSON it was probably convenient to emit the tests that way (to show loading status) and then it just didn't translate very nicely to the JSON. I think having some specific event for "progress" events might be better though.