templates: Break out build/test summaries

[mrbazzan]

Another approach to #310. It should potentially close #290. Closes #290

Hi @spbnick. Please review when you can.

[mrbazzan]

Hi @spbnick, silent reminder to check this PR when you can

[spbnick]

Hi @mrbazzan, sorry for the long delay. I have been preparing for and attending the Linux Plumbers conference in the past couple of weeks. Flying home now, and will get back to work next week. I took a brief look at the PR a while ago, but didn't have time to give you a extensive review. One thing I can say already, though, I would prefer to avoid defining extra filters if at all possible. We should be able to process our data in pure Jinja2, if that's too difficult we can change the data we're exposing to make the code simpler, in the worst case we should just make our requirements simpler :)

[mrbazzan]

Hi @mrbazzan, sorry for the long delay. I have been preparing for and attending the Linux Plumbers conference in the past couple of weeks. Flying home now, and will get back to work next week.

Hi. No problem. How was the conference? safe journey :)

I took a brief look at the PR a while ago, but didn't have time to give you a extensive review. One thing I can say already, though, I would prefer to avoid defining extra filters if at all possible. We should be able to process our data in pure Jinja2, if that's too difficult we can change the data we're exposing to make the code simpler, in the worst case we should just make our requirements simpler :)

I thought as much; is there any reason for not using extra filters? I'll try to do it with pure Jinja2 again while awaiting an extensive review.

[spbnick]

Thank you, @mrbazzan, the travel went smoothly, no problems there, but unfortunately I was among the few who caught COVID at the conference, and am now recovering.

I'd prefer avoiding extra filters because it makes the system much more obscure and complicated. You have to jump between Jinja2 and Python code to figure it out. The worst part is that the Python code is so specific. I'm more-or-less OK with adding some generic filters, like basic string or container operations and such, if we really have to, but not the highly-specific operations. If we want to make the Jinja2 code simpler it might be better to rethink our object representation and the available fields to help it do what it needs.

[mrbazzan]

Thank you, @mrbazzan, the travel went smoothly, no problems there, but unfortunately I was among the few who caught COVID at the conference, and am now recovering.

That's sad to hear. Sorry about that. :(
Get well soon!

I'd prefer avoiding extra filters because it makes the system much more obscure and complicated. You have to jump between Jinja2 and Python code to figure it out. The worst part is that the Python code is so specific. I'm more-or-less OK with adding some generic filters, like basic string or container operations and such, if we really have to, but not the highly-specific operations.

I see. This makes sense.

If we want to make the Jinja2 code simpler it might be better to rethink our object representation and the available fields to help it do what it needs.

Since, I'm working on it again (using pure Jinja2) — I'll let you know if I find it necessary. By the way, what do you mean by "rethink our object representation"?

[spbnick]

Since, I'm working on it again (using pure Jinja2) — I'll let you know if I find it necessary. By the way, what do you mean by "rethink our object representation"?

Add some fields or change existing fields, make values more uniform, define some fields in base classes, etc.

[mrbazzan]

I'm more-or-less OK with adding some generic filters, like basic string or container operations and such, if we really have to, but not the highly-specific operations.

Hi @spbnick,

On generic filters, a filter like that compares each element of two iterables and return the maximum by length.

Does that count as generic?

[spbnick]

On generic filters, a filter like that compares each element of two iterables and return the maximum by length.

Does that count as generic?

This is still fairly specific, as we need that only in a couple places (I assume), and it could be implemented in Jinja2.

Something like this could be generic enough to get a filter: https://gitlab.com/cki-project/kpet/-/issues/36

[mrbazzan]

Okay. 👍🏿

[mrbazzan]

Hello @spbnick,
The failing tests are not from my end. It has something to do with the action setting up Python3.6.

Please review when you can.

[mrbazzan]

@spbnick ping.

[spbnick]

This is looking better, thank you, Abbdulwasiu!

We're getting there. I like how you tucked the overview generation into a macro (there's one inline comment about its placement, BTW).

However, I acutely felt the lack of documentation and comments explaining what's exactly going on, and I have to admit I didn't manage to understand everything in the limited time I had 🙈

Would be great to have some comments explaining what each chunk of such complex code does. OTOH, it wasn't necessary to split writing the overview macro into multiple commits. Just a single commit adding it would be better. Especially since the macro would have no use until it was complete. Adding code comments would've been more useful (although I appreciate trying to split it down and comment in commit messages). And perhaps your code could start the convention of documenting Jinja2 macros in KCIDB 😁

I wonder if we could simplify this further.

Could we perhaps add a macro called e.g. container_emoji_counts to both build.j2 and test.j2, and make each macro take an output dictionary, and a build/test container respectively as the arguments. The macros could then fill the output dictionary with status emojis -> count entries. E.g. the one for a test container could produce:

{
    "❌": 2,
    "✅": 21,
    "🚧✅": 4,
    "🚧❓": 2,
}

The build one would produce the same output, but with a limited set of three emojis. E.g.:

{
    "❌": 20,
    "✅": 60,
    "❓": 2,
}

Then we could have a macro in misc.j2 which would accept an output list and a list of such dictionaries as the arguments, iterate over all possible status emoji strings, highest-priority first (we could hardcode both of those for the start, I suppose), find the longest counts for each, and output the overview strings corresponding to each of them, also substituting the "➖" emoji for padding as needed. E.g. for the above two dictionaries, in order, it would produce:

[
    "❌  2 ✅ 21 ➖   🚧✅ 4 🚧❓ 2",
    "❌ 20 ✅ 60 ❓ 2"
]

Perhaps a simpler interface for that macro could be to simply accept a variable number of arguments (see varargs in Jinja2 docs), each being an emoji count dictionary, and output the overview string for the first one.

So, if we call that macro e.g. emoji_overview, and have the above emoji counts, the Jinja2 code like this:

{{ set build_emoji_counts = {} }}
{% set _ = build_macros.container_emoji_counts(build_emoji_counts, revision) %}
{{ set test_emoji_counts = {} }}
{% set _ = test_macros.container_emoji_counts(test_emoji_counts, revision) %}

    Builds: {{ misc_macros.emoji_overview(build_emoji_counts, test_emoji_counts) }}
     Tests: {{ misc_macros.emoji_overview(test_emoji_counts, build_emoji_counts) }}

Would produce this output:

    Builds: ❌  2 ✅ 21 ➖   🚧✅ 4 🚧❓ 2
     Tests: ❌ 20 ✅ 60 ❓ 2

What do you think?

[spbnick]

The misc.j2 is more general-purpose than test.j2, so it's best not to include the latter into the former, or include loops may occur.

A better place for macros you're adding could be a separate file, e.g. called overview.j2.

[mrbazzan]

Hi @spbnick

Then we could have a macro in misc.j2 which would accept an output list and a list of such dictionaries as the arguments, iterate over all possible status emoji strings, highest-priority first (we could hardcode both of those for the start, I suppose), find the longest counts for each, and output the overview strings corresponding to each of them, also substituting the "heavy_minus_sign" emoji for padding as needed. E.g. for the above two dictionaries, in order, it would produce:

What do you mean by "hardcode"? Do you mean the first three emojis [❌, ✅, ❓]

How do you think about this outputs?

    Builds: ❌  2 ✅ 21 ➖   🚧✅ 4
     Tests: ❌ 20 ➖    ❓ 2

    Builds: ❌  2 ➖  ➖   🚧✅ 4 
     Tests: ❌ 20 ➖ ❓2

In this cases, should I just remove the PASS ( and UNKNOWN) completely since both Build and Test don't record any case?

    Builds: ❌  2 ➖  ➖   🚧✅ 4 
     Tests: ❌ 20 ➖  ❓2

    Builds: ❌  2 ➖ ➖ 🚧✅ 4 
     Tests: ❌ 20 ➖ ➖ 

[spbnick]

Then we could have a macro in misc.j2 which would accept an output list and a list of such dictionaries as the arguments, iterate over all possible status emoji strings, highest-priority first (we could hardcode both of those for the start, I suppose), find the longest counts for each, and output the overview strings corresponding to each of them, also substituting the "heavy_minus_sign" emoji for padding as needed. E.g. for the above two dictionaries, in order, it would produce:

What do you mean by "hardcode"? Do you mean the first three emojis [x, white_check_mark, question]

I meant put the list of emoji strings and their priorities in whatever way necessary for our functions, directly into the template code (beside them or inside them, whatever works). It would duplicate the similar information in kcidb.oo and would introduce the risk of them going out of sync, but it would be good enough for the start.

How do you think about this outputs?

    Builds: ❌  2 ✅ 21 ➖   🚧✅ 4
     Tests: ❌ 20 ➖    ❓ 2

    Builds: ❌  2 ➖  ➖   🚧✅ 4 
     Tests: ❌ 20 ➖ ❓2

Yes, these look pretty good!

The last line looks unaligned (which I suppose is a typo), and we have the unnecessary blank column which we discuss below, but otherwise they look nice, thanks!

In this cases, should I just remove the PASS ( and UNKNOWN) completely since both Build and Test don't record any case?

    Builds: ❌  2 ➖  ➖   🚧✅ 4 
     Tests: ❌ 20 ➖  ❓2

    Builds: ❌  2 ➖ ➖ 🚧✅ 4 
     Tests: ❌ 20 ➖ ➖ 

Yes, we need to remove the statuses which are not used by either, otherwise listing all the possible statuses would be overwhelming most of the time.

[mrbazzan]

Hello @spbnick, ready for review.

The failing tests are not from any files I edited.

[mrbazzan]

Ping.

[spbnick]

Hi Abdulwasiu! Sorry it's taking so long, by I've been swamped with work the past couple of months, and am still struggling with long COVID symptoms. Now that we're participating in Outreachy there's even less time. Hopefully I'll get to this soon, though. Thank you.

[mrbazzan]

Sure. No problem.

[mrbazzan]

Hi @spbnick, silent reminder to check this when you can

[spbnick]

Thank you, Abdulwasiu. This is quite close to what I was talking about. I went ahead and refined this further in the two commits I put on top. Mostly streamlining the code, and also adding support for handling arbitrary number of emoji dictionaries (what we're using varargs for), not just two. I haven't tested it thoroughly, and it's probably wrong, but this is the direction I think we need to go. Please take a look, ask any questions you have, and take over to finish this.

Thank you, and sorry it took me so long to get around to this.

[spbnick]

Thank you for the comments, they really help!

However, Jinja2 comment syntax is {# ... #}, not # .... Your code works without side-effects only because you assign the result of this macro to a throw-away variable (_). Otherwise they would end in the output.

[spbnick]

Oh, and please rebase this on the latest main!

[mrbazzan]

Mostly streamlining the code, and also adding support for handling arbitrary number of emoji dictionaries (what we're using varargs for), not just two.

Thanks for this. You did the brunt of the job 😄

Thank you, and sorry it took me so long to get around to this.

No problem. Please review when you can

[mrbazzan]

@spbnick
Silent ping

[spbnick]

Thank you @mrbazzan ❤️, I had some time to look at this this week and most of it looks good, and I'm trying to come up with a test framework to test this. I have only a few comments, which I'll post once I figure it out.

[mrbazzan]

Thank you @mrbazzan ❤️, I had some time to look at this this week

Thanks for taking the time. 👍🏿

I'm trying to come up with a test framework to test this. I have only a few comments, which I'll post once I figure it out.

Sure. I'll be waiting for the comments.

NB: I've been super free these days, if there is any urgent (or not) issues you need work done on. I'll be glad to help