Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

templates: Break out build/test summaries #310

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mrbazzan
Copy link
Contributor

Closes #290

@mrbazzan mrbazzan changed the title templates: Change overview for build templates: Break out build/test summaries May 20, 2022
Copy link
Contributor Author

@mrbazzan mrbazzan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spbnick
Please review when you can.

@@ -11,13 +11,12 @@ for this checkout so far. See complete and up-to-date report at:

OVERVIEW

Checkout: {{ misc_macros.valid_badge(checkout.valid) }}
Checkout: {{ misc_macros.valid_badge(checkout.valid) }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so the summaries will align in the mail. Something like this;

Build:   1V 2F
 Test:   4V 3E

@spbnick
Copy link
Collaborator

spbnick commented Jun 2, 2022

Thank you for this PR, @mrbazzan! Sorry it's taking me so long. I've been very busy with #311, which I hope to finish this week, but next week I'm on vacation, so it would take me still longer to get to it.

@mrbazzan
Copy link
Contributor Author

mrbazzan commented Jun 2, 2022

I've been very busy with #311

#311 👍🏿 💯

next week, I'm on vacation, so it would take me still longer to get to it.

@spbnick No problem. Have a nice vacation :)

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this @mrbazzan, and waiting for me to get to this PR ☺️

I would say this is a pretty good start, and it works almost as needed, it seems. However, this could produce unaligned output in some cases (sample1.json.gz), like this:

        Builds:   ❌ 2  ✅ 208
         Tests:   ❌ 14  ✅ 9

While other cases (sample2.json.gz) look better, but still not perfect:

        Builds:   ❌ 34  ✅ 867
         Tests:   ❌ 47  ✅ 45  🚧❌ 6  🚧✅ 4

It would be good to instead get each emoji aligned on its own column, as seen in the current builds summary section, including using the character where results are missing, and to have numbers aligned right in each field.

I.e., for both of the outputs above, respectively:

        Builds:   ❌  2  ✅ 208
         Tests:   ❌ 14  ✅   9
        Builds:   ❌ 34  ✅ 867
         Tests:   ❌ 47  ✅  45  🚧❌ 6  🚧✅ 4

You can generate emails for the above results using commands like this:

zcat sample1.json.gz | kcidb-notify '>revision#' > output.eml

Finally, for easier review and manipulating changes later, it's better to split your changes into commits refactoring the existing code step-by-step, with explanations, and commits that build upon the refactoring to introduce actual behavior changes, instead of grouping commits by changed areas/files. I.e. in your case you could have a few commits extracting the sub macros and adding new macros, which shouldn't be changing anything, and could be tested not to, and then have commits switching to the new behavior in the overview. Could you do that?

Thank you!

kcidb/templates/build.j2 Show resolved Hide resolved
@mrbazzan
Copy link
Contributor Author

mrbazzan commented Jun 20, 2022

Thank you for working on this @mrbazzan, and waiting for me to get to this PR relaxed

👍🏿 . I'll start refactoring the PR this week.
It might take some time as I'm busy with some personal things...

@mrbazzan
Copy link
Contributor Author

Thanks a lot for the sample data.

@mrbazzan
Copy link
Contributor Author

Hi @spbnick,
How's it going? I have been able to refactor the commits as you mentioned above, but I have issues properly formatting each emoji on its own column in the OVERVIEW section.
This is mostly because the data used for the Test and Build are from different methods. Any help/insight will be appreciated

@spbnick
Copy link
Collaborator

spbnick commented Jul 19, 2022

Hi @mrbazzan! Nice to have you back 🙂 What exact problems do you have? Would you like to perhaps push your code so far, so that we can discuss it?

Copy link
Contributor Author

@mrbazzan mrbazzan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spbnick
The Build and Test in the OVERVIEW section for each object type is still pretty much unaligned.

kcidb/templates/build.j2 Show resolved Hide resolved
@spbnick
Copy link
Collaborator

