Remove test wall time from xml_test_generator usage

[werkt]

The duration report introduces unnecessary noise in determinism for build which otherwise represent identical behaviors - each test that varies in execution enough to cross a second threshold would appear with a different action, making it extremely difficult to isolate determinism across a build's worth of actions.

Duration reporting takes place in bazel through alternative means.

[bharadwaj08-one]

@werkt Could you please take a look at the failing checks?

[fmeum]

This is very useful for interop with test infrastructure that isn't Bazel-aware. JUnit XML is pretty much universally supported, other means for getting this information would need an adapter.

Generally speaking, wouldn't the test logs, which are also inputs to this action, usually be non-deterministic as well? In that case the XML generation spawn would differ anyway.

[werkt]

The test outputs can be deterministic, or not. The problem here is that regardless of whether they are, if the wall time for the test crosses a second threshold, it automatically changes the command definition from one value to the next, and prevents even deterministically outputting tests from avoiding differences between no-change builds being run to locate determinism issues elsewhere. Objectively, the test isn't in control of whether it gets scheduled enough to avoid modifying its own wall time, no matter how much an author seeks deterministic, reproducible output.
I'm game for marking this up as a test-command option, but figured it may be more useful to avoid this very subjective data altogether than try to cobble conditional xml output.

[fmeum]

While I see how it can be annoying that this introduces non-determinism into an otherwise deterministic test, I've honestly never seen a non-trivial project in which every test emits a deterministic log. In the context of detecting non-determinism, getting rid of the runtime argument would only really make a qualitative difference if that was the case though.

bb explain has special handling for this particular spawn, which is recognized by its TestRunner mnemonic and the basename of the output. What do you think of introducing a distinctive mnemonic (say TestXmlGeneration)? That would make it very easy to filter this out without taking away value from non-Bazel-aware test integrations.

[werkt]

The mnemonic should be switched regardless of the outcome here. Qualifying this activity with the TestRunner is ambiguous and something I've wanted changed for a while, I will submit a separate change.

It's not satisfactory though to say that unlikeliness of universal determinism is a reason to permit this behavior to introduce arbitrary nondeterminism. Cases where overt remote cache misses occur here solely because of this change is the domain of this improvement. Less convincing still is the interest to maintain external tooling. I don't have the option to disable this step of output entirely from bazel's execution, and I don't necessarily know that removing this timing will break any particular downstream parser to ignore the missing content (and its obvious duplication for the use in two fields).
Real timing information is clearly available in bazel-specific contexts, because it exists at higher precision than 1s.

[fmeum]

cc @meisterT @tjgq

[werkt]

One better than this with further thought. This is a mechanism that, outside of java, no other user of bazel can perform (starlark for instance): modifying the composite state of an action in its command args such that they are only effective and known during the execution phase. I don't care about the presence in the xml, it can be zero for all that matters here, but the unpredictable nature of the action makes the graph mutable, something against the stated intention of bazel. This represents bazel mutating its own "immutable build graph".

[dzbarsky]

This is a breaking change to multiple repos I'm aware of. Can we at least put it behind an incompatible flag?

(+1 that in my experience most tests do not have deterministic logging, it's common for logs to contain randomly-generated IDs, timestamps, etc.)

[fmeum]

Cases where overt remote cache misses occur here solely because of this change is the domain of this improvement.

Which situation are you referring to here? If the test execution itself is a cache hit, the subsequent XML generation should be as well since the test runtime is obtained from the cached action result. If a test is forced to rerun somehow then this action could potentially be a cache hit if it didn't contain the time, but that seems to be a pretty rare case.

Note that rules can always avoid running this spawn by generating the XML directly as part of the test action. But that's of course not something users can add without patching or forking rules they don't own.

modifying the composite state of an action in its command args such that they are only effective and known during the execution phase

Starlark can't access action timing data, but aren't tree artifacts and the newly exposed action templates examples of this?