refactor: make the bucketing more explicit

[jsilvela]

Closes #11

[jsilvela]

heeding deprecation warning

[mnencia]

Good refactor, it makes the bucketing shape simpler.

Left three comments inline, none of them are blocking. One is about a small behavior change that maybe deserves one line in the PR body, the other two are just small things.

[mnencia]

test_summary.py does not exercise compute_bucketized_summary or any format_* function, and the fixture (few-artifacts/) gives to every asserted structure exactly one bucket key. This is not hypothetical: an earlier version of this refactor had compute_bucketized_summary that was summing run-counts instead of counting distinct buckets, and also a missing zero-failure filter, and both problems were caught by eyeballing the output, not by a test that failed.

It is worth to add one fixture case with two distinct keys in a single structure, plus one assertion on compute_bucketized_summary, so a similar regression will fail loudly next time. Not a blocker for this PR, only a gap worth to close.

Separately, one real behavior change entered that is worth to mention in the PR body: this line was keeping the first error text seen for a given file:line, now it keeps the last one, and which one this is depends on the order of os.listdir. Is this intentional, or should it go back to first-write-wins?

[jsilvela]

Thanks Marco. Your observation is true, and I think I'm going to exercise this in a unit test.
The change of order dependence you found is right, and not intentional. Also not necessarily wrong.

[mnencia]

In track_time_taken, suite_times[platform] is mixing together the platform aggregate (start_time/end_time, values are datetime) with the per-matrix_id entries (values are dict), like if they were the same kind of key. This is why format_suite_durations_table needs to skip the matrix_id, when it is literally the string "start_time" or "end_time". If some day a real matrix_id has this name, it will overwrite the aggregate and the next comparison will raise a TypeError.

Maybe it is better to separate them from the start, something like:

suite_times[platform] = {
    "aggregate": {"start_time": start_time, "end_time": end_time},
    "matrices": {
        matrix_id: {"start_time": start_time, "end_time": end_time},
    },
}

This way there is no name that can collide anymore, and the special case in the formatting function can be removed. Not urgent since no real matrix_id has this name today, only worth to fix if this part is touched again.

[jsilvela]

Thanks Marco. This bugs me. I was thinking of having two separate data structures, but I like your idea. Tomorrow I may try it on.

[mnencia]

In this function and in count_bucketed_by_code, count_bucketed_by_special_failures and count_bucketized_stats, the newEntry dict is built on every call, even in the case where the key already exists and newEntry is thrown away. Small thing, but it can be avoided by building the dict only inside the if key not in buckets: branch, for example:

if name not in by_test:
    by_test[name] = {
        "total": 0,
        "failed": 0,
        "k8s_versions_failed": {},
        "pg_versions_failed": {},
        "platforms_failed": {},
    }
test_bucket = by_test[name]

Same idea for the other three functions. Not important for correctness, only for not wasting one allocation each call.