Fix bazel coverage false negative

[keith]

Previously this short circuit meant the tests weren't actually run, and
they would always exit 0, in the case the test rule didn't set the lcov
related attributes.

More context: #13978

[keith]

Maybe it would be safer to no-op LCOV_MERGER instead in case any of the test setup below this short circuit is expected even without this attribute

[c-mita]

Maybe it would be safer to no-op LCOV_MERGER instead in case any of the test setup below this short circuit is expected even without this attribute

Part of me agrees; I didn't really like the early exit in the first place and this makes the collect_coverage script more complicated.
The downside is that a "null" LCOV_MERGER command still has to output the parameters and take all the parameters.

I think, for now, this best preserves the "old" behaviour, where effectively no coverage information is collected and none of the coverage outputs are generated (modulo the empty $COVERAGE_OUTPUT_FILE).

[keith]

Missing variables like COVERAGE=1 worries me a bit, since we don't know what folks do with that. I think setting it to true which is what most folks (including ourselves) do here would be a fine no-op.

But looking at the script a bit more, even though you would have missed some variable setup I think in the past this would have just failed when LCOV_MERGER wasn't set, so maybe at least this is a bit better?

[c-mita]

In the past this script wouldn't be run, it then started being run without the full setup. From my perspective, the early exit without full setup in this case isn't too bad for the time being, but this isn't a state of affairs I would be happy with for ever.

[keith]

Sg, should we merge and cherry pick for 5.1?

[brentleyjones]

@bazel-io fork 5.1