On Mon, Mar 23, 2020 at 11:33:00AM +0100, Jan Tluka wrote:
Fri, Mar 20, 2020 at 04:20:05PM CET, olichtne(a)redhat.com wrote:
>On Mon, Mar 16, 2020 at 07:03:22PM +0100, Jan Tluka wrote:
> +
>> + def single_ping_report(self, ping_config, result):
>> fmt = "From: <{0.client.hostid} ({0.client_bind})> To:
" \
>> "<{0.destination.hostid}
({0.destination_address})>"
>> description = fmt.format(ping_config)
>> - if result["rate"] > 50:
>> - message = "Ping successful --- " + description
>> - self.add_result(True, message, result)
>> - else:
>> - message = "Ping unsuccessful --- " + description
>> - self.add_result(False, message, result)
>> + message = "Ping result --- " + description
>> + self.add_result(result[0], message, result[1])
>
>I think the PASS/FAIL distinction here is unnecessary. If the Ping test
>module returns with a FAIL, this is already automatically represented in
>the results and will be shown in the summary.
>
>This should just be a "passing" result object that reports what was
>returned by the ping module - in other words... what was measured.
>
I think this would be confusing. In FAIL case you would have two
records in the log:
FAIL I could not do ping for whatever reason ... (from Ping test module)
PASS Ping result --- from ... to ... (from the Ping recipe)
I also looked at the Perf code and it also reports FAIL if there are no
measurements to report.
yeah so there is a difference in the Perf code - the FAIL is reported
only if there are no results to report on, these "no results" are when
the Perf test fails - nothing gets parsed into the Parallel/Sequential
perf results. If these get parsed and created then this always returns
PASS and just prints the number.
I wrote a longer email here... but in the end I realized that your Ping
approach is probably equivalent to what Perf does - the PASS/FAIL
matches the PASS/FAIL of the lnst.Test.* module. The structure is
slightly different because Ping doesn't have the additional layer of
parsing measurements.
Still in doubts as I understand your idea behind.
I'll wait for your further comments.
In the end, I think the original patch is correct. No need to fix
anything here.
-Ondrej