spbnick commented Jul 29, 2022

Thank you @mrbazzan, and sorry for the late review - this week was hard 😂

I like the new commit breakdown, thank you!

I think what you need to achieve consistent formatting is to generate a single format string accommodating both test and builds summary lines for a container, and taking an optional container name (e.g. architecture name). So basically calculate the maximum count for each category (PASS/FAIL/DONE/SKIP/UNKNOWN/etc. multiplied by NOT-WAIVED/WAIVED/UNKNOWN) across both builds and tests, omitting categories with zero counts, and generate the format string with appropriate width. Then have a macro taking the format strings and counts for corresponding categories (as a list, or a dictionary?) and outputting the summary line.

E.g. have a macro calculate counts for tests for a container and returning a list, or a dictionary in a particular format, and have another doing the same for builds, returning the same format. Add them up to generate the format string with another macro, and pass both of these count list/dictionaries separately to the macro for producing the corresponding summary line.

Make sure the formatting string/macro also accepts the optional container name (e.g. architecture), and then you would also be able to use them for other summaries. Or maybe even forgo passing around the format string and just pass around the counters (and the container name), and embed the format string generation in the summary-formatting macro.

In any case, I would try going this way. If this turns out to be too cumbersome/awkward in Jinja2, then perhaps we could compromise and produce a somewhat uglier, but much simpler to code summary 😁

if unknown_build_count else "") -}}
{% endmacro %}

{%- macro build_status(container, space="") -%}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be named something like container_status, since this is producing a build status summary for a container, not one particular build. Besides the file is already called build.j2 and having "build" in the name would be repeating ourselves.

invalid_build_count,
valid_build_count,
unknown_build_count,
build_fmt) -%}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called architecture_status, as in "producing a summary for builds in an "architecture" container". However, it might turn out that we don't need this particular separate macro after all.

@mrbazzan
Copy link
Contributor Author

mrbazzan commented Aug 22, 2022

Thank you @mrbazzan, and sorry for the late review - this week was hard joy

👍🏿
Hi @spbnick, the last two months have been really hectic. Thanks for being patient while I address this PR

I think what you need to achieve consistent formatting is to generate a single format string accommodating both test and builds summary lines for a container, and taking an optional container name (e.g. architecture name). So basically calculate the maximum count for each category (PASS/FAIL/DONE/SKIP/UNKNOWN/etc. multiplied by NOT-WAIVED/WAIVED/UNKNOWN) across both builds and tests, omitting categories with zero counts, and generate the format string with appropriate width. Then have a macro taking the format strings and counts for corresponding categories (as a list, or a dictionary?) and outputting the summary line.

E.g. have a macro calculate counts for tests for a container and returning a list, or a dictionary in a particular format, and have another doing the same for builds, returning the same format. Add them up to generate the format string with another macro, and pass both of these count list/dictionaries separately to the macro for producing the corresponding summary line.

Thanks 👍🏿

Make sure the formatting string/macro also accepts the optional container name (e.g. architecture), and then you would also be able to use them for other summaries. Or maybe even forgo passing around the format string and just pass around the counters (and the container name), and embed the format string generation in the summary-formatting macro.

I'm still confused about this part. I'm sure it has something to do with being able to use the build format string with the various architectures and having them properly aligned, right?. After I get the initial feedback, I think it should be possible to improve the new implementation to add this :)

CC #320.
Thanks a lot for this thorough explanation. It took me a while before I actually understood your approach to solving the problem.
#320 is more or less similar to the approach you have explained above; it uses custom filters to perform most of the functionality instead of doing in the .j2 files. I also separated out the overview functionality(I'm sure you might be skeptical about this approach) to stand alone without interfering with the previous logic.

I should probably draft this PR while you check out the new one.

@mrbazzan mrbazzan marked this pull request as draft August 22, 2022 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider breaking out build/test summaries
2 participants