-
Notifications
You must be signed in to change notification settings - Fork 33
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
Codec summary report #566
Codec summary report #566
Conversation
2e040bd
to
18e5158
Compare
Hello @denisyuji @helen-fornazier
Please let me know your feedback on this. |
18e5158
to
dbe2f2e
Compare
@JenySadadia thanks for this work. I was just wondering if it wouldn't be better to point to this instead: I also noticed that it takes some time for the tests to be run after the checkout. For instance, by the time of this post we still don't have tests for the last checkout, so I was wondering how you handle this. |
Sure, I'll update the report to point it to the new dashboard.
We are sending out notifications after 3 hours when a revision create/update event is received. Tests will be executed in that waiting period. |
dbe2f2e
to
7d6308f
Compare
Do you consider MISS/ERROR/SKIP tests in total count? |
5f9cd1a
to
304fc48
Compare
Updated templates to display |
Yes |
Report v2:
|
14f7de3
to
cf7657e
Compare
@helen-fornazier @denisyuji |
@JenySadadia , this looks good, good work!
The result is displayed at |
At the moment, maestro is not sending regressions to KCIDB. Hence, I don't think that is possible along with this report. However, we can send this information directly from maestro pipeline. |
cf7657e
to
4045777
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Jeny!
I added some inline comments, a draft amendment, and also a change to raise the limit on notification body size. I forgot we had that, and that was where the ✂️ came from 🙈
Please move your logic for fluster tests from Python to Jinja template code, either the way I did it, or whatever way you prefer. Do not keep my Thank you! Also, I think we need to make the notification generation a bit more flexible, so you could prepare data to be rendered inside the subscription, and then pass it to Jinja. |
As you have contributed to the PR by converting python code to Jinja2, I would like to keep your name in commits with |
Templates for generating fluster tests report. Signed-off-by: Jeny Sadadia <[email protected]> Co-authored-by: Nikolai.Kondrashov <[email protected]>
Script for generating email notifications for codec summary report. Signed-off-by: Jeny Sadadia <[email protected]>
6cd2216
to
5cbbadc
Compare
Report v3:
|
Update test to increate body text size according to increased maximum allowed size for notification body to keep it over limit length. Signed-off-by: Jeny Sadadia <[email protected]>
5cbbadc
to
b930d10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I left one non-blocking comment inline, for later.
Thank you, Jeny!
"gstreamer_h264_frext": "h264-frext", | ||
"gstreamer_h265": "h265", | ||
"gstreamer_vp8": "vp8", | ||
"gstreamer_vp9": "vp9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't need this map, we can have a list of current keys instead, and just cut off gstreamer_
from the names to get the current values. But we can do that separately next time. Just spotted this.
Generate codec summary report and email notifications for the same.
Created on top of #565 to use
common.j2
.