Refactoring printer code into functions and adding unit tests

[wjhoward]

@hatoo would it be helpful if I work through adding some unit tests? printer - percentiles and print_distribution tests included as examples of the approach I would use. Let me know what you think?

If so I can work through adding more tests as part of this PR.

[hatoo]

Thank you!

Adding unit tests is good.

Regarding testing the printer, the original implementation was bad because test_percentiles and print_distribution are doing the same data process.

I added percentile_iter and please test it to test the numbers.
And if you want to test outputs, just test the format itself e.g. testing indent (no testing numbers).

[hatoo]

Just #[test] and no async fn would be enough

[hatoo]

true?

[hatoo]

I prefer unwrap at above

[wjhoward]

And if you want to test outputs, just test the format itself e.g. testing indent (no testing numbers).

@hatoo to clarify are you suggesting checking the full output rather than checking if the output contains substrings? The reason I was testing print_distribution for a specific set of substrings was so that if we changed the color scheme the test wouldn't immediately fail, but can simply check the entire string including colors and indentation etc if preferred? The checking of numbers was merely to check they were placed in the right format / position rather than aimed at testing the actual values generated.

My other thought is that it could be worth moving some of the calculations into functions to allow unit testing, e.g:

oha/src/printer.rs

Lines 208 to 213 in 21aa87d

	slowest: res
	.iter()
	.filter_map(|r| r.as_ref().ok())
	.map(|r| r.duration().as_secs_f64())
	.collect::<average::Max>()
	.max(),

to something like:

slowest: slowest(res)

fn slowest(res: &[Result<RequestResult, E>]){
    res
    .iter()
    .filter_map(|r| r.as_ref().ok())
    .map(|r| r.duration().as_secs_f64())
    .collect::<average::Max>()
    .max()
}

[hatoo]

@hatoo to clarify are you suggesting checking the full output rather than checking if the output contains substrings?

No, what I mean is you can test such like the "align"
e.g. checking outputs are not like

10% in 10.0000 secs
25% in 10.0000 secs
50%            in 100.0000 secs
75%        in 100.0000 secs
90%     in 100.0000 secs
95% in 100.0000 secs
99% in 100.0000 secs

by only using pure text processing.

However, since you think

The checking of numbers was merely to check they were placed in the right format / position rather than aimed at testing the actual values generated.

I feel that's OK 👍

My other thought is that it could be worth moving some of the calculations into functions to allow unit testing, e.g:

It looks good!

[wjhoward]

This is still a work in progress, but I thought that it makes sense to push the work done so far for more immediate feedback. I still need to write more tests and possibly refactor more code into functions.

[wjhoward]

@hatoo I've now refactored a large chunk of printer code into functions and added corresponding unit tests, this also has the benefit of re-using the same code for the printer and JSON output. (The only missing component is the size_per_request calculation which I've not yet added a function for given it differs for the printer and JSON output).

I'm now thinking maybe it makes sense to remove the output testing (so far only the test_print_distribution function) and just stick to calculation tests) maybe easier to just test the output as part of the integration tests?

Let me know what you think?

[hatoo]

For size_per_request, I think printer's calculation is better. JSON's one should be replaced.

I'm now thinking maybe it makes sense to remove the output testing (so far only the test_print_distribution function) and just stick to calculation tests) maybe easier to just test the output as part of the integration tests?

Agree. In this PR, please only add calculation tests and add output another in PR if you think it's needed.

[hatoo]

Good

[wjhoward]

For size_per_request, I think printer's calculation is better. JSON's one should be replaced.

I'm now thinking maybe it makes sense to remove the output testing (so far only the test_print_distribution function) and just stick to calculation tests) maybe easier to just test the output as part of the integration tests?

Agree. In this PR, please only add calculation tests and add output another in PR if you think it's needed.

I've made these changes, overall I think this is probably enough changes for one PR, and covers the core printer calculations. What are your thoughts @hatoo ?

[hatoo]

LGTM, Thanks! 🚀