should we force result to false here? if this code line get hit, that probably means we should indicate failure.

alternatively, we could consider propagating absl::status_or even further up (so we return that instead of the boolean we use return right now) if you'd be up for it, but feel free to not do this, I don't intend to bloat this PR.

What shall be the status code for [1]?

Consider setting result to false and fall trough, to have the if(!result) {... clause handle the logging/error handling. Minimizing the control logic helps keeping coverage up. If we want to specialise the message that gets logged we can declare/set that separately.

This seems like a rather large decrease, looking at the coverage report we could set this set this at 93.9 with the current state.

having said that, looking at https://26453-180498819-gh.circle-artifacts.com/0/coverage/source/client/output_formatter_impl.cc.gcov.html, I think it should be possible to hit some of the failure cases for the Fortio output formatter, by creating a test that provides an input that doesn't include a "global" result. As far as I am concerned this can be moved to a new issue though.

Consider s/, if not/or an/ to make line flow a little better.

Consider setting result to false and fall trough, to have the if(!result) {... clause handle the logging/error handling. Minimizing the control logic helps keeping coverage up. If we want to specialise the message that gets logged we can declare/set that separately.

Do we need this include?

Consider the ternary operator here, less code, less impact on coverage for the lines we can't cover. And personally I like that it makes it slightly more obvious that the method inescapable returns a value at a glance.

if the else part of this code doesn't ever execute, how is the idea of simply returning status_proto.value()?
We can get rid of the if-else branching.

Hmm, yes. In fact, I think we can just return status_proto? E.g. condense the whole method body to a one-liner, return Envoy::MessageUtil::getJsonStringFromMessage(output, true, true);

This could be covered in a test as well, I think

Even if we create a test with input that doesn't contain a global result, it gets handled at

therefore, the lines @oschaaf mentioned remain uncovered.

That makes me wonder if it's even possible to hit this code.. if that's not the case it means this is dead code, and we ought to remove it. IMHO that would make sense and is related to why I figured that throwing an exception in this case didn't make sense. It looks like the Fortio UI would indicate an error on the output we'd produce, which is sufficient:
https://github.com/fortio/fortio/blob/ee098c572115f4149a67d2a812cb3d1ca79e30e1/ui/static/js/fortio_chart.js#L105

We should probably make sure that this really works though.

returning here prevents us from calling process->shutdown below, and I'm worried that's a problem. Could we instead set result to false? This would have the same effect except it would call shutdown and log that an error occurred.

Can we avoid this added local copy and just return getYamlStringFromMessage as we were doing before?

Was it intentional for this to stop being a reference that is returned?

This was intentional. Putting a reference inside absl::StatusOr<> was causing a compilation error.
@oschaaf told that it isn't that performance-critical, so can be switch to non-reference.

Note that with this current code you're actually dropping the status result from getAverageExecutionDuration on the floor and returning a more generic message. Could you instead just return it if it's an error? (Related to my comment above about improving the error message there, so that it gives all needed context.).

This message doesn't really give the context about why it's looking. Could we make it something like "No results in output while getting average execution duration" or "getAverageExecutionDuration found no results in output"?

An important drawback of statuses as compared to errors that is important to keep in mind is that they don't have stacktraces.

I know that you and Otto had a conversation here about whether or not to include a line here returning the status if it's not ok() and deciding not to. I think I disagree with the conclusion. Could we please return the status if it's not ok here?

Reasoning:

1. I don't think it's a good idea to code with perfect understanding of a function's error state, because functions aren't static - they change over time. While it may not be possible for getGlobalResult to actually return an error yet, it might be later, and now this code wouldn't operate as expected.

2. Removing lines just because covering them isn't possible is a distorted incentive. Coverage should never discourage us from doing the right thing.

This comment drew me into looking into this once more; what I think might be appropriate is for getGlobalResult() is to return an absl:::optional<const nighthawk::client::Result> instead of a status: I can't anticipate failures modes where an exception wouldn't be appropriate, and not having a global result in the output is business as usual in terms of transforming to Fortio output.

I wasn't calling it out in other pre-existing locations, but we should try to avoid using auto unless the type is fully obvious from context (which it isn't here). Can you please declare the full type?

Oh I think we need:

if (!formatted_proto.ok()) {
  // .... <snip> looks ok to me as-is.
} else {
  // only when formatted_proto.ok() == true
  std::cout << *formatted_proto;
}

I'm sorry if my earlier comment here was off or caused confusion